[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)
This revision was automatically updated to reflect the committed changes.
Closed by commit rL296213: [ubsan] Omit superflous overflow checks for promoted
arithmetic (PR20193) (authored by vedantk).
Changed prior to commit:
https://reviews.llvm.org/D29369?vs=89733=89752#toc
Repository:
rL LLVM
https://reviews.llvm.org/D29369
Files:
cfe/trunk/lib/CodeGen/CGExprScalar.cpp
cfe/trunk/test/CodeGen/compound-assign-overflow.c
cfe/trunk/test/CodeGen/ubsan-promoted-arith.cpp
cfe/trunk/test/CodeGen/unsigned-promotion.c
Index: cfe/trunk/test/CodeGen/ubsan-promoted-arith.cpp
===
--- cfe/trunk/test/CodeGen/ubsan-promoted-arith.cpp
+++ cfe/trunk/test/CodeGen/ubsan-promoted-arith.cpp
@@ -0,0 +1,124 @@
+// 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
+};
+
+// CHECK-LABEL: define signext i8 @_Z4add1
+// CHECK-NOT: sadd.with.overflow
+char add1(char c) { return c + c; }
+
+// CHECK-LABEL: define zeroext i8 @_Z4add2
+// CHECK-NOT: uadd.with.overflow
+uchar add2(uchar uc) { return uc + uc; }
+
+// CHECK-LABEL: define i32 @_Z4add3
+// CHECK: sadd.with.overflow
+int add3(E1 e) { return e + a; }
+
+// CHECK-LABEL: define signext i8 @_Z4add4
+// CHECK-NOT: sadd.with.overflow
+char add4(E2 e) { return e + b; }
+
+// CHECK-LABEL: define signext i8 @_Z4sub1
+// CHECK-NOT: ssub.with.overflow
+char sub1(char c) { return c - c; }
+
+// CHECK-LABEL: define zeroext i8 @_Z4sub2
+// CHECK-NOT: usub.with.overflow
+uchar sub2(uchar uc) { return uc - uc; }
+
+// CHECK-LABEL: define signext i8 @_Z4sub3
+// CHECK-NOT: ssub.with.overflow
+char sub3(char c) { return -c; }
+
+// Note: -INT_MIN can overflow.
+//
+// CHECK-LABEL: define i32 @_Z4sub4
+// CHECK: ssub.with.overflow
+int sub4(int i) { return -i; }
+
+// CHECK-LABEL: define signext i8 @_Z4mul1
+// CHECK-NOT: smul.with.overflow
+char mul1(char c) { return c * c; }
+
+// CHECK-LABEL: define zeroext i8 @_Z4mul2
+// CHECK-NOT: smul.with.overflow
+uchar mul2(uchar uc) { return uc * uc; }
+
+// Note: USHRT_MAX * USHRT_MAX can overflow.
+//
+// CHECK-LABEL: define zeroext i16 @_Z4mul3
+// CHECK: smul.with.overflow
+ushort mul3(ushort us) { return us * us; }
+
+// CHECK-LABEL: define i32 @_Z4mul4
+// CHECK: smul.with.overflow
+int mul4(int i, char c) { return i * c; }
+
+// CHECK-LABEL: define i32 @_Z4mul5
+// CHECK: smul.with.overflow
+int mul5(int i, char c) { return c * i; }
+
+// CHECK-LABEL: define signext i16 @_Z4mul6
+// CHECK-NOT: smul.with.overflow
+short mul6(short s) { return s * s; }
+
+// CHECK-LABEL: define signext i8 @_Z4div1
+// CHECK-NOT: ubsan_handle_divrem_overflow
+char div1(char c) { return c / c; }
+
+// CHECK-LABEL: define zeroext i8 @_Z4div2
+// CHECK-NOT: ubsan_handle_divrem_overflow
+uchar div2(uchar uc) { return uc / uc; }
+
+// CHECK-LABEL: define signext i8 @_Z4div3
+// CHECK-NOT: ubsan_handle_divrem_overflow
+char div3(char c, int i) { return c / i; }
+
+// CHECK-LABEL: define signext i8 @_Z4div4
+// CHECK: ubsan_handle_divrem_overflow
+char div4(int i, char c) { return i / c; }
+
+// Note: INT_MIN / -1 can overflow.
+//
+// CHECK-LABEL: define signext i8 @_Z4div5
+// CHECK: ubsan_handle_divrem_overflow
+char div5(int i, char c) { return i / c; }
+
+// CHECK-LABEL: define signext i8 @_Z4rem1
+// CHECK-NOT: ubsan_handle_divrem_overflow
+char rem1(char c) { return c % c; }
+
+// CHECK-LABEL: define zeroext i8 @_Z4rem2
+// CHECK-NOT: ubsan_handle_divrem_overflow
+uchar rem2(uchar uc) { return uc % uc; }
+
+// FIXME: This is a long-standing false negative.
+//
+// CHECK-LABEL: define signext i8 @_Z4rem3
+// rdar30301609: ubsan_handle_divrem_overflow
+char rem3(int i, char c) { return i % c; }
+
+// CHECK-LABEL: define signext i8 @_Z4inc1
+// CHECK-NOT: sadd.with.overflow
+char inc1(char c) { return c++ + (char)0; }
+
+// CHECK-LABEL: define zeroext i8 @_Z4inc2
+// CHECK-NOT: uadd.with.overflow
+uchar inc2(uchar uc) { return uc++ + (uchar)0; }
+
+// CHECK-LABEL: define void @_Z4inc3
+// CHECK-NOT: sadd.with.overflow
+void inc3(char c) { c++; }
+
+// CHECK-LABEL: define void @_Z4inc4
+// CHECK-NOT: uadd.with.overflow
+void inc4(uchar uc) { uc++; }
Index: cfe/trunk/test/CodeGen/unsigned-promotion.c
===
--- cfe/trunk/test/CodeGen/unsigned-promotion.c
+++ cfe/trunk/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:
[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)
dtzWill accepted this revision. dtzWill added a comment. This revision is now accepted and ready to land. Sorry for the delay! LGTM, thanks! 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)
vsk updated this revision to Diff 89733.
vsk added a comment.
- Make the suggested readability improvements, and fix a comment in the test
case.
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:%.*]] = load i8,
[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)
vsk added a comment. Ping, is the argument in favor of making the change in my last comment satisfactory? 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)
vsk added a comment. In https://reviews.llvm.org/D29369#676521, @dtzWill wrote: > In https://reviews.llvm.org/D29369#673064, @vsk wrote: > > > In https://reviews.llvm.org/D29369#672166, @dtzWill wrote: > > > > > After some thought, can we discuss why this is a good idea? > > > > > > The goal is to lower ubsan's compile-time + instrumentation overhead at > > -O0, since this reduces the friction of debugging a ubsan-instrumented > > project. > > > Apologies for the delay, thank you for the explanation. Np, thanks for taking a look! >>> This increases the cyclomatic complexity of code that already is difficult >>> to reason about, and seems like it's both brittle and out-of-place in >>> CGExprScalar. >> >> Are there cleanups or ways to reorganize the code that would make this sort >> of change less complex / brittle? I'm open to taking that on. > > None that I see immediately (heh, otherwise I'd be working on them myself...) > but the code paths for trapping/non-trapping are particularly what I meant > re:complexity, and while I suppose the AST or its interface is probably > unlikely to change much (?) I'm concerned about these checks silently > removing checks they shouldn't in the future. > > (Who would notice if this happened?) I don't have a good answer for this. I've tried to make sure we don't introduce any false negatives with this patch by covering all the cases I can think of, but it's possible we could have missed something. There are enough people using this feature that I think we'd be alerted to + fix false negatives. >>> It really seems it would be better to let InstCombine or some other >>> analysis/transform deal with proving checks redundant instead of attempting >>> to do so on-the-fly during CodeGen. >> >> -O1/-O2 do get rid of a lot of checks, but they also degrade the debugging >> experience, so it's not really a solution for this use case. > > Understood, that makes sense. > > Would running InstCombine (and only InstCombine): > > 1. Fail to remove any checks elided by this change? No, instcombine gets all of these. > 2. Have a negative impact on debugging experience? For this I'm mostly asking > for a guess, I don't know how to exactly quantify this easily. Probably, but I'm not 100% sure. Instcombine can touch a fair amount of debug info. > (3) Have an undesirable impact on compilation time or other negative > consequence?) Instcombine is one of the slower llvm passes, IIRC. At any rate, the idea of modifying the -O0 pipeline when ubsan is enabled just to turn on instcombine doesn't seem palatable.. >>> Can you better motivate why this is worth these costs, or explain your use >>> case a bit more? >> >> I have some numbers from LNT. I did a pre-patch and post-patch run at -O0 + >> -fsanitize=signed-integer-overflow,unsigned-integer-overflow. There were >> 4,672 object files produced in each run. This patch brings the average >> object size down from 36,472.0 to 36,378.3 bytes (a 0.26% improvement), and >> the average number of overflow checks per object down from 66.8 to 66.2 (a >> 0.81% improvement). > > Wonderful, thank you for producing and sharing these numbers. Those > improvements don't convince me, but if you're saying this is important to you > and your use-case/users I'm happy to go with that. Yeah, on average, the patch isn't a huge improvement. What makes it worthwhile (imo) is that the risk is also very low, and that it can pay to emit less IR (for the one person out there that wants to add a million shorts together). Some context: we have a project that adds ~17,000 integers together in straight-line code (someone must have auto-generated the C code that does this ><...). The amount of add/sub overflow checks generated there brings clang to its knees at -O0, -Os, etc. We had to kill the build of this project. I get that it's not a representative example, but this is the kind of behavior I really don't want unsuspecting users to hit. >> I don't have reliable compile-time numbers, but not emitting IR really seems >> like a straightforward improvement over emitting/analyzing/removing it. > > Hard to say. Separation of concerns is important too, but of course there's > trade-offs everywhere :). I'd suspect this doesn't change compile-time much > either way. > >> So, those are the benefits. IMO getting close to 1% better at reducing >> instrumentation overhead is worth the complexity cost here. > > Couldn't say, but that sounds reasonable to me and I don't mean to stand in > the way of progress! > > Can you answer my questions about InstCombine above? Thanks! 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)
dtzWill added a comment. In https://reviews.llvm.org/D29369#673064, @vsk wrote: > In https://reviews.llvm.org/D29369#672166, @dtzWill wrote: > > > After some thought, can we discuss why this is a good idea? > > > The goal is to lower ubsan's compile-time + instrumentation overhead at -O0, > since this reduces the friction of debugging a ubsan-instrumented project. Apologies for the delay, thank you for the explanation. > > >> This increases the cyclomatic complexity of code that already is difficult >> to reason about, and seems like it's both brittle and out-of-place in >> CGExprScalar. > > Are there cleanups or ways to reorganize the code that would make this sort > of change less complex / brittle? I'm open to taking that on. None that I see immediately (heh, otherwise I'd be working on them myself...) but the code paths for trapping/non-trapping are particularly what I meant re:complexity, and while I suppose the AST or its interface is probably unlikely to change much (?) I'm concerned about these checks silently removing checks they shouldn't in the future. (Who would notice if this happened?) > > >> It really seems it would be better to let InstCombine or some other >> analysis/transform deal with proving checks redundant instead of attempting >> to do so on-the-fly during CodeGen. > > -O1/-O2 do get rid of a lot of checks, but they also degrade the debugging > experience, so it's not really a solution for this use case. Understood, that makes sense. Would running InstCombine (and only InstCombine): 1. Fail to remove any checks elided by this change? 2. Have a negative impact on debugging experience? For this I'm mostly asking for a guess, I don't know how to exactly quantify this easily. (3) Have an undesirable impact on compilation time or other negative consequence?) > > >> Can you better motivate why this is worth these costs, or explain your use >> case a bit more? > > I have some numbers from LNT. I did a pre-patch and post-patch run at -O0 + > -fsanitize=signed-integer-overflow,unsigned-integer-overflow. There were > 4,672 object files produced in each run. This patch brings the average object > size down from 36,472.0 to 36,378.3 bytes (a 0.26% improvement), and the > average number of overflow checks per object down from 66.8 to 66.2 (a 0.81% > improvement). Wonderful, thank you for producing and sharing these numbers. Those improvements don't convince me, but if you're saying this is important to you and your use-case/users I'm happy to go with that. > I don't have reliable compile-time numbers, but not emitting IR really seems > like a straightforward improvement over emitting/analyzing/removing it. Hard to say. Separation of concerns is important too, but of course there's trade-offs everywhere :). I'd suspect this doesn't change compile-time much either way. > So, those are the benefits. IMO getting close to 1% better at reducing > instrumentation overhead is worth the complexity cost here. Couldn't say, but that sounds reasonable to me and I don't mean to stand in the way of progress! Can you answer my questions about InstCombine above? Thanks! 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)
vsk added a comment. In https://reviews.llvm.org/D29369#672166, @dtzWill wrote: > After some thought, can we discuss why this is a good idea? The goal is to lower ubsan's compile-time + instrumentation overhead at -O0, since this reduces the friction of debugging a ubsan-instrumented project. > This increases the cyclomatic complexity of code that already is difficult to > reason about, and seems like it's both brittle and out-of-place in > CGExprScalar. Are there cleanups or ways to reorganize the code that would make this sort of change less complex / brittle? I'm open to taking that on. > It really seems it would be better to let InstCombine or some other > analysis/transform deal with proving checks redundant instead of attempting > to do so on-the-fly during CodeGen. -O1/-O2 do get rid of a lot of checks, but they also degrade the debugging experience, so it's not really a solution for this use case. > Can you better motivate why this is worth these costs, or explain your use > case a bit more? I have some numbers from LNT. I did a pre-patch and post-patch run at -O0 + -fsanitize=signed-integer-overflow,unsigned-integer-overflow. There were 4,672 object files produced in each run. This patch brings the average object size down from 36,472.0 to 36,378.3 bytes (a 0.26% improvement), and the average number of overflow checks per object down from 66.8 to 66.2 (a 0.81% improvement). I don't have reliable compile-time numbers, but not emitting IR really seems like a straightforward improvement over emitting/analyzing/removing it. So, those are the benefits. IMO getting close to 1% better at reducing instrumentation overhead is worth the complexity cost here. 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)
dtzWill requested changes to this revision. dtzWill added a comment. This revision now requires changes to proceed. After some thought, can we discuss why this is a good idea? This increases the cyclomatic complexity of code that already is difficult to reason about, and seems like it's both brittle and out-of-place in CGExprScalar. It really seems it would be better to let InstCombine or some other analysis/transform deal with proving checks redundant instead of attempting to do so on-the-fly during CodeGen. Can you better motivate why this is worth theses costs, or explain your use case a bit more? 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)
dtzWill accepted this revision. dtzWill added a comment. I've been bitten when attempting to use existence/nature of casts in the AST to reason about the original code, but this looks like it does the right thing in all the situations I can think of. Missing overflows because of a bugged attempt to optimize the -O0 case would be unfortunate-- has this been tested and compared on larger codes (test-suite, other projects)? When it comes to C/C++ standards and constructs, it seems there's always some extension/language feature/flag that someone (ab)uses that you forgot all about when reasoning about these things abstractly... so it'd be good to see this checked out before the next major release. +1 to suggestion for a more readable name or wrapper for the common pattern of 'getUnwidenedIntegerType..hasValue()', if you get a chance. LGTM, thanks! 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)
regehr added a comment. Paging @dtzWill 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)
filcab added a comment. Minor nits, now. LGTM, but having someone more familiar with clang chime in would be great. Comment at: lib/CodeGen/CGExprScalar.cpp:1700 case LangOptions::SOB_Trapping: +if (getUnwidenedIntegerType(CGF.getContext(), E->getSubExpr()).hasValue()) + return Builder.CreateNSWAdd(InVal, Amount, Name); Maybe a helper `IsWidenedIntegerOp(...)` (or `IsOpWiderThanBaseType`or something) would make this (and others, like the first return of `getUnwidenedIntegerType`) easier to read? Comment at: test/CodeGen/ubsan-promoted-arith.cpp:90 + +// Note: -INT_MIN / -1 can overflow. +// Extra `-`. `INT_MIN/-1` is what you want. You already have a test above for `-INT_MIN` (which would overflow before the division. 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)
vsk updated this revision to Diff 87030.
vsk edited the summary of this revision.
vsk added a comment.
- Use switches per Filipe's comment, and fix a comment in the test case.
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:
[PATCH] D29369: [ubsan] Omit superflous overflow checks for promoted arithmetic (PR20193)
vsk marked an inline comment as done. vsk added a comment. In https://reviews.llvm.org/D29369#666308, @vsk wrote: > In https://reviews.llvm.org/D29369#665878, @filcab wrote: > > > Why the switch to `if` instead of a fully-covered switch/case? > > > It lets me avoid repeating two function calls: Ah, sorry about that, I think I understand what you had in mind now. I'll fix that too. 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)
filcab added a comment.
Why the switch to `if` instead of a fully-covered switch/case?
In https://reviews.llvm.org/D29369#664426, @vsk wrote:
> In https://reviews.llvm.org/D29369#664366, @regehr wrote:
>
> > Out of curiosity, how many of these superfluous checks are not subsequently
> > eliminated by InstCombine?
>
>
> I don't have numbers from a benchmark prepped. Here's what we get with the
> 'ubsan-promoted-arith.cpp' test case from this patch:
>
> | Setup| # of overflow checks |
> | unpatched, -O0 | 22 |
> | unpatched, -O0 + instcombine | 7|
> | patched, -O0 | 8|
> | patched, -O0 + instcombine | 7|
>
> (There's a difference between the "patched, -O0" setup and the "patched, -O0
> + instcombine" setup because llvm figures out that the symbol 'a' is 0, and
> gets rid of an addition that way.)
>
> At least for us, this patch is still worthwhile, because our use case is `-O0
> -fsanitized=undefined`. Also, this makes less work for instcombine, but I
> haven't measured the compile-time effect.
Probably running mem2reg and others before instcombine would make it elide more
checks. But if you're using -O0 anyway, I guess this would help anyway.
Comment at: test/CodeGen/ubsan-promoted-arith.cpp:56
+
+// Note: -SHRT_MIN * -SHRT_MIN can overflow.
+//
Nit: Maybe `USHRT_MAX * USHRT_MAX` is more understandable?
Comment at: test/CodeGen/ubsan-promoted-arith.cpp:99
+// CHECK-LABEL: define signext i8 @_Z4rem3
+// rdar30301609: ubsan_handle_divrem_overflow
+char rem3(int i, char c) { return i % c; }
Maybe put the rdar ID next to the FIXME? Like this it looks like you might have
written that instead of `CHECK` by mistake.
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)
vsk added a comment. In https://reviews.llvm.org/D29369#664366, @regehr wrote: > Out of curiosity, how many of these superfluous checks are not subsequently > eliminated by InstCombine? I don't have numbers from a benchmark prepped. Here's what we get with the 'ubsan-promoted-arith.cpp' test case from this patch: | Setup| # of overflow checks | | unpatched, -O0 | 22 | | unpatched, -O0 + instcombine | 7| | patched, -O0 | 8| | patched, -O0 + instcombine | 7| (There's a difference between the "patched, -O0" setup and the "patched, -O0 + instcombine" setup because llvm figures out that the symbol 'a' is 0, and gets rid of an addition that way.) At least for us, this patch is still worthwhile, because our use case is `-O0 -fsanitized=undefined`. Also, this makes less work for instcombine, but I haven't measured the compile-time effect. 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)
regehr added a comment. Out of curiosity, how many of these superfluous checks are not subsequently eliminated by InstCombine? 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)
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)
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)
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)
efriedma added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:72 + if (const auto *UO = dyn_cast(Op.E)) +return IsPromotedInteger(UO->getSubExpr()); + 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. 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)
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
+++
