[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

[Vedant Kumar via Phabricator via cfe-commits]

vsk updated this revision to Diff 86746.
vsk edited the summary of this revision.
vsk added a comment.

- Remove a stale test case in unsigned-promotion.c.


https://reviews.llvm.org/D29369

Files:
  lib/CodeGen/CGExprScalar.cpp
  test/CodeGen/compound-assign-overflow.c
  test/CodeGen/ubsan-promoted-arith.cpp
  test/CodeGen/unsigned-promotion.c

Index: test/CodeGen/unsigned-promotion.c
===
--- test/CodeGen/unsigned-promotion.c
+++ test/CodeGen/unsigned-promotion.c
@@ -7,53 +7,6 @@
 // RUN:   -fsanitize=unsigned-integer-overflow | FileCheck %s --check-prefix=CHECKU
 
 unsigned short si, sj, sk;
-unsigned char ci, cj, ck;
-
-extern void opaqueshort(unsigned short);
-extern void opaquechar(unsigned char);
-
-// CHECKS-LABEL:   define void @testshortadd()
-// CHECKU-LABEL: define void @testshortadd()
-void testshortadd() {
-  // CHECKS:load i16, i16* @sj
-  // CHECKS:load i16, i16* @sk
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_add_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i16, i16* @sj
-  // CHECKU:  [[T2:%.*]] = zext i16 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i16, i16* @sk
-  // CHECKU:  [[T4:%.*]] = zext i16 [[T3]]
-  // CHECKU-NOT:  llvm.sadd
-  // CHECKU-NOT:  llvm.uadd
-  // CHECKU:  [[T5:%.*]] = add nsw i32 [[T2]], [[T4]]
-
-  si = sj + sk;
-}
-
-// CHECKS-LABEL:   define void @testshortsub()
-// CHECKU-LABEL: define void @testshortsub()
-void testshortsub() {
-
-  // CHECKS:load i16, i16* @sj
-  // CHECKS:load i16, i16* @sk
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_sub_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i16, i16* @sj
-  // CHECKU:  [[T2:%.*]] = zext i16 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i16, i16* @sk
-  // CHECKU:  [[T4:%.*]] = zext i16 [[T3]]
-  // CHECKU-NOT:  llvm.ssub
-  // CHECKU-NOT:  llvm.usub
-  // CHECKU:  [[T5:%.*]] = sub nsw i32 [[T2]], [[T4]]
-
-  si = sj - sk;
-}
 
 // CHECKS-LABEL:   define void @testshortmul()
 // CHECKU-LABEL: define void @testshortmul()
@@ -75,69 +28,3 @@
   // CHECKU:  [[T5:%.*]] = mul nsw i32 [[T2]], [[T4]]
   si = sj * sk;
 }
-
-// CHECKS-LABEL:   define void @testcharadd()
-// CHECKU-LABEL: define void @testcharadd()
-void testcharadd() {
-
-  // CHECKS:load i8, i8* @cj
-  // CHECKS:load i8, i8* @ck
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_add_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i8, i8* @cj
-  // CHECKU:  [[T2:%.*]] = zext i8 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i8, i8* @ck
-  // CHECKU:  [[T4:%.*]] = zext i8 [[T3]]
-  // CHECKU-NOT:  llvm.sadd
-  // CHECKU-NOT:  llvm.uadd
-  // CHECKU:  [[T5:%.*]] = add nsw i32 [[T2]], [[T4]]
-
-  ci = cj + ck;
-}
-
-// CHECKS-LABEL:   define void @testcharsub()
-// CHECKU-LABEL: define void @testcharsub()
-void testcharsub() {
-
-  // CHECKS:load i8, i8* @cj
-  // CHECKS:load i8, i8* @ck
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_sub_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i8, i8* @cj
-  // CHECKU:  [[T2:%.*]] = zext i8 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i8, i8* @ck
-  // CHECKU:  [[T4:%.*]] = zext i8 [[T3]]
-  // CHECKU-NOT:  llvm.ssub
-  // CHECKU-NOT:  llvm.usub
-  // CHECKU:  [[T5:%.*]] = sub nsw i32 [[T2]], [[T4]]
-
-  ci = cj - ck;
-}
-
-// CHECKS-LABEL:   define void @testcharmul()
-// CHECKU-LABEL: define void @testcharmul()
-void testcharmul() {
-
-  // CHECKS:load i8, i8* @cj
-  // CHECKS:load i8, i8* @ck
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.smul.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_mul_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i8, i8* @cj
-  // CHECKU:  [[T2:%.*]] = zext i8 [[T1]]
-  // CHECKU:  [[T3:%.*]] = 

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

[Vedant Kumar via Phabricator via cfe-commits]

vsk updated this revision to Diff 86739.
vsk marked an inline comment as done.
vsk added a comment.

- Per Eli's comment: check that integers are actually widened, instead of 
incorrectly assuming they are always widened. I added some test cases for this.
- Address the 'fixme' regarding multiplication with unsigned operands.


https://reviews.llvm.org/D29369

Files:
  lib/CodeGen/CGExprScalar.cpp
  test/CodeGen/compound-assign-overflow.c
  test/CodeGen/ubsan-promoted-arith.cpp
  test/CodeGen/unsigned-promotion.c

Index: test/CodeGen/unsigned-promotion.c
===
--- test/CodeGen/unsigned-promotion.c
+++ test/CodeGen/unsigned-promotion.c
@@ -9,52 +9,6 @@
 unsigned short si, sj, sk;
 unsigned char ci, cj, ck;
 
-extern void opaqueshort(unsigned short);
-extern void opaquechar(unsigned char);
-
-// CHECKS-LABEL:   define void @testshortadd()
-// CHECKU-LABEL: define void @testshortadd()
-void testshortadd() {
-  // CHECKS:load i16, i16* @sj
-  // CHECKS:load i16, i16* @sk
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_add_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i16, i16* @sj
-  // CHECKU:  [[T2:%.*]] = zext i16 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i16, i16* @sk
-  // CHECKU:  [[T4:%.*]] = zext i16 [[T3]]
-  // CHECKU-NOT:  llvm.sadd
-  // CHECKU-NOT:  llvm.uadd
-  // CHECKU:  [[T5:%.*]] = add nsw i32 [[T2]], [[T4]]
-
-  si = sj + sk;
-}
-
-// CHECKS-LABEL:   define void @testshortsub()
-// CHECKU-LABEL: define void @testshortsub()
-void testshortsub() {
-
-  // CHECKS:load i16, i16* @sj
-  // CHECKS:load i16, i16* @sk
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_sub_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i16, i16* @sj
-  // CHECKU:  [[T2:%.*]] = zext i16 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i16, i16* @sk
-  // CHECKU:  [[T4:%.*]] = zext i16 [[T3]]
-  // CHECKU-NOT:  llvm.ssub
-  // CHECKU-NOT:  llvm.usub
-  // CHECKU:  [[T5:%.*]] = sub nsw i32 [[T2]], [[T4]]
-
-  si = sj - sk;
-}
-
 // CHECKS-LABEL:   define void @testshortmul()
 // CHECKU-LABEL: define void @testshortmul()
 void testshortmul() {
@@ -76,50 +30,6 @@
   si = sj * sk;
 }
 
-// CHECKS-LABEL:   define void @testcharadd()
-// CHECKU-LABEL: define void @testcharadd()
-void testcharadd() {
-
-  // CHECKS:load i8, i8* @cj
-  // CHECKS:load i8, i8* @ck
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_add_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i8, i8* @cj
-  // CHECKU:  [[T2:%.*]] = zext i8 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i8, i8* @ck
-  // CHECKU:  [[T4:%.*]] = zext i8 [[T3]]
-  // CHECKU-NOT:  llvm.sadd
-  // CHECKU-NOT:  llvm.uadd
-  // CHECKU:  [[T5:%.*]] = add nsw i32 [[T2]], [[T4]]
-
-  ci = cj + ck;
-}
-
-// CHECKS-LABEL:   define void @testcharsub()
-// CHECKU-LABEL: define void @testcharsub()
-void testcharsub() {
-
-  // CHECKS:load i8, i8* @cj
-  // CHECKS:load i8, i8* @ck
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_sub_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i8, i8* @cj
-  // CHECKU:  [[T2:%.*]] = zext i8 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i8, i8* @ck
-  // CHECKU:  [[T4:%.*]] = zext i8 [[T3]]
-  // CHECKU-NOT:  llvm.ssub
-  // CHECKU-NOT:  llvm.usub
-  // CHECKU:  [[T5:%.*]] = sub nsw i32 [[T2]], [[T4]]
-
-  ci = cj - ck;
-}
-
 // CHECKS-LABEL:   define void @testcharmul()
 // CHECKU-LABEL: define void @testcharmul()
 void testcharmul() {
Index: test/CodeGen/ubsan-promoted-arith.cpp
===
--- /dev/null
+++ test/CodeGen/ubsan-promoted-arith.cpp
@@ -0,0 +1,116 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=signed-integer-overflow,unsigned-integer-overflow | FileCheck %s
+
+typedef unsigned char uchar;
+typedef unsigned short ushort;
+
+enum E1 : int {
+  a
+};
+
+enum E2 : char {
+  b
+};
+
+// 

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

[Vedant Kumar via Phabricator via cfe-commits]

vsk marked an inline comment as done.
vsk added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:72
+  if (const auto *UO = dyn_cast(Op.E))
+return IsPromotedInteger(UO->getSubExpr());
+

efriedma wrote:
> Checking isPromotableIntegerType doesn't work the way you want it to; types 
> can be "promoted" without actually widening them.  For example, enum types 
> are promotable, and in C++ wchar_t is promotable.
Thanks for catching this! I have fixed the issue and will update this patch 
shortly.


https://reviews.llvm.org/D29369



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

[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)

[Vedant Kumar via Phabricator via cfe-commits]

vsk created this revision.

C requires the operands of arithmetic expressions to be promoted if
their types are smaller than an int. Ubsan emits overflow checks when
this sort of type promotion occurs, even if there is no way to actually
get an overflow with the promoted type.

This patch teaches clang how to omit the superflous overflow checks
(addressing PR20193). To my knowledge, this patch only misses the case
where we have multiplications with promoted unsigned operands. E.g, in
this case, we don't need an overflow check when targeting a platform
with >=32-bit ints:

  uint8_t a, b;
  a * b;

Testing: check-clang and check-ubsan.


https://reviews.llvm.org/D29369

Files:
  lib/CodeGen/CGExprScalar.cpp
  test/CodeGen/compound-assign-overflow.c
  test/CodeGen/ubsan-promoted-arith.c
  test/CodeGen/unsigned-promotion.c

Index: test/CodeGen/unsigned-promotion.c
===
--- test/CodeGen/unsigned-promotion.c
+++ test/CodeGen/unsigned-promotion.c
@@ -9,52 +9,6 @@
 unsigned short si, sj, sk;
 unsigned char ci, cj, ck;
 
-extern void opaqueshort(unsigned short);
-extern void opaquechar(unsigned char);
-
-// CHECKS-LABEL:   define void @testshortadd()
-// CHECKU-LABEL: define void @testshortadd()
-void testshortadd() {
-  // CHECKS:load i16, i16* @sj
-  // CHECKS:load i16, i16* @sk
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_add_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i16, i16* @sj
-  // CHECKU:  [[T2:%.*]] = zext i16 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i16, i16* @sk
-  // CHECKU:  [[T4:%.*]] = zext i16 [[T3]]
-  // CHECKU-NOT:  llvm.sadd
-  // CHECKU-NOT:  llvm.uadd
-  // CHECKU:  [[T5:%.*]] = add nsw i32 [[T2]], [[T4]]
-
-  si = sj + sk;
-}
-
-// CHECKS-LABEL:   define void @testshortsub()
-// CHECKU-LABEL: define void @testshortsub()
-void testshortsub() {
-
-  // CHECKS:load i16, i16* @sj
-  // CHECKS:load i16, i16* @sk
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_sub_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i16, i16* @sj
-  // CHECKU:  [[T2:%.*]] = zext i16 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i16, i16* @sk
-  // CHECKU:  [[T4:%.*]] = zext i16 [[T3]]
-  // CHECKU-NOT:  llvm.ssub
-  // CHECKU-NOT:  llvm.usub
-  // CHECKU:  [[T5:%.*]] = sub nsw i32 [[T2]], [[T4]]
-
-  si = sj - sk;
-}
-
 // CHECKS-LABEL:   define void @testshortmul()
 // CHECKU-LABEL: define void @testshortmul()
 void testshortmul() {
@@ -76,50 +30,6 @@
   si = sj * sk;
 }
 
-// CHECKS-LABEL:   define void @testcharadd()
-// CHECKU-LABEL: define void @testcharadd()
-void testcharadd() {
-
-  // CHECKS:load i8, i8* @cj
-  // CHECKS:load i8, i8* @ck
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_add_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i8, i8* @cj
-  // CHECKU:  [[T2:%.*]] = zext i8 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i8, i8* @ck
-  // CHECKU:  [[T4:%.*]] = zext i8 [[T3]]
-  // CHECKU-NOT:  llvm.sadd
-  // CHECKU-NOT:  llvm.uadd
-  // CHECKU:  [[T5:%.*]] = add nsw i32 [[T2]], [[T4]]
-
-  ci = cj + ck;
-}
-
-// CHECKS-LABEL:   define void @testcharsub()
-// CHECKU-LABEL: define void @testcharsub()
-void testcharsub() {
-
-  // CHECKS:load i8, i8* @cj
-  // CHECKS:load i8, i8* @ck
-  // CHECKS:[[T1:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 [[T2:%.*]], i32 [[T3:%.*]])
-  // CHECKS-NEXT:   [[T4:%.*]] = extractvalue { i32, i1 } [[T1]], 0
-  // CHECKS-NEXT:   [[T5:%.*]] = extractvalue { i32, i1 } [[T1]], 1
-  // CHECKS:call void @__ubsan_handle_sub_overflow
-  //
-  // CHECKU:  [[T1:%.*]] = load i8, i8* @cj
-  // CHECKU:  [[T2:%.*]] = zext i8 [[T1]]
-  // CHECKU:  [[T3:%.*]] = load i8, i8* @ck
-  // CHECKU:  [[T4:%.*]] = zext i8 [[T3]]
-  // CHECKU-NOT:  llvm.ssub
-  // CHECKU-NOT:  llvm.usub
-  // CHECKU:  [[T5:%.*]] = sub nsw i32 [[T2]], [[T4]]
-
-  ci = cj - ck;
-}
-
 // CHECKS-LABEL:   define void @testcharmul()
 // CHECKU-LABEL: define void @testcharmul()
 void testcharmul() {
Index: test/CodeGen/ubsan-promoted-arith.c
===
--- /dev/null
+++ 

[PATCH] D29234: [ubsan] Sanity-check shift amounts before truncation (fixes PR27271)

[Vedant Kumar via Phabricator via cfe-commits]

vsk created this revision.

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.


https://reviews.llvm.org/D29234

Files:
  lib/CodeGen/CGExprScalar.cpp
  test/CodeGen/ubsan-shift.c


Index: test/CodeGen/ubsan-shift.c
===
--- /dev/null
+++ test/CodeGen/ubsan-shift.c
@@ -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);
+}
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -2751,8 +2751,8 @@
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(


Index: test/CodeGen/ubsan-shift.c
===
--- /dev/null
+++ test/CodeGen/ubsan-shift.c
@@ -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);
+}
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -2751,8 +2751,8 @@
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(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28867: [Profile] Warn about out-of-date profiles only when there are mismatches

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

Ah ok, so it sounds like a better approach would be to split the missing record 
message into a separate off-by-default warning. I don't have the time to update 
this diff this week, but will shoot for the next.


https://reviews.llvm.org/D28867



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

[PATCH] D28867: [Profile] Warn about out-of-date profiles only when there are mismatches

[Vedant Kumar via Phabricator via cfe-commits]

vsk created this revision.

Clang warns that a profile is out-of-date if it can't find a profile
record for any function in a TU. This warning is now noisy because llvm
can dead-strip functions with profiling instrumentation.

Only emit the out-of-date warning if there is an actual record mismatch.


https://reviews.llvm.org/D28867

Files:
  lib/CodeGen/CodeGenModule.cpp
  test/Profile/c-outdated-data.c


Index: test/Profile/c-outdated-data.c
===
--- test/Profile/c-outdated-data.c
+++ test/Profile/c-outdated-data.c
@@ -4,23 +4,23 @@
 // doesn't play well with warnings that have no line number.
 
 // RUN: llvm-profdata merge %S/Inputs/c-outdated-data.proftext -o %t.profdata
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name 
c-outdated-data.c %s -o /dev/null -emit-llvm 
-fprofile-instrument-use-path=%t.profdata -Wprofile-instr-dropped 2>&1 | 
FileCheck %s
-// CHECK: warning: profile data may be out of date: of 3 functions, 1 has no 
data and 1 has mismatched data that will be ignored
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name 
c-outdated-data.c %s -o /dev/null -emit-llvm 
-fprofile-instrument-use-path=%t.profdata 2>&1 | FileCheck %s 
-check-prefix=OUTDATED
+// OUTDATED: warning: profile data may be out of date: of 3 functions, 1 has 
no data and 1 has mismatched data that will be ignored
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -DGENERATE_USABLE_DATA 
-main-file-name c-outdated-data.c %s -o /dev/null -emit-llvm 
-fprofile-instrument-use-path=%t.profdata 2>&1 | FileCheck %s 
-check-prefix=USABLE -allow-empty
+// USABLE-NOT: warning: profile data may be out of date
 
 void no_usable_data() {
   int i = 0;
 
   if (i) {}
 
-#ifdef GENERATE_OUTDATED_DATA
+#ifdef GENERATE_USABLE_DATA
   if (i) {}
 #endif
 }
 
-#ifndef GENERATE_OUTDATED_DATA
 void no_data() {
 }
-#endif
 
 int main(int argc, const char *argv[]) {
   no_usable_data();
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -367,7 +367,7 @@
 if (MainFile.empty())
   MainFile = "";
 Diags.Report(diag::warn_profile_data_unprofiled) << MainFile;
-  } else
+  } else if (Mismatched > 0)
 Diags.Report(diag::warn_profile_data_out_of_date) << Visited << Missing
   << Mismatched;
 }


Index: test/Profile/c-outdated-data.c
===
--- test/Profile/c-outdated-data.c
+++ test/Profile/c-outdated-data.c
@@ -4,23 +4,23 @@
 // doesn't play well with warnings that have no line number.
 
 // RUN: llvm-profdata merge %S/Inputs/c-outdated-data.proftext -o %t.profdata
-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-outdated-data.c %s -o /dev/null -emit-llvm -fprofile-instrument-use-path=%t.profdata -Wprofile-instr-dropped 2>&1 | FileCheck %s
-// CHECK: warning: profile data may be out of date: of 3 functions, 1 has no data and 1 has mismatched data that will be ignored
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -main-file-name c-outdated-data.c %s -o /dev/null -emit-llvm -fprofile-instrument-use-path=%t.profdata 2>&1 | FileCheck %s -check-prefix=OUTDATED
+// OUTDATED: warning: profile data may be out of date: of 3 functions, 1 has no data and 1 has mismatched data that will be ignored
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.9 -DGENERATE_USABLE_DATA -main-file-name c-outdated-data.c %s -o /dev/null -emit-llvm -fprofile-instrument-use-path=%t.profdata 2>&1 | FileCheck %s -check-prefix=USABLE -allow-empty
+// USABLE-NOT: warning: profile data may be out of date
 
 void no_usable_data() {
   int i = 0;
 
   if (i) {}
 
-#ifdef GENERATE_OUTDATED_DATA
+#ifdef GENERATE_USABLE_DATA
   if (i) {}
 #endif
 }
 
-#ifndef GENERATE_OUTDATED_DATA
 void no_data() {
 }
-#endif
 
 int main(int argc, const char *argv[]) {
   no_usable_data();
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -367,7 +367,7 @@
 if (MainFile.empty())
   MainFile = "";
 Diags.Report(diag::warn_profile_data_unprofiled) << MainFile;
-  } else
+  } else if (Mismatched > 0)
 Diags.Report(diag::warn_profile_data_out_of_date) << Visited << Missing
   << Mismatched;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28131: [libcxx] Fix PR31402: map::__find_equal_key has undefined behavior.

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

LGTM. I'm not sure what to do for a test. Have you tried checking the IR for 
the affected object file in tablegen at '-O2 -fsanitize=undefined'? If there's 
an unconditional call to the ubsan handler, maybe the tablegen source could be 
used to find a small reproducer?


https://reviews.llvm.org/D28131



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

[PATCH] D27410: Always issue vtables when generating coverage instrumentation

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



Comment at: llvm/tools/clang/test/CodeGenCXX/vtable-coverage-gen.cpp:3
+// RUN: FileCheck %s < %t 
+// CHECK: @_ZTV8Category = linkonce_odr unnamed_addr constant {{.*}}, comdat,
+

vsk wrote:
> ahatanak wrote:
> > I'm not sure I understood the purpose of this test, but It looks like the 
> > vtable for Category is generated in the IR with linkonce_odr without your 
> > patch.
> Yes, I'm seeing the same thing and am also confused. I can reproduce the 
> build failure without using any vtables. To me this suggests the problem 
> could be elsewhere. Here is a minimal reproducer:
> 
> ```
> struct Base {
>   static const Base *get();
> };
> 
> struct Derived : public Base {};
> 
> const Base *Base::get() {
>   static Derived d;
>   return 
> }
> 
> int main() { return 0; }
> ```
^ Please ignore my last comment, I made a mistake while trying to compile your 
reproducer. When I used a proper compile command, I could not reproduce the 
build failure on Darwin/MachO (-fprofile-instr-generate -fcoverage-mapping).


https://reviews.llvm.org/D27410



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

[PATCH] D27410: Always issue vtables when generating coverage instrumentation

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



Comment at: llvm/tools/clang/test/CodeGenCXX/vtable-coverage-gen.cpp:3
+// RUN: FileCheck %s < %t 
+// CHECK: @_ZTV8Category = linkonce_odr unnamed_addr constant {{.*}}, comdat,
+

ahatanak wrote:
> I'm not sure I understood the purpose of this test, but It looks like the 
> vtable for Category is generated in the IR with linkonce_odr without your 
> patch.
Yes, I'm seeing the same thing and am also confused. I can reproduce the build 
failure without using any vtables. To me this suggests the problem could be 
elsewhere. Here is a minimal reproducer:

```
struct Base {
  static const Base *get();
};

struct Derived : public Base {};

const Base *Base::get() {
  static Derived d;
  return 
}

int main() { return 0; }
```


https://reviews.llvm.org/D27410



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

[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}

[Vedant Kumar via Phabricator via cfe-commits]

vsk updated this revision to Diff 80960.
vsk marked 3 inline comments as done.
vsk added a comment.

- Use NSAPI's 'is-BOOL' predicate.
- Simplify test.


https://reviews.llvm.org/D27607

Files:
  lib/CodeGen/CGExpr.cpp
  test/CodeGenObjC/ubsan-bool.m


Index: test/CodeGenObjC/ubsan-bool.m
===
--- /dev/null
+++ test/CodeGenObjC/ubsan-bool.m
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x objective-c -emit-llvm -triple 
x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s 
-check-prefixes=SHARED,OBJC
+// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple 
x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s 
-check-prefixes=SHARED,OBJC
+// RUN: %clang_cc1 -x c -emit-llvm -triple x86_64-apple-macosx10.10.0 
-fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,C
+
+typedef signed char BOOL;
+
+// SHARED-LABEL: f1
+BOOL f1() {
+  // OBJC: call void @__ubsan_handle_load_invalid_value
+  // C-NOT: call void @__ubsan_handle_load_invalid_value
+  BOOL a = 2;
+  return a + 1;
+}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -24,6 +24,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/NSAPI.h"
 #include "clang/Frontend/CodeGenOptions.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringExtras.h"
@@ -1219,11 +1220,10 @@
 
 static bool getRangeForType(CodeGenFunction , QualType Ty,
 llvm::APInt , llvm::APInt ,
-bool StrictEnums) {
+bool StrictEnums, bool IsBool) {
   const EnumType *ET = Ty->getAs();
   bool IsRegularCPlusPlusEnum = CGF.getLangOpts().CPlusPlus && StrictEnums &&
 ET && !ET->getDecl()->isFixed();
-  bool IsBool = hasBooleanRepresentation(Ty);
   if (!IsBool && !IsRegularCPlusPlusEnum)
 return false;
 
@@ -1253,8 +1253,8 @@
 
 llvm::MDNode *CodeGenFunction::getRangeForLoadFromType(QualType Ty) {
   llvm::APInt Min, End;
-  if (!getRangeForType(*this, Ty, Min, End,
-   CGM.getCodeGenOpts().StrictEnums))
+  if (!getRangeForType(*this, Ty, Min, End, CGM.getCodeGenOpts().StrictEnums,
+   hasBooleanRepresentation(Ty)))
 return nullptr;
 
   llvm::MDBuilder MDHelper(getLLVMContext());
@@ -1313,14 +1313,15 @@
   false /*ConvertTypeToTag*/);
   }
 
-  bool NeedsBoolCheck =
-  SanOpts.has(SanitizerKind::Bool) && hasBooleanRepresentation(Ty);
+  bool IsBool = hasBooleanRepresentation(Ty) ||
+NSAPI(CGM.getContext()).isObjCBOOLType(Ty);
+  bool NeedsBoolCheck = SanOpts.has(SanitizerKind::Bool) && IsBool;
   bool NeedsEnumCheck =
   SanOpts.has(SanitizerKind::Enum) && Ty->getAs();
   if (NeedsBoolCheck || NeedsEnumCheck) {
 SanitizerScope SanScope(this);
 llvm::APInt Min, End;
-if (getRangeForType(*this, Ty, Min, End, true)) {
+if (getRangeForType(*this, Ty, Min, End, /*StrictEnums=*/true, IsBool)) {
   --End;
   llvm::Value *Check;
   if (!Min)


Index: test/CodeGenObjC/ubsan-bool.m
===
--- /dev/null
+++ test/CodeGenObjC/ubsan-bool.m
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC
+// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC
+// RUN: %clang_cc1 -x c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,C
+
+typedef signed char BOOL;
+
+// SHARED-LABEL: f1
+BOOL f1() {
+  // OBJC: call void @__ubsan_handle_load_invalid_value
+  // C-NOT: call void @__ubsan_handle_load_invalid_value
+  BOOL a = 2;
+  return a + 1;
+}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -24,6 +24,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/NSAPI.h"
 #include "clang/Frontend/CodeGenOptions.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringExtras.h"
@@ -1219,11 +1220,10 @@
 
 static bool getRangeForType(CodeGenFunction , QualType Ty,
 llvm::APInt , llvm::APInt ,
-bool StrictEnums) {
+bool StrictEnums, bool IsBool) {
   const EnumType *ET = Ty->getAs();
   bool IsRegularCPlusPlusEnum = CGF.getLangOpts().CPlusPlus && StrictEnums &&
 ET && !ET->getDecl()->isFixed();
-  bool IsBool = hasBooleanRepresentation(Ty);
   if (!IsBool && !IsRegularCPlusPlusEnum)
 return false;
 
@@ 

[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}

[Vedant Kumar via Phabricator via cfe-commits]

vsk marked 3 inline comments as done.
vsk added a comment.

Thanks for your feedback. I will updated the patch shortly.




Comment at: lib/CodeGen/CGExpr.cpp:1221
+/// Check if \p Ty is defined as BOOL in a system header. In ObjC language
+/// modes, it's safe to treat such a type as 'the builtin bool'.
+static bool isObjCBool(QualType Ty, const SourceManager ,

ahatanak wrote:
> If your intention is to exclude BOOLs defined in files that aren't system 
> headers, is it possible to add a test for that?
> 
> 
> ```
> void foo() {
>   typedef long long BOOL;
>   ...
> }
> ```
Thinking about this some more, I think it's enough to check for an ObjC 
language mode, and that the system header check is unnecessary.



Comment at: lib/CodeGen/CGExpr.cpp:1222
+/// modes, it's safe to treat such a type as 'the builtin bool'.
+static bool isObjCBool(QualType Ty, const SourceManager ,
+   const LangOptions ) {

zaks.anna wrote:
> Could you use the existing method for this? From NSAPI.h:
> 
> ```
>// \brief Returns true if \param T is a typedef of "BOOL" in objective-c.
>bool isObjCBOOLType(QualType T) const;
> 
> ```
> 
Thanks, this is a lot better than rolling my own.



Comment at: lib/CodeGen/CGExpr.cpp:1224
+   const LangOptions ) {
+  if (!LO.ObjC1 && !LO.ObjC2)
+return false;

arphaman wrote:
> LangOptions.ObjC1 should be always set if LangOptions.ObjC2 is set, so the 
> second check is redundant I think.
Using NSAPI takes care of this.


https://reviews.llvm.org/D27607



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

[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}

[Vedant Kumar via Phabricator via cfe-commits]

vsk created this revision.
vsk added a reviewer: ahatanak.
vsk added subscribers: kubabrecka, cfe-commits.

On some Apple platforms, the ObjC BOOL type is defined as a signed char.
When performing instrumentation for -fsanitize=bool, we'd like to treat
the range of BOOL like it's always {0, 1}. While we can't change clang's
IRGen for char-backed BOOL's due to ABI compatibility concerns, we can
teach ubsan to catch potential abuses of this type.

rdar://problem/29502773


https://reviews.llvm.org/D27607

Files:
  lib/CodeGen/CGExpr.cpp
  test/CodeGenObjC/ubsan-bool.m


Index: test/CodeGenObjC/ubsan-bool.m
===
--- /dev/null
+++ test/CodeGenObjC/ubsan-bool.m
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -x objective-c -emit-llvm -triple 
x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s 
-check-prefixes=SHARED,OBJC
+// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple 
x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s 
-check-prefixes=SHARED,OBJC
+// RUN: %clang_cc1 -x c -emit-llvm -triple x86_64-apple-macosx10.10.0 
-fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,C
+
+#ifdef IS_SYSHEADER
+
+// Create a system definition of ObjC's BOOL.
+#pragma clang system_header
+typedef signed char BOOL;
+
+#else
+
+// Import an official-looking definition of BOOL.
+#define IS_SYSHEADER
+#include __FILE__
+
+// SHARED-LABEL: f1
+BOOL f1() {
+  // OBJC: call void @__ubsan_handle_load_invalid_value
+  // C-NOT: call void @__ubsan_handle_load_invalid_value
+  BOOL a = 2;
+  return a + 1;
+}
+
+#endif // IS_SYSHEADER
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -1217,13 +1217,29 @@
   return false;
 }
 
+/// Check if \p Ty is defined as BOOL in a system header. In ObjC language
+/// modes, it's safe to treat such a type as 'the builtin bool'.
+static bool isObjCBool(QualType Ty, const SourceManager ,
+   const LangOptions ) {
+  if (!LO.ObjC1 && !LO.ObjC2)
+return false;
+
+  const auto *TT = Ty.getTypePtr()->getAs();
+  if (!TT)
+return false;
+
+  const TypedefNameDecl *TND = TT->getDecl();
+  return TND->getName() == "BOOL" &&
+ SM.isInSystemHeader(
+ TND->getTypeSourceInfo()->getTypeLoc().getBeginLoc());
+}
+
 static bool getRangeForType(CodeGenFunction , QualType Ty,
 llvm::APInt , llvm::APInt ,
-bool StrictEnums) {
+bool StrictEnums, bool IsBool) {
   const EnumType *ET = Ty->getAs();
   bool IsRegularCPlusPlusEnum = CGF.getLangOpts().CPlusPlus && StrictEnums &&
 ET && !ET->getDecl()->isFixed();
-  bool IsBool = hasBooleanRepresentation(Ty);
   if (!IsBool && !IsRegularCPlusPlusEnum)
 return false;
 
@@ -1253,8 +1269,8 @@
 
 llvm::MDNode *CodeGenFunction::getRangeForLoadFromType(QualType Ty) {
   llvm::APInt Min, End;
-  if (!getRangeForType(*this, Ty, Min, End,
-   CGM.getCodeGenOpts().StrictEnums))
+  if (!getRangeForType(*this, Ty, Min, End, CGM.getCodeGenOpts().StrictEnums,
+   hasBooleanRepresentation(Ty)))
 return nullptr;
 
   llvm::MDBuilder MDHelper(getLLVMContext());
@@ -1313,14 +1329,16 @@
   false /*ConvertTypeToTag*/);
   }
 
-  bool NeedsBoolCheck =
-  SanOpts.has(SanitizerKind::Bool) && hasBooleanRepresentation(Ty);
+  bool IsBool =
+  hasBooleanRepresentation(Ty) ||
+  isObjCBool(Ty, CGM.getContext().getSourceManager(), CGM.getLangOpts());
+  bool NeedsBoolCheck = SanOpts.has(SanitizerKind::Bool) && IsBool;
   bool NeedsEnumCheck =
   SanOpts.has(SanitizerKind::Enum) && Ty->getAs();
   if (NeedsBoolCheck || NeedsEnumCheck) {
 SanitizerScope SanScope(this);
 llvm::APInt Min, End;
-if (getRangeForType(*this, Ty, Min, End, true)) {
+if (getRangeForType(*this, Ty, Min, End, /*StrictEnums=*/true, IsBool)) {
   --End;
   llvm::Value *Check;
   if (!Min)


Index: test/CodeGenObjC/ubsan-bool.m
===
--- /dev/null
+++ test/CodeGenObjC/ubsan-bool.m
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC
+// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC
+// RUN: %clang_cc1 -x c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,C
+
+#ifdef IS_SYSHEADER
+
+// Create a system definition of ObjC's BOOL.
+#pragma clang system_header
+typedef signed char BOOL;
+
+#else
+
+// Import an official-looking definition of BOOL.
+#define IS_SYSHEADER
+#include __FILE__
+
+// SHARED-LABEL: f1
+BOOL f1() {
+  // 

[PATCH] D27180: Testbed and skeleton of a new expression parser

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

Looks good. Apart from some nitpicks, I'm happy with this. This isn't really my 
area so it'd be good to get an explicit lgtm from one of the reviewers.




Comment at: tools/clang-import-test/CMakeLists.txt:7
+  clang-import-test.cpp
+  )
+

@beanz recently went on a crusade to add dependencies to intrinsics_gen to a 
bunch of stuff. Chances are, this tool probably depends on intrinsics_gen, so 
I'd look into that and add the dep if needed.



Comment at: tools/clang-import-test/clang-import-test.cpp:306
+  std::transform(ImportCIs.begin(), ImportCIs.end(), UnownedCIs.begin(),
+ std::mem_fn(::unique_ptr::get));
+  llvm::Expected ExpressionCI =

This transform smells a little strange. I'd rather see 
`ArrayRef>` used everywhere, through a typedef/using-decl 
if necessary.



Comment at: tools/clang-import-test/clang-import-test.cpp:310
+  if (auto E = ExpressionCI.takeError()) {
+llvm::errs() << (llvm::toString(std::move(E)));
+exit(-1);

There are redundant parens around your calls to toString(): I think these 
should be dropped.


Repository:
  rL LLVM

https://reviews.llvm.org/D27180



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

[PATCH] D27180: Testbed and skeleton of a new expression parser

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

Thanks for working on this! I'm happy with the direction of this patch. It 
makes sense that clang would have a tool to help test AST reconstruction from 
'external' sources. As you pointed out, it provides a good avenue for clients 
of the clang AST's to get better test coverage for their custom serialization 
formats. I haven't been paying much attention to the discussion about the 
clangDebuggerSupport library, but it sounds like your work will ultimately 
depend on it. Is that correct?

I've left some lower-level comments inline.

At a higher level, I'm concerned about the amount of covered-but-untested code 
this patch introduces. Since there are no CHECK lines, it's hard for me to 
verify that this tool is doing the right thing. What we really want to test 
right away is that the tool can "dump" the correct definition for `struct S` 
(since this implies that the importing went OK). A good way to go about this 
would be to add a hidden "debug" cl::opt, dump all imported struct decls when 
in -debug mode, and then add some CHECK lines for the expected struct decl.

Along with this change, I suggest stripping out a fair amount of code for the 
initial commit (probably PrintSourceForLocation, and maybe anything related to 
LogLookups).




Comment at: tools/clang-import-test/clang-import-test.cpp:238
+
+  CompilerInvocation::CreateFromArgs(*Inv, ClangArgv.data(),
+ ()[ClangArgv.size()],

spyffe wrote:
> a.sidorin wrote:
> > `ClangArgv.begin(), ClangArgv.end()`
> ```
> .../llvm/tools/clang/tools/clang-import-test/clang-import-test.cpp:236:44: 
> error: no viable conversion from 'iterator' (aka '__wrap_iter **>') to 'const char *const *'
>   CompilerInvocation::CreateFromArgs(*Inv, ClangArgv.begin(), ClangArgv.end(),
>^
> .../llvm/tools/clang/include/clang/Frontend/CompilerInvocation.h:139:49: 
> note: passing argument to parameter 'ArgBegin' here
>  const char* const *ArgBegin,
> ^
> ```
This should resolve it, but it seems less readable: `&*args.cbegin(), 
&*args.cend()`. I'd leave this as-is..



Comment at: tools/clang-import-test/clang-import-test.cpp:164
+  ExpressionCI.getASTContext(), ExpressionCI.getFileManager(),
+  ImportCI->getASTContext(), ImportCI->getFileManager(), 
MinimalImport);
+  ReverseImporters[ImportCI] = llvm::make_unique(

I think it's more idiomatic to say `/*MinimalImport=*/true` in clang.



Comment at: tools/clang-import-test/clang-import-test.cpp:282
+llvm::LLVMContext ) {
+  std::string ModuleName("$__module");
+  return std::unique_ptr(CreateLLVMCodeGen(

This might as well be constructed as a StringRef from the get-go, since it's 
eventually converted into one.



Comment at: tools/clang-import-test/clang-import-test.cpp:302
+bool Parse(const std::string , std::unique_ptr ,
+   llvm::ArrayRef Imports) {
+  CI = BuildCompilerInstance();

I suggest making this `Expected Parse(..., 
ArrayRef)`. This way, there's no way to 
mistake CI for an input param, there's no need for an extra step to convert 
std::unique_ptr to CompilerInstance *, and it's harder to 
drop an error from Parse without logging/handling it.


Repository:
  rL LLVM

https://reviews.llvm.org/D27180



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

[PATCH] D21695: [clang] Version support for UBSan handlers

[Vedant Kumar via Phabricator via cfe-commits]

vsk accepted this revision.
vsk added a reviewer: vsk.
vsk added a comment.
This revision is now accepted and ready to land.

Thanks for working on this. LGTM with a nit.




Comment at: lib/CodeGen/CGExpr.cpp:2506
+  assert(CheckHandler >= 0 &&
+ CheckHandler < sizeof(SanitizerHandlers) / 
sizeof(*SanitizerHandlers));
+  const StringRef CheckName = SanitizerHandlers[CheckHandler].Name;

Use llvm::array_lengthof? Also, I don't think the >= 0 check is really 
necessary, but I'll leave it up to you to decide.


https://reviews.llvm.org/D21695



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