Re: r293343 - [ubsan] Sanity-check shift amounts before truncation (fixes PR27271)

[Filipe Cabecinhas via cfe-commits]

Another example + possible fix (two WidthMinusOne, one per (possibly
different) bitwidth): (not fully tested)

int f() {
  return 0 << 0L;
}

diff --git a/lib/CodeGen/CGExprScalar.cpp b/lib/CodeGen/CGExprScalar.cpp
index 40d949dece..5c9055d49a 100644
--- a/lib/CodeGen/CGExprScalar.cpp
+++ b/lib/CodeGen/CGExprScalar.cpp
@@ -2751,8 +2751,9 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo ) {
isa(Ops.LHS->getType())) {
 CodeGenFunction::SanitizerScope SanScope();
 SmallVector Checks;
-llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, Ops.RHS);
-llvm::Value *ValidExponent = Builder.CreateICmpULE(Ops.RHS, WidthMinusOne);
+llvm::Value *WidthMinusOneRHS = GetWidthMinusOneValue(Ops.LHS, Ops.RHS);
+llvm::Value *WidthMinusOneLHS = GetWidthMinusOneValue(Ops.LHS, Ops.LHS);
+llvm::Value *ValidExponent = Builder.CreateICmpULE(Ops.RHS,
WidthMinusOneRHS);

 if (SanitizeExponent) {
   Checks.push_back(
@@ -2768,11 +2769,11 @@ Value *ScalarExprEmitter::EmitShl(const
BinOpInfo ) {
   llvm::BasicBlock *CheckShiftBase = CGF.createBasicBlock("check");
   Builder.CreateCondBr(ValidExponent, CheckShiftBase, Cont);
   CGF.EmitBlock(CheckShiftBase);
-  llvm::Value *BitsShiftedOff =
-Builder.CreateLShr(Ops.LHS,
-   Builder.CreateSub(WidthMinusOne, RHS, "shl.zeros",
- /*NUW*/true, /*NSW*/true),
-   "shl.check");
+  llvm::Value *BitsShiftedOff = Builder.CreateLShr(
+  Ops.LHS,
+  Builder.CreateSub(WidthMinusOneLHS, RHS, "shl.zeros",
+/*NUW*/ true, /*NSW*/ true),
+  "shl.check");
   if (CGF.getLangOpts().CPlusPlus) {
 // In C99, we are not permitted to shift a 1 bit into the sign bit.
 // Under C++11's rules, shifting a 1 bit into the sign bit is
  F


On Mon, Jan 30, 2017 at 11:51 AM, Alex L via cfe-commits
 wrote:
> Hi Vedant,
>
> This commit has caused a compiler crash in our stage 2 green dragon
> ASAN+Ubsan bot
> (http://lab.llvm.org:8080/green/job/clang-stage2-cmake-RgSan_build/). I have
> reverted it in r293475. The following example reproduces the crash with
> -fsanitize=undefined :
>
>   typedef unsigned long long uint64_t;
>   typedef signed long long int64_t;
>
>   uint64_t foo(int64_t x, unsigned i) {
>return x << i;
>   }
>
> Alex
>
>
> On 27 January 2017 at 23:02, Vedant Kumar via cfe-commits
>  wrote:
>>
>> Author: vedantk
>> Date: Fri Jan 27 17:02:44 2017
>> New Revision: 293343
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=293343=rev
>> Log:
>> [ubsan] Sanity-check shift amounts before truncation (fixes PR27271)
>>
>> Ubsan does not report UB shifts in some cases where the shift exponent
>> needs to be truncated to match the type of the shift base. We perform a
>> range check on the truncated shift amount, leading to false negatives.
>>
>> Fix the issue (PR27271) by performing the range check on the original
>> shift amount.
>>
>> Differential Revision: https://reviews.llvm.org/D29234
>>
>> Added:
>> cfe/trunk/test/CodeGen/ubsan-shift.c
>> Modified:
>> cfe/trunk/lib/CodeGen/CGExprScalar.cpp
>>
>> Modified: cfe/trunk/lib/CodeGen/CGExprScalar.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprScalar.cpp?rev=293343=293342=293343=diff
>>
>> ==
>> --- cfe/trunk/lib/CodeGen/CGExprScalar.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Fri Jan 27 17:02:44 2017
>> @@ -2751,8 +2751,8 @@ Value *ScalarExprEmitter::EmitShl(const
>> isa(Ops.LHS->getType())) {
>>  CodeGenFunction::SanitizerScope SanScope();
>>  SmallVector Checks;
>> -llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, RHS);
>> -llvm::Value *ValidExponent = Builder.CreateICmpULE(RHS,
>> WidthMinusOne);
>> +llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, Ops.RHS);
>> +llvm::Value *ValidExponent = Builder.CreateICmpULE(Ops.RHS,
>> WidthMinusOne);
>>
>>  if (SanitizeExponent) {
>>Checks.push_back(
>>
>> Added: cfe/trunk/test/CodeGen/ubsan-shift.c
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ubsan-shift.c?rev=293343=auto
>>
>> ==
>> --- cfe/trunk/test/CodeGen/ubsan-shift.c (added)
>> +++ cfe/trunk/test/CodeGen/ubsan-shift.c Fri Jan 27 17:02:44 2017
>> @@ -0,0 +1,29 @@
>> +// RUN: %clang_cc1 -triple=x86_64-apple-darwin -fsanitize=shift-exponent
>> -emit-llvm %s -o - | FileCheck %s
>> +
>> +// CHECK-LABEL: define i32 @f1
>> +int f1(int c, int shamt) {
>> +// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
>> +// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
>> +  return 1 << (c << shamt);
>> +}
>> +
>> +// CHECK-LABEL: define i32 @f2
>> +int 

Re: r293343 - [ubsan] Sanity-check shift amounts before truncation (fixes PR27271)

[Alex L via cfe-commits]

Hi Vedant,

This commit has caused a compiler crash in our stage 2 green dragon
ASAN+Ubsan bot (
http://lab.llvm.org:8080/green/job/clang-stage2-cmake-RgSan_build/). I have
reverted it in r293475. The following example reproduces the crash with
-fsanitize=undefined :

  typedef unsigned long long uint64_t;
  typedef signed long long int64_t;

  uint64_t foo(int64_t x, unsigned i) {
   return x << i;
  }

Alex


On 27 January 2017 at 23:02, Vedant Kumar via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: vedantk
> Date: Fri Jan 27 17:02:44 2017
> New Revision: 293343
>
> URL: http://llvm.org/viewvc/llvm-project?rev=293343=rev
> Log:
> [ubsan] Sanity-check shift amounts before truncation (fixes PR27271)
>
> Ubsan does not report UB shifts in some cases where the shift exponent
> needs to be truncated to match the type of the shift base. We perform a
> range check on the truncated shift amount, leading to false negatives.
>
> Fix the issue (PR27271) by performing the range check on the original
> shift amount.
>
> Differential Revision: https://reviews.llvm.org/D29234
>
> Added:
> cfe/trunk/test/CodeGen/ubsan-shift.c
> Modified:
> cfe/trunk/lib/CodeGen/CGExprScalar.cpp
>
> Modified: cfe/trunk/lib/CodeGen/CGExprScalar.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/
> CGExprScalar.cpp?rev=293343=293342=293343=diff
> 
> ==
> --- cfe/trunk/lib/CodeGen/CGExprScalar.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Fri Jan 27 17:02:44 2017
> @@ -2751,8 +2751,8 @@ Value *ScalarExprEmitter::EmitShl(const
> isa(Ops.LHS->getType())) {
>  CodeGenFunction::SanitizerScope SanScope();
>  SmallVector Checks;
> -llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, RHS);
> -llvm::Value *ValidExponent = Builder.CreateICmpULE(RHS,
> WidthMinusOne);
> +llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, Ops.RHS);
> +llvm::Value *ValidExponent = Builder.CreateICmpULE(Ops.RHS,
> WidthMinusOne);
>
>  if (SanitizeExponent) {
>Checks.push_back(
>
> Added: cfe/trunk/test/CodeGen/ubsan-shift.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/
> CodeGen/ubsan-shift.c?rev=293343=auto
> 
> ==
> --- cfe/trunk/test/CodeGen/ubsan-shift.c (added)
> +++ cfe/trunk/test/CodeGen/ubsan-shift.c Fri Jan 27 17:02:44 2017
> @@ -0,0 +1,29 @@
> +// RUN: %clang_cc1 -triple=x86_64-apple-darwin -fsanitize=shift-exponent
> -emit-llvm %s -o - | FileCheck %s
> +
> +// CHECK-LABEL: define i32 @f1
> +int f1(int c, int shamt) {
> +// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
> +// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
> +  return 1 << (c << shamt);
> +}
> +
> +// CHECK-LABEL: define i32 @f2
> +int f2(long c, int shamt) {
> +// CHECK: icmp ule i32 %{{.*}}, 63, !nosanitize
> +// CHECK: icmp ule i64 %{{.*}}, 31, !nosanitize
> +  return 1 << (c << shamt);
> +}
> +
> +// CHECK-LABEL: define i32 @f3
> +unsigned f3(unsigned c, int shamt) {
> +// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
> +// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
> +  return 1U << (c << shamt);
> +}
> +
> +// CHECK-LABEL: define i32 @f4
> +unsigned f4(unsigned long c, int shamt) {
> +// CHECK: icmp ule i32 %{{.*}}, 63, !nosanitize
> +// CHECK: icmp ule i64 %{{.*}}, 31, !nosanitize
> +  return 1U << (c << shamt);
> +}
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r293343 - [ubsan] Sanity-check shift amounts before truncation (fixes PR27271)

[Vedant Kumar via cfe-commits]

Author: vedantk
Date: Fri Jan 27 17:02:44 2017
New Revision: 293343

URL: http://llvm.org/viewvc/llvm-project?rev=293343=rev
Log:
[ubsan] Sanity-check shift amounts before truncation (fixes PR27271)

Ubsan does not report UB shifts in some cases where the shift exponent
needs to be truncated to match the type of the shift base. We perform a
range check on the truncated shift amount, leading to false negatives.

Fix the issue (PR27271) by performing the range check on the original
shift amount.

Differential Revision: https://reviews.llvm.org/D29234

Added:
cfe/trunk/test/CodeGen/ubsan-shift.c
Modified:
cfe/trunk/lib/CodeGen/CGExprScalar.cpp

Modified: cfe/trunk/lib/CodeGen/CGExprScalar.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprScalar.cpp?rev=293343=293342=293343=diff
==
--- cfe/trunk/lib/CodeGen/CGExprScalar.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Fri Jan 27 17:02:44 2017
@@ -2751,8 +2751,8 @@ Value *ScalarExprEmitter::EmitShl(const
isa(Ops.LHS->getType())) {
 CodeGenFunction::SanitizerScope SanScope();
 SmallVector Checks;
-llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, RHS);
-llvm::Value *ValidExponent = Builder.CreateICmpULE(RHS, WidthMinusOne);
+llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, Ops.RHS);
+llvm::Value *ValidExponent = Builder.CreateICmpULE(Ops.RHS, WidthMinusOne);
 
 if (SanitizeExponent) {
   Checks.push_back(

Added: cfe/trunk/test/CodeGen/ubsan-shift.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ubsan-shift.c?rev=293343=auto
==
--- cfe/trunk/test/CodeGen/ubsan-shift.c (added)
+++ cfe/trunk/test/CodeGen/ubsan-shift.c Fri Jan 27 17:02:44 2017
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -triple=x86_64-apple-darwin -fsanitize=shift-exponent 
-emit-llvm %s -o - | FileCheck %s
+
+// CHECK-LABEL: define i32 @f1
+int f1(int c, int shamt) {
+// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
+// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
+  return 1 << (c << shamt);
+}
+
+// CHECK-LABEL: define i32 @f2
+int f2(long c, int shamt) {
+// CHECK: icmp ule i32 %{{.*}}, 63, !nosanitize
+// CHECK: icmp ule i64 %{{.*}}, 31, !nosanitize
+  return 1 << (c << shamt);
+}
+
+// CHECK-LABEL: define i32 @f3
+unsigned f3(unsigned c, int shamt) {
+// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
+// CHECK: icmp ule i32 %{{.*}}, 31, !nosanitize
+  return 1U << (c << shamt);
+}
+
+// CHECK-LABEL: define i32 @f4
+unsigned f4(unsigned long c, int shamt) {
+// CHECK: icmp ule i32 %{{.*}}, 63, !nosanitize
+// CHECK: icmp ule i64 %{{.*}}, 31, !nosanitize
+  return 1U << (c << shamt);
+}


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits