[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-06 Thread Adam Magier via cfe-commits 

https://github.com/AdamMagierFOSS updated 
https://github.com/llvm/llvm-project/pull/80515

>From f990fb28b68bb42b35dde4abd9d39f03060e1e3f Mon Sep 17 00:00:00 2001
From: Adam Magier 
Date: Sat, 3 Feb 2024 00:38:54 +0100
Subject: [PATCH] [clang][CodeGen][UBSan] Fixing shift-exponent generation for
 _BitInt

Testing the shift-exponent check with small width _BitInt values exposed
a bug in ScalarExprEmitter::GetWidthMinusOneValue when using the result
to determine valid exponent sizes. False positives were reported for
some left shifts when width(LHS)-1 > range(RHS) and false negatives were
reported for right shifts when value(RHS) > range(LHS). This patch caps
the maximum value of GetWidthMinusOneValue to fit within range(RHS) to
fix the issue with left shifts and fixes a code generation in EmitShr to
fix the issue with right shifts.
---
 clang/lib/CodeGen/CGExprScalar.cpp  | 22 ++-
 clang/test/CodeGen/ubsan-shift-bitint.c | 36 +
 2 files changed, 51 insertions(+), 7 deletions(-)
 create mode 100644 clang/test/CodeGen/ubsan-shift-bitint.c

diff --git a/clang/lib/CodeGen/CGExprScalar.cpp 
b/clang/lib/CodeGen/CGExprScalar.cpp
index 5502f685f6474..df8f71cf1d900 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -774,7 +774,7 @@ class ScalarExprEmitter
   void EmitUndefinedBehaviorIntegerDivAndRemCheck(const BinOpInfo &Ops,
   llvm::Value *Zero,bool 
isDiv);
   // Common helper for getting how wide LHS of shift is.
-  static Value *GetWidthMinusOneValue(Value* LHS,Value* RHS);
+  static Value *GetMaximumShiftAmount(Value *LHS, Value *RHS);
 
   // Used for shifting constraints for OpenCL, do mask for powers of 2, URem 
for
   // non powers of two.
@@ -4115,13 +4115,21 @@ Value *ScalarExprEmitter::EmitSub(const BinOpInfo &op) {
   return Builder.CreateExactSDiv(diffInChars, divisor, "sub.ptr.div");
 }
 
-Value *ScalarExprEmitter::GetWidthMinusOneValue(Value* LHS,Value* RHS) {
+Value *ScalarExprEmitter::GetMaximumShiftAmount(Value *LHS, Value *RHS) {
   llvm::IntegerType *Ty;
   if (llvm::VectorType *VT = dyn_cast(LHS->getType()))
 Ty = cast(VT->getElementType());
   else
 Ty = cast(LHS->getType());
-  return llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth() - 1);
+  // For a given type of LHS the maximum shift amount is width(LHS)-1, however
+  // it can occur that width(LHS)-1 > range(RHS). Since there is no check for
+  // this in ConstantInt::get, this results in the value getting truncated.
+  // Constrain the return value to be max(RHS) in this case.
+  llvm::Type *RHSTy = RHS->getType();
+  llvm::APInt RHSMax = llvm::APInt::getMaxValue(RHSTy->getScalarSizeInBits());
+  if (RHSMax.ult(Ty->getBitWidth()))
+return llvm::ConstantInt::get(RHSTy, RHSMax);
+  return llvm::ConstantInt::get(RHSTy, Ty->getBitWidth() - 1);
 }
 
 Value *ScalarExprEmitter::ConstrainShiftValue(Value *LHS, Value *RHS,
@@ -4133,7 +4141,7 @@ Value *ScalarExprEmitter::ConstrainShiftValue(Value *LHS, 
Value *RHS,
 Ty = cast(LHS->getType());
 
   if (llvm::isPowerOf2_64(Ty->getBitWidth()))
-return Builder.CreateAnd(RHS, GetWidthMinusOneValue(LHS, RHS), Name);
+return Builder.CreateAnd(RHS, GetMaximumShiftAmount(LHS, RHS), Name);
 
   return Builder.CreateURem(
   RHS, llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth()), Name);
@@ -4166,7 +4174,7 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
isa(Ops.LHS->getType())) {
 CodeGenFunction::SanitizerScope SanScope(&CGF);
 SmallVector, 2> Checks;
-llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, Ops.RHS);
+llvm::Value *WidthMinusOne = GetMaximumShiftAmount(Ops.LHS, Ops.RHS);
 llvm::Value *ValidExponent = Builder.CreateICmpULE(Ops.RHS, WidthMinusOne);
 
 if (SanitizeExponent) {
@@ -4184,7 +4192,7 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
   Builder.CreateCondBr(ValidExponent, CheckShiftBase, Cont);
   llvm::Value *PromotedWidthMinusOne =
   (RHS == Ops.RHS) ? WidthMinusOne
-   : GetWidthMinusOneValue(Ops.LHS, RHS);
+   : GetMaximumShiftAmount(Ops.LHS, RHS);
   CGF.EmitBlock(CheckShiftBase);
   llvm::Value *BitsShiftedOff = Builder.CreateLShr(
   Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros",
@@ -4235,7 +4243,7 @@ Value *ScalarExprEmitter::EmitShr(const BinOpInfo &Ops) {
isa(Ops.LHS->getType())) {
 CodeGenFunction::SanitizerScope SanScope(&CGF);
 llvm::Value *Valid =
-Builder.CreateICmpULE(RHS, GetWidthMinusOneValue(Ops.LHS, RHS));
+Builder.CreateICmpULE(Ops.RHS, GetMaximumShiftAmount(Ops.LHS, 
Ops.RHS));
 EmitBinOpCheck(std::make_pair(Valid, SanitizerKind::ShiftExponent), Ops);
   }
 
diff --git a/clang/test/CodeGen/ubsan-shift-bitint.c 
b/clang/test/CodeGen/ubsan-shift-bitint.c
new file mode 100644
index 00

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-06 Thread Adam Magier via cfe-commits 

https://github.com/AdamMagierFOSS updated 
https://github.com/llvm/llvm-project/pull/80515

>From d4b92b247dd55abd0ed1a9f599e77f6417475adf Mon Sep 17 00:00:00 2001
From: Adam Magier 
Date: Sat, 3 Feb 2024 00:38:54 +0100
Subject: [PATCH] [clang][CodeGen][UBSan] Fixing shift-exponent generation for
 _BitInt

Testing the shift-exponent check with small width _BitInt values exposed
a bug in ScalarExprEmitter::GetWidthMinusOneValue when using the result
to determine valid exponent sizes. False positives were reported for
some left shifts when width(LHS)-1 > range(RHS) and false negatives were
reported for right shifts when value(RHS) > range(LHS). This patch caps
the maximum value of GetWidthMinusOneValue to fit within range(RHS) to
fix the issue with left shifts and fixes a code generation in EmitShr to
fix the issue with right shifts and renames the function to
GetMaximumShiftAmount to better reflect the new behaviour.

Fixes #80135.
---
 clang/lib/CodeGen/CGExprScalar.cpp  | 22 ++-
 clang/test/CodeGen/ubsan-shift-bitint.c | 36 +
 2 files changed, 51 insertions(+), 7 deletions(-)
 create mode 100644 clang/test/CodeGen/ubsan-shift-bitint.c

diff --git a/clang/lib/CodeGen/CGExprScalar.cpp 
b/clang/lib/CodeGen/CGExprScalar.cpp
index 5502f685f6474..df8f71cf1d900 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -774,7 +774,7 @@ class ScalarExprEmitter
   void EmitUndefinedBehaviorIntegerDivAndRemCheck(const BinOpInfo &Ops,
   llvm::Value *Zero,bool 
isDiv);
   // Common helper for getting how wide LHS of shift is.
-  static Value *GetWidthMinusOneValue(Value* LHS,Value* RHS);
+  static Value *GetMaximumShiftAmount(Value *LHS, Value *RHS);
 
   // Used for shifting constraints for OpenCL, do mask for powers of 2, URem 
for
   // non powers of two.
@@ -4115,13 +4115,21 @@ Value *ScalarExprEmitter::EmitSub(const BinOpInfo &op) {
   return Builder.CreateExactSDiv(diffInChars, divisor, "sub.ptr.div");
 }
 
-Value *ScalarExprEmitter::GetWidthMinusOneValue(Value* LHS,Value* RHS) {
+Value *ScalarExprEmitter::GetMaximumShiftAmount(Value *LHS, Value *RHS) {
   llvm::IntegerType *Ty;
   if (llvm::VectorType *VT = dyn_cast(LHS->getType()))
 Ty = cast(VT->getElementType());
   else
 Ty = cast(LHS->getType());
-  return llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth() - 1);
+  // For a given type of LHS the maximum shift amount is width(LHS)-1, however
+  // it can occur that width(LHS)-1 > range(RHS). Since there is no check for
+  // this in ConstantInt::get, this results in the value getting truncated.
+  // Constrain the return value to be max(RHS) in this case.
+  llvm::Type *RHSTy = RHS->getType();
+  llvm::APInt RHSMax = llvm::APInt::getMaxValue(RHSTy->getScalarSizeInBits());
+  if (RHSMax.ult(Ty->getBitWidth()))
+return llvm::ConstantInt::get(RHSTy, RHSMax);
+  return llvm::ConstantInt::get(RHSTy, Ty->getBitWidth() - 1);
 }
 
 Value *ScalarExprEmitter::ConstrainShiftValue(Value *LHS, Value *RHS,
@@ -4133,7 +4141,7 @@ Value *ScalarExprEmitter::ConstrainShiftValue(Value *LHS, 
Value *RHS,
 Ty = cast(LHS->getType());
 
   if (llvm::isPowerOf2_64(Ty->getBitWidth()))
-return Builder.CreateAnd(RHS, GetWidthMinusOneValue(LHS, RHS), Name);
+return Builder.CreateAnd(RHS, GetMaximumShiftAmount(LHS, RHS), Name);
 
   return Builder.CreateURem(
   RHS, llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth()), Name);
@@ -4166,7 +4174,7 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
isa(Ops.LHS->getType())) {
 CodeGenFunction::SanitizerScope SanScope(&CGF);
 SmallVector, 2> Checks;
-llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, Ops.RHS);
+llvm::Value *WidthMinusOne = GetMaximumShiftAmount(Ops.LHS, Ops.RHS);
 llvm::Value *ValidExponent = Builder.CreateICmpULE(Ops.RHS, WidthMinusOne);
 
 if (SanitizeExponent) {
@@ -4184,7 +4192,7 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
   Builder.CreateCondBr(ValidExponent, CheckShiftBase, Cont);
   llvm::Value *PromotedWidthMinusOne =
   (RHS == Ops.RHS) ? WidthMinusOne
-   : GetWidthMinusOneValue(Ops.LHS, RHS);
+   : GetMaximumShiftAmount(Ops.LHS, RHS);
   CGF.EmitBlock(CheckShiftBase);
   llvm::Value *BitsShiftedOff = Builder.CreateLShr(
   Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros",
@@ -4235,7 +4243,7 @@ Value *ScalarExprEmitter::EmitShr(const BinOpInfo &Ops) {
isa(Ops.LHS->getType())) {
 CodeGenFunction::SanitizerScope SanScope(&CGF);
 llvm::Value *Valid =
-Builder.CreateICmpULE(RHS, GetWidthMinusOneValue(Ops.LHS, RHS));
+Builder.CreateICmpULE(Ops.RHS, GetMaximumShiftAmount(Ops.LHS, 
Ops.RHS));
 EmitBinOpCheck(std::make_pair(Valid, SanitizerKind::ShiftExponent), Ops);
   }
 
diff --git a/clang/test/Code

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-06 Thread Adam Magier via cfe-commits 

https://github.com/AdamMagierFOSS closed 
https://github.com/llvm/llvm-project/pull/80515
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-02 Thread Adam Magier via cfe-commits 

https://github.com/AdamMagierFOSS created 
https://github.com/llvm/llvm-project/pull/80515

Testing the shift-exponent check with small width _BitInt values exposed a bug 
in ScalarExprEmitter::GetWidthMinusOneValue when using the result to determine 
valid exponent sizes. False positives were reported for some left shifts when 
width(LHS)-1 > range(RHS) and false negatives were reported for right shifts 
when value(RHS) > range(LHS). This patch caps the maximum value of 
GetWidthMinusOneValue to fit within range(RHS) to fix the issue with left 
shifts and fixes a code generation in EmitShr to fix the issue with right 
shifts.

>From 4e1c37ae83dec050fc9b7aa172db01fa0b2b6d68 Mon Sep 17 00:00:00 2001
From: Adam Magier 
Date: Sat, 3 Feb 2024 00:38:54 +0100
Subject: [PATCH] [clang][CodeGen][UBSan] Fixing shift-exponent generation for
 _BitInt

Testing the shift-exponent check with small width _BitInt values exposed
a bug in ScalarExprEmitter::GetWidthMinusOneValue when using the result
to determine valid exponent sizes. False positives were reported for
some left shifts when width(LHS)-1 > range(RHS) and false negatives were
reported for right shifts when value(RHS) > range(LHS). This patch caps
the maximum value of GetWidthMinusOneValue to fit within range(RHS) to
fix the issue with left shifts and fixes a code generation in EmitShr to
fix the issue with right shifts.
---
 clang/lib/CodeGen/CGExprScalar.cpp  |  9 ++-
 clang/test/CodeGen/ubsan-shift-bitint.c | 36 +
 2 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/CodeGen/ubsan-shift-bitint.c

diff --git a/clang/lib/CodeGen/CGExprScalar.cpp 
b/clang/lib/CodeGen/CGExprScalar.cpp
index 5502f685f6474..e2e3ed839714a 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -4121,6 +4121,13 @@ Value *ScalarExprEmitter::GetWidthMinusOneValue(Value* 
LHS,Value* RHS) {
 Ty = cast(VT->getElementType());
   else
 Ty = cast(LHS->getType());
+  // Testing with small _BitInt types has shown that Ty->getBitwidth() - 1
+  // can sometimes overflow the capacity of RHS->getType(), cap the value
+  // to be the largest RHS->getType() can hold
+  llvm::APInt RHSMax =
+  llvm::APInt::getMaxValue(RHS->getType()->getScalarSizeInBits());
+  if (RHSMax.ult(Ty->getBitWidth()))
+return llvm::ConstantInt::get(RHS->getType(), RHSMax);
   return llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth() - 1);
 }
 
@@ -4235,7 +4242,7 @@ Value *ScalarExprEmitter::EmitShr(const BinOpInfo &Ops) {
isa(Ops.LHS->getType())) {
 CodeGenFunction::SanitizerScope SanScope(&CGF);
 llvm::Value *Valid =
-Builder.CreateICmpULE(RHS, GetWidthMinusOneValue(Ops.LHS, RHS));
+Builder.CreateICmpULE(Ops.RHS, GetWidthMinusOneValue(Ops.LHS, 
Ops.RHS));
 EmitBinOpCheck(std::make_pair(Valid, SanitizerKind::ShiftExponent), Ops);
   }
 
diff --git a/clang/test/CodeGen/ubsan-shift-bitint.c 
b/clang/test/CodeGen/ubsan-shift-bitint.c
new file mode 100644
index 0..8ca94b7de5a42
--- /dev/null
+++ b/clang/test/CodeGen/ubsan-shift-bitint.c
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 %s -O0 -fsanitize=shift-exponent -emit-llvm -o - | 
FileCheck %s
+
+// Checking that the code generation is using the unextended/untruncated
+// exponent values and capping the values accordingly
+
+// CHECK-LABEL: define{{.*}} i32 @test_left_variable
+int test_left_variable(unsigned _BitInt(5) b, unsigned _BitInt(2) e) {
+  // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i2]]
+  // CHECK: icmp ule [[E_SIZE]] [[E_REG]], -1
+  return b << e;
+}
+
+// CHECK-LABEL: define{{.*}} i32 @test_right_variable
+int test_right_variable(unsigned _BitInt(2) b, unsigned _BitInt(3) e) {
+  // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i3]]
+  // CHECK: icmp ule [[E_SIZE]] [[E_REG]], 1
+  return b >> e;
+}
+
+// Old code generation would give false positives on left shifts when:
+//   value(e) > (width(b) - 1 % 2 ** width(e))
+// CHECK-LABEL: define{{.*}} i32 @test_left_literal
+int test_left_literal(unsigned _BitInt(5) b) {
+  // CHECK-NOT: br i1 false, label %cont, label %handler.shift_out_of_bounds
+  // CHECK: br i1 true, label %cont, label %handler.shift_out_of_bounds
+  return b << 3uwb;
+}
+
+// Old code generation would give false positives on right shifts when:
+//   (value(e) % 2 ** width(b)) < width(b)
+// CHECK-LABEL: define{{.*}} i32 @test_right_literal
+int test_right_literal(unsigned _BitInt(2) b) {
+  // CHECK-NOT: br i1 true, label %cont, label %handler.shift_out_of_bounds
+  // CHECK: br i1 false, label %cont, label %handler.shift_out_of_bounds
+  return b >> 4uwb;
+}

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

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-02 Thread Adam Magier via cfe-commits 

AdamMagierFOSS wrote:

One thing I'll preemptively address is I didn't know where to put the new unit 
testing - creating a separate file seems a little heavy handed but I see that 
there's a test for UBSan shift generation (`clang/test/CodeGen/ubsan-shift.c`) 
and one for UBSan + _BitInt (`clang/test/CodeGen/ext-int-sanitizer.cpp`). Both 
seem equally "valid" but neither seem to test in the same way that I'm trying 
to test these changes. Advice on this would be appreciated.

https://github.com/llvm/llvm-project/pull/80515
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-05 Thread Adam Magier via cfe-commits 

https://github.com/AdamMagierFOSS edited 
https://github.com/llvm/llvm-project/pull/80515
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-05 Thread Adam Magier via cfe-commits 

https://github.com/AdamMagierFOSS updated 
https://github.com/llvm/llvm-project/pull/80515

>From 4e1c37ae83dec050fc9b7aa172db01fa0b2b6d68 Mon Sep 17 00:00:00 2001
From: Adam Magier 
Date: Sat, 3 Feb 2024 00:38:54 +0100
Subject: [PATCH 1/2] [clang][CodeGen][UBSan] Fixing shift-exponent generation
 for _BitInt

Testing the shift-exponent check with small width _BitInt values exposed
a bug in ScalarExprEmitter::GetWidthMinusOneValue when using the result
to determine valid exponent sizes. False positives were reported for
some left shifts when width(LHS)-1 > range(RHS) and false negatives were
reported for right shifts when value(RHS) > range(LHS). This patch caps
the maximum value of GetWidthMinusOneValue to fit within range(RHS) to
fix the issue with left shifts and fixes a code generation in EmitShr to
fix the issue with right shifts.
---
 clang/lib/CodeGen/CGExprScalar.cpp  |  9 ++-
 clang/test/CodeGen/ubsan-shift-bitint.c | 36 +
 2 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/CodeGen/ubsan-shift-bitint.c

diff --git a/clang/lib/CodeGen/CGExprScalar.cpp 
b/clang/lib/CodeGen/CGExprScalar.cpp
index 5502f685f6474..e2e3ed839714a 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -4121,6 +4121,13 @@ Value *ScalarExprEmitter::GetWidthMinusOneValue(Value* 
LHS,Value* RHS) {
 Ty = cast(VT->getElementType());
   else
 Ty = cast(LHS->getType());
+  // Testing with small _BitInt types has shown that Ty->getBitwidth() - 1
+  // can sometimes overflow the capacity of RHS->getType(), cap the value
+  // to be the largest RHS->getType() can hold
+  llvm::APInt RHSMax =
+  llvm::APInt::getMaxValue(RHS->getType()->getScalarSizeInBits());
+  if (RHSMax.ult(Ty->getBitWidth()))
+return llvm::ConstantInt::get(RHS->getType(), RHSMax);
   return llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth() - 1);
 }
 
@@ -4235,7 +4242,7 @@ Value *ScalarExprEmitter::EmitShr(const BinOpInfo &Ops) {
isa(Ops.LHS->getType())) {
 CodeGenFunction::SanitizerScope SanScope(&CGF);
 llvm::Value *Valid =
-Builder.CreateICmpULE(RHS, GetWidthMinusOneValue(Ops.LHS, RHS));
+Builder.CreateICmpULE(Ops.RHS, GetWidthMinusOneValue(Ops.LHS, 
Ops.RHS));
 EmitBinOpCheck(std::make_pair(Valid, SanitizerKind::ShiftExponent), Ops);
   }
 
diff --git a/clang/test/CodeGen/ubsan-shift-bitint.c 
b/clang/test/CodeGen/ubsan-shift-bitint.c
new file mode 100644
index 0..8ca94b7de5a42
--- /dev/null
+++ b/clang/test/CodeGen/ubsan-shift-bitint.c
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 %s -O0 -fsanitize=shift-exponent -emit-llvm -o - | 
FileCheck %s
+
+// Checking that the code generation is using the unextended/untruncated
+// exponent values and capping the values accordingly
+
+// CHECK-LABEL: define{{.*}} i32 @test_left_variable
+int test_left_variable(unsigned _BitInt(5) b, unsigned _BitInt(2) e) {
+  // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i2]]
+  // CHECK: icmp ule [[E_SIZE]] [[E_REG]], -1
+  return b << e;
+}
+
+// CHECK-LABEL: define{{.*}} i32 @test_right_variable
+int test_right_variable(unsigned _BitInt(2) b, unsigned _BitInt(3) e) {
+  // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i3]]
+  // CHECK: icmp ule [[E_SIZE]] [[E_REG]], 1
+  return b >> e;
+}
+
+// Old code generation would give false positives on left shifts when:
+//   value(e) > (width(b) - 1 % 2 ** width(e))
+// CHECK-LABEL: define{{.*}} i32 @test_left_literal
+int test_left_literal(unsigned _BitInt(5) b) {
+  // CHECK-NOT: br i1 false, label %cont, label %handler.shift_out_of_bounds
+  // CHECK: br i1 true, label %cont, label %handler.shift_out_of_bounds
+  return b << 3uwb;
+}
+
+// Old code generation would give false positives on right shifts when:
+//   (value(e) % 2 ** width(b)) < width(b)
+// CHECK-LABEL: define{{.*}} i32 @test_right_literal
+int test_right_literal(unsigned _BitInt(2) b) {
+  // CHECK-NOT: br i1 true, label %cont, label %handler.shift_out_of_bounds
+  // CHECK: br i1 false, label %cont, label %handler.shift_out_of_bounds
+  return b >> 4uwb;
+}

>From c3be3ebd7a8ae57d319eedbb97ab85324c814db2 Mon Sep 17 00:00:00 2001
From: Adam Magier 
Date: Tue, 6 Feb 2024 00:11:19 +0100
Subject: [PATCH 2/2] Updating with feedback

---
 clang/lib/CodeGen/CGExprScalar.cpp  | 27 +
 clang/test/CodeGen/ubsan-shift-bitint.c |  4 ++--
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/clang/lib/CodeGen/CGExprScalar.cpp 
b/clang/lib/CodeGen/CGExprScalar.cpp
index e2e3ed839714a..f08fb8d4e3b34 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -774,7 +774,7 @@ class ScalarExprEmitter
   void EmitUndefinedBehaviorIntegerDivAndRemCheck(const BinOpInfo &Ops,
   llvm::Value *Zero,bool 
isDiv);
   // Common helper for getting how wide LHS of shift is.
-  static Value *GetWidthMinusOneValue(Value* LHS,Value* RHS)

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-05 Thread Adam Magier via cfe-commits 


@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 %s -O0 -fsanitize=shift-exponent -emit-llvm -o - | 
FileCheck %s

AdamMagierFOSS wrote:

Sounds good, thank you for the recommendation!

https://github.com/llvm/llvm-project/pull/80515
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-05 Thread Adam Magier via cfe-commits 


@@ -4121,6 +4121,13 @@ Value *ScalarExprEmitter::GetWidthMinusOneValue(Value* 
LHS,Value* RHS) {
 Ty = cast(VT->getElementType());
   else
 Ty = cast(LHS->getType());
+  // Testing with small _BitInt types has shown that Ty->getBitwidth() - 1

AdamMagierFOSS wrote:

I've renamed the function to be more accurate, but to keep a consistent style 
with the surrounding code I've not put a formal function description and 
instead kept the explanatory comment (though rewriting it to not include 
mentions of _BitInt).

https://github.com/llvm/llvm-project/pull/80515
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-05 Thread Adam Magier via cfe-commits 


@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 %s -O0 -fsanitize=shift-exponent -emit-llvm -o - | 
FileCheck %s
+
+// Checking that the code generation is using the unextended/untruncated
+// exponent values and capping the values accordingly
+
+// CHECK-LABEL: define{{.*}} i32 @test_left_variable
+int test_left_variable(unsigned _BitInt(5) b, unsigned _BitInt(2) e) {
+  // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i2]]
+  // CHECK: icmp ule [[E_SIZE]] [[E_REG]], -1

AdamMagierFOSS wrote:

I tried to include this type of optimization in this patch but there's an 
assert that expects a check to be generated when the shift-exponent check is 
enabled. I suppose it wouldn't be too difficult to refactor this but from what 
I can tell none of the UBSan checks are really optimized in this way and 
instead rely on the middle end to optimize this out (e.g. the array-bounds 
check still generates a check on an array of size 256 when indexing with 
`uint8_t`). Don't know how much of an impact this type of pre-emptive 
optimization would have either.

https://github.com/llvm/llvm-project/pull/80515
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-05 Thread Adam Magier via cfe-commits 

AdamMagierFOSS wrote:

> > One thing I'll preemptively address is I didn't know where to put the new 
> > unit testing - creating a separate file seems a little heavy handed but I 
> > see that there's a test for UBSan shift generation 
> > (`clang/test/CodeGen/ubsan-shift.c`) and one for UBSan + _BitInt 
> > (`clang/test/CodeGen/ext-int-sanitizer.cpp`). Both seem equally "valid" but 
> > neither seem to test in the same way that I'm trying to test these changes. 
> > Advice on this would be appreciated.
> 
> Either one is fine as long as you aren't having to narrow the portability of 
> a test case in order to use `_BitInt` in it.
> 
> Patch generally LGTM.

After thinking about it some more maybe it would be best to keep this test its 
own file. The other two tests check the optimized code but in this test I 
explicitly want to test the unoptimized code to make sure the base code 
generation is going as anticipated. The way I understand it, either I include 
these test cases in the existing tests and there's a disconnect in how the 
"first part" of the test gets performed vs the "second part" of the test or I 
keep the test separate. Unless there's a strong opinion otherwise I think this 
would be the best way forward.

https://github.com/llvm/llvm-project/pull/80515
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-05 Thread Adam Magier via cfe-commits 

https://github.com/AdamMagierFOSS edited 
https://github.com/llvm/llvm-project/pull/80515
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CodeGen][UBSan] Fixing shift-exponent generation for _BitInt (PR #80515)

2024-02-05 Thread Adam Magier via cfe-commits 

https://github.com/AdamMagierFOSS updated 
https://github.com/llvm/llvm-project/pull/80515

>From 4e1c37ae83dec050fc9b7aa172db01fa0b2b6d68 Mon Sep 17 00:00:00 2001
From: Adam Magier 
Date: Sat, 3 Feb 2024 00:38:54 +0100
Subject: [PATCH 1/3] [clang][CodeGen][UBSan] Fixing shift-exponent generation
 for _BitInt

Testing the shift-exponent check with small width _BitInt values exposed
a bug in ScalarExprEmitter::GetWidthMinusOneValue when using the result
to determine valid exponent sizes. False positives were reported for
some left shifts when width(LHS)-1 > range(RHS) and false negatives were
reported for right shifts when value(RHS) > range(LHS). This patch caps
the maximum value of GetWidthMinusOneValue to fit within range(RHS) to
fix the issue with left shifts and fixes a code generation in EmitShr to
fix the issue with right shifts.
---
 clang/lib/CodeGen/CGExprScalar.cpp  |  9 ++-
 clang/test/CodeGen/ubsan-shift-bitint.c | 36 +
 2 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/CodeGen/ubsan-shift-bitint.c

diff --git a/clang/lib/CodeGen/CGExprScalar.cpp 
b/clang/lib/CodeGen/CGExprScalar.cpp
index 5502f685f64743..e2e3ed839714a2 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -4121,6 +4121,13 @@ Value *ScalarExprEmitter::GetWidthMinusOneValue(Value* 
LHS,Value* RHS) {
 Ty = cast(VT->getElementType());
   else
 Ty = cast(LHS->getType());
+  // Testing with small _BitInt types has shown that Ty->getBitwidth() - 1
+  // can sometimes overflow the capacity of RHS->getType(), cap the value
+  // to be the largest RHS->getType() can hold
+  llvm::APInt RHSMax =
+  llvm::APInt::getMaxValue(RHS->getType()->getScalarSizeInBits());
+  if (RHSMax.ult(Ty->getBitWidth()))
+return llvm::ConstantInt::get(RHS->getType(), RHSMax);
   return llvm::ConstantInt::get(RHS->getType(), Ty->getBitWidth() - 1);
 }
 
@@ -4235,7 +4242,7 @@ Value *ScalarExprEmitter::EmitShr(const BinOpInfo &Ops) {
isa(Ops.LHS->getType())) {
 CodeGenFunction::SanitizerScope SanScope(&CGF);
 llvm::Value *Valid =
-Builder.CreateICmpULE(RHS, GetWidthMinusOneValue(Ops.LHS, RHS));
+Builder.CreateICmpULE(Ops.RHS, GetWidthMinusOneValue(Ops.LHS, 
Ops.RHS));
 EmitBinOpCheck(std::make_pair(Valid, SanitizerKind::ShiftExponent), Ops);
   }
 
diff --git a/clang/test/CodeGen/ubsan-shift-bitint.c 
b/clang/test/CodeGen/ubsan-shift-bitint.c
new file mode 100644
index 00..8ca94b7de5a42d
--- /dev/null
+++ b/clang/test/CodeGen/ubsan-shift-bitint.c
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 %s -O0 -fsanitize=shift-exponent -emit-llvm -o - | 
FileCheck %s
+
+// Checking that the code generation is using the unextended/untruncated
+// exponent values and capping the values accordingly
+
+// CHECK-LABEL: define{{.*}} i32 @test_left_variable
+int test_left_variable(unsigned _BitInt(5) b, unsigned _BitInt(2) e) {
+  // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i2]]
+  // CHECK: icmp ule [[E_SIZE]] [[E_REG]], -1
+  return b << e;
+}
+
+// CHECK-LABEL: define{{.*}} i32 @test_right_variable
+int test_right_variable(unsigned _BitInt(2) b, unsigned _BitInt(3) e) {
+  // CHECK: [[E_REG:%.+]] = load [[E_SIZE:i3]]
+  // CHECK: icmp ule [[E_SIZE]] [[E_REG]], 1
+  return b >> e;
+}
+
+// Old code generation would give false positives on left shifts when:
+//   value(e) > (width(b) - 1 % 2 ** width(e))
+// CHECK-LABEL: define{{.*}} i32 @test_left_literal
+int test_left_literal(unsigned _BitInt(5) b) {
+  // CHECK-NOT: br i1 false, label %cont, label %handler.shift_out_of_bounds
+  // CHECK: br i1 true, label %cont, label %handler.shift_out_of_bounds
+  return b << 3uwb;
+}
+
+// Old code generation would give false positives on right shifts when:
+//   (value(e) % 2 ** width(b)) < width(b)
+// CHECK-LABEL: define{{.*}} i32 @test_right_literal
+int test_right_literal(unsigned _BitInt(2) b) {
+  // CHECK-NOT: br i1 true, label %cont, label %handler.shift_out_of_bounds
+  // CHECK: br i1 false, label %cont, label %handler.shift_out_of_bounds
+  return b >> 4uwb;
+}

>From c3be3ebd7a8ae57d319eedbb97ab85324c814db2 Mon Sep 17 00:00:00 2001
From: Adam Magier 
Date: Tue, 6 Feb 2024 00:11:19 +0100
Subject: [PATCH 2/3] Updating with feedback

---
 clang/lib/CodeGen/CGExprScalar.cpp  | 27 +
 clang/test/CodeGen/ubsan-shift-bitint.c |  4 ++--
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/clang/lib/CodeGen/CGExprScalar.cpp 
b/clang/lib/CodeGen/CGExprScalar.cpp
index e2e3ed839714a2..f08fb8d4e3b349 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -774,7 +774,7 @@ class ScalarExprEmitter
   void EmitUndefinedBehaviorIntegerDivAndRemCheck(const BinOpInfo &Ops,
   llvm::Value *Zero,bool 
isDiv);
   // Common helper for getting how wide LHS of shift is.
-  static Value *GetWidthMinusOneValue(Value* LHS,Value

[clang] b271566 - [clang][CodeGen][UBSan] VLA size checking for unsigned integer parameter

2022-01-11 Thread Adam Magier via cfe-commits 

Author: Adam Magier
Date: 2022-01-12T01:11:52+01:00
New Revision: b2715660ed0f821619f51158fb92cd8bddd105d8

URL: 
https://github.com/llvm/llvm-project/commit/b2715660ed0f821619f51158fb92cd8bddd105d8
DIFF: 
https://github.com/llvm/llvm-project/commit/b2715660ed0f821619f51158fb92cd8bddd105d8.diff

LOG: [clang][CodeGen][UBSan] VLA size checking for unsigned integer parameter

The code generation for the UBSan VLA size check was qualified by a con-
dition that the parameter must be a signed integer, however the C spec
does not make any distinction that only signed integer parameters can be
used to declare a VLA, only qualifying that it must be greater than zero
if it is not a constant.

Reviewed By: rjmccall

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

Added: 


Modified: 
clang/lib/CodeGen/CodeGenFunction.cpp
clang/test/CodeGen/catch-undef-behavior.c

Removed: 




diff  --git a/clang/lib/CodeGen/CodeGenFunction.cpp 
b/clang/lib/CodeGen/CodeGenFunction.cpp
index 640c73f1a556..54ddbff3fb03 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2247,32 +2247,36 @@ void CodeGenFunction::EmitVariablyModifiedType(QualType 
type) {
 
   // Unknown size indication requires no size computation.
   // Otherwise, evaluate and record it.
-  if (const Expr *size = vat->getSizeExpr()) {
+  if (const Expr *sizeExpr = vat->getSizeExpr()) {
 // It's possible that we might have emitted this already,
 // e.g. with a typedef and a pointer to it.
-llvm::Value *&entry = VLASizeMap[size];
+llvm::Value *&entry = VLASizeMap[sizeExpr];
 if (!entry) {
-  llvm::Value *Size = EmitScalarExpr(size);
+  llvm::Value *size = EmitScalarExpr(sizeExpr);
 
   // C11 6.7.6.2p5:
   //   If the size is an expression that is not an integer constant
   //   expression [...] each time it is evaluated it shall have a value
   //   greater than zero.
-  if (SanOpts.has(SanitizerKind::VLABound) &&
-  size->getType()->isSignedIntegerType()) {
+  if (SanOpts.has(SanitizerKind::VLABound)) {
 SanitizerScope SanScope(this);
-llvm::Value *Zero = llvm::Constant::getNullValue(Size->getType());
+llvm::Value *Zero = llvm::Constant::getNullValue(size->getType());
+clang::QualType SEType = sizeExpr->getType();
+llvm::Value *CheckCondition =
+SEType->isSignedIntegerType()
+? Builder.CreateICmpSGT(size, Zero)
+: Builder.CreateICmpUGT(size, Zero);
 llvm::Constant *StaticArgs[] = {
-EmitCheckSourceLocation(size->getBeginLoc()),
-EmitCheckTypeDescriptor(size->getType())};
-EmitCheck(std::make_pair(Builder.CreateICmpSGT(Size, Zero),
- SanitizerKind::VLABound),
-  SanitizerHandler::VLABoundNotPositive, StaticArgs, Size);
+EmitCheckSourceLocation(sizeExpr->getBeginLoc()),
+EmitCheckTypeDescriptor(SEType)};
+EmitCheck(std::make_pair(CheckCondition, SanitizerKind::VLABound),
+  SanitizerHandler::VLABoundNotPositive, StaticArgs, size);
   }
 
   // Always zexting here would be wrong if it weren't
   // undefined behavior to have a negative bound.
-  entry = Builder.CreateIntCast(Size, SizeTy, /*signed*/ false);
+  // FIXME: What about when size's type is larger than size_t?
+  entry = Builder.CreateIntCast(size, SizeTy, /*signed*/ false);
 }
   }
   type = vat->getElementType();

diff  --git a/clang/test/CodeGen/catch-undef-behavior.c 
b/clang/test/CodeGen/catch-undef-behavior.c
index 55f6e420a4fc..c10dd2504073 100644
--- a/clang/test/CodeGen/catch-undef-behavior.c
+++ b/clang/test/CodeGen/catch-undef-behavior.c
@@ -17,6 +17,7 @@
 // CHECK-UBSAN: @[[LINE_700:.*]] = {{.*}}, i32 700, i32 14 {{.*}} 
@[[STRUCT_S]], i8 2, i8 3 }
 // CHECK-UBSAN: @[[LINE_800:.*]] = {{.*}}, i32 800, i32 12 {{.*}} @{{.*}} }
 // CHECK-UBSAN: @[[LINE_900:.*]] = {{.*}}, i32 900, i32 11 {{.*}} @{{.*}} }
+// CHECK-UBSAN: @[[LINE_1000:.*]] = {{.*}}, i32 1000, i32 11 {{.*}} @{{.*}} }
 // CHECK-UBSAN: @[[FP16:.*]] = private unnamed_addr constant { i16, i16, [9 x 
i8] } { i16 1, i16 16, [9 x i8] c"'__fp16'\00" }
 // CHECK-UBSAN: @[[LINE_1200:.*]] = {{.*}}, i32 1200, i32 10 {{.*}} @{{.*}} }
 // CHECK-UBSAN: @[[LINE_1300:.*]] = {{.*}}, i32 1300, i32 10 {{.*}} @{{.*}} }
@@ -186,6 +187,16 @@ void vla_bound(int n) {
   int arr[n * 3];
 }
 
+// CHECK-UBSAN-LABEL: @vla_bound_unsigned
+void vla_bound_unsigned(unsigned int n) {
+  // CHECK-UBSAN:  icmp ugt i32 %[[PARAM:.*]], 0
+  //
+  // CHECK-UBSAN:  %[[ARG:.*]] = zext i32 %[[PARAM]] to i64
+  // CHECK-UBSAN-NEXT: call void @__ub

[clang] [compiler-rt] [ubsan] Display correct runtime messages for negative _BitInt (PR #96240)

2024-08-19 Thread Adam Magier via cfe-commits 


@@ -3324,6 +3359,20 @@ llvm::Constant 
*CodeGenFunction::EmitCheckTypeDescriptor(QualType T) {
   DiagnosticsEngine::ak_qualtype, (intptr_t)T.getAsOpaquePtr(), 
StringRef(),
   StringRef(), std::nullopt, Buffer, std::nullopt);
 
+  if (IsBitInt) {
+// The Structure is: 0 to end the string, 32 bit unsigned integer in target
+// endianness, zero.
+char S[6] = {'\0', '\0', '\0', '\0', '\0', '\0'};

AdamMagierFOSS wrote:

Probably a bit too late to the party and I struggle with following discussion 
history on GitHub, but what was the reasoning behind storing the bit width 
encoded in the name of the type versus parsing out the bit width from the name 
of the type itself in the runtime (e.g. parsing out the value 37 from 
'_BitInt(37)' instead of extracting the literal 37 from 
'_BitInt(37)\x00\x25\x00\x00\x00' )? I would imagine that the former would be 
easier since there wouldn't be any platform dependence when traversing the 
string, and I don't think it would incur too much of an extra performance hit 
(the solution might look a bit kludgey though)

https://github.com/llvm/llvm-project/pull/96240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits