I'll just add that we (my students and I) are interested in making the UB semantics easily pluggable / switchable. They'll still (almost certainly) be in C++, but we'd like to factor these out so at least they're cleanly separated and easily swapped out.

I am also in the "be a bit conservative at first and see how things shake out" camp, but it's y'all doing the hard work here, not me :)

In D63423#1547216, @xbolva00 wrote:

The only remaining question is, as your said, whether or not to diagnose xors in macros. @regerh @jfb ?

Just wanted to say that I think I agree with the design choices here!

tweak spacing

refactor a bit-- getRangeForOperand() now returns an optional since it can fail

eliminate tabs

factor out the getRangeForOperand() code

Refresh patch, and fix a test broken by increased LVI precision.

Regarding ++ and --, I think for now it's fine to just document that these aren't instrumented.

I do not agree that ++ is performed on the original type. The C99 standard (6.5.3.1.2) appears to be very clear on this point: "The expression ++E is equivalent to (E+=1)."

This patch doesn't appear to yet fix the "x++" or "x--" cases.

I can test this and write a few test cases.

Could you please quantify the effect that this has on compile time? Time for an optimized -j1 compile of LLVM itself is reasonable. Also it would be nice to know what effect this has on generated code.

Well, my second program should subtract a multiple of sizeof(T). But anyway, my view is that this is a real overflow and a nasty consequence of the unsigned size_t and the usual arithmetic conversions and I don't think we want to try to poke a hole in UBSan to allow this idiom unless it turns out to be extremely common.

Sorry, let's go with this example instead, which makes it clear that the program is attempting to do something completely sensible:

I'm taking a look. For reference here's the test program I'm using.

I'm afraid I don't have a better name for this.

In D20322#750381, @vsk wrote:

Hi @dtzWill, I have some time to work on this, and would be happy to implement the changes suggested by @rsmith. Let me know if it's all right for me to take a stab at this.

Paging @dtzWill

Does this check need to be sensitive to the dialect of C/C++ that the user asked for? I know that it used to be the case that the standard could be read either way for this case, but as you observe it is now unambiguously UB.

Out of curiosity, how many of these superfluous checks are not subsequently eliminated by InstCombine?

I had some deadlines to get through, but now I'll get back to updating this patch, hopefully in a few days!

I've looked at the code and I agree that the implementation is sound and is precise for non-wrapped inputs.

Also see section 3.5 of this paper (by the authors of the code above):

Great to see some work going on here!

rebase and bump

Fix borked test. Thanks folks!

I should add a note about testing:

updated patch from Will:

Hi guys, I don't know the best place for this-- just wanted to implement it since Rich Felker, author of Musl libc, was complaining that the transformation is missing. If this isn't the right place I'm happy to close this out.

fix misspelling

revise wording as suggested by Sanjoy

Oops-- instead of

I have a small, well-defined program that gets compiled into a crash by clang -O2 -fsanitize=signed-integer-overflow -fsanitize-trap=signed-integer-overflow. So either there's a bug in this patch or else teaching LLVM to see through overflow intrinsics is exposing a bug elsewhere. I'll narrow it down.

A few more reps of the build completed, and it's more like a 6 second slowdown on an 1.5-hour non-parallel build.

Address Philip's feedback and rebase. Thanks for the comments!

Looks like this patch makes a -j1 build of LLVM + Clang about 4 seconds slower on a Haswell-E. The total build takes about 87 minutes. The effect is robust across repetitions. I can give more details if anyone wants.

Do we have a standard benchmark for measuring effect on compile time? If I was going to be somewhat careful about it I'd do a single-threaded build of LLVM using itself, then repeat enough times to get statistical significance. Is that good enough? Overkill? Is there something other than LLVM that I should be building? I have SPEC 2006 handy but an not sure it's widely available.

rebase

Rebase.

Mostly subsumed by changes implemented by Philip; I'll introduce the remaining changes (taking advantage of LVI information on both sides of a binop) in a different revision.

LGTM

Hi Philip, since this is a relatively straightforward change (or should be) and you're knee deep in the code anyway, I would certainly not be offended if you pushed whatever parts of this into the code base yourself in whatever order makes the most sense.

Added tests.

I agree that tests would be good, thanks for confirming that I hadn't simply failed to spot the infrastructure I should be using to do that...

Addresses Philip's comments. Thanks!

Formatting fixes.

Looks great.