[PATCH] D86000: Add an unsigned shift base sanitizer

This revision was automatically updated to reflect the committed changes. Closed by commit rG82d29b397bb2: Add an unsigned shift base sanitizer (authored by jfb). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D860

[PATCH] D86000: Add an unsigned shift base sanitizer

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. Lgtm, thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 __

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb added inline comments. Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6 + volatile unsigned _v = (val);\ + volatile unsigned _a = (amount); \ + unsigned res = _v << _a; \ vsk wrote: > jfb wrote: > > vsk wrote: > > > Doe

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288444. jfb marked 4 inline comments as done. jfb added a comment. Remove the "suppress this" in release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files: cla

[PATCH] D86000: Add an unsigned shift base sanitizer

lebedev.ri added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:153 + + `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs` + vsk wrote: > jfb wrote: > > lebedev.ri wrote: > > > Surely not `~1U`. > > Indeed, fixed. > I don't think this pattern wor

[PATCH] D86000: Add an unsigned shift base sanitizer

vsk added inline comments. Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:6 + volatile unsigned _v = (val);\ + volatile unsigned _a = (amount); \ + unsigned res = _v << _a; \ jfb wrote: > vsk wrote: > > Does the test not wo

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:153 + + `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs` + lebedev.ri wrote: > Surely not `~1U`. Indeed, fixed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288388. jfb marked an inline comment as done. jfb added a comment. Fix notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files: clang/docs/UndefinedBehaviorSanitizer

[PATCH] D86000: Add an unsigned shift base sanitizer

lebedev.ri added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:153 + + `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs` + Surely not `~1U`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/ne

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3872-3884 llvm::Value *BitsShiftedOff = Builder.CreateLShr( Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros", /*NUW*/ true, /*NSW*/

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288383. jfb marked 6 inline comments as done. jfb added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ ht

[PATCH] D86000: Add an unsigned shift base sanitizer

lebedev.ri added a comment. Release notes missing. I think the canonical way to silence it (via masking?) should be at least mentioned. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3872-3884 llvm::Value *BitsShiftedOff = Builder.CreateLShr( Ops.LHS, Builder

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288133. jfb added a comment. Re-upload with full context. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files: clang/docs/UndefinedBehaviorSanitizer.rst clang/include

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb updated this revision to Diff 288132. jfb added a comment. As Vedant suggested, make it part of 'integer' sanitizer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 Files: clang/include/clang/Basic/Sa

[PATCH] D86000: Add an unsigned shift base sanitizer

lebedev.ri added a comment. What's next, a sanitizer that input to signed right-shift is always non-negative? :) FWIW "fixing" these "bugs" via `(x & ~(~1U << (32-shamt))) << shamt` is going to be fine, i've already taught instcombine to drop such pointless masking before left-shift in PR42563

[PATCH] D86000: Add an unsigned shift base sanitizer

eugenis added a comment. In D86000#2219322 , @jfb wrote: > In D86000#2219288 , @vsk wrote: > >> It'd be nice to fold the new check into an existing sanitizer group to bring >> this to a wider audience. Do you fores

[PATCH] D86000: Add an unsigned shift base sanitizer

vsk added a comment. In D86000#2219322 , @jfb wrote: > In D86000#2219288 , @vsk wrote: > >> It'd be nice to fold the new check into an existing sanitizer group to bring >> this to a wider audience. Do you foresee a

[PATCH] D86000: Add an unsigned shift base sanitizer

vsk added inline comments. Comment at: clang/test/Driver/fsanitize.c:911 +// CHECK-unsigned-shift-base-RECOVER-NOT: "-fsanitize-trap=unsigned-shift-base" +// CHECK-unsigned-shift-base-NORECOVER-NOT: "-fno-sanitize-recover=unsigned-shift-base" +// CHECK-unsigned-shift-base-NORECO

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb added a comment. In D86000#2219288 , @vsk wrote: > It'd be nice to fold the new check into an existing sanitizer group to bring > this to a wider audience. Do you foresee adoption issues for existing > -fsanitize=integer adopters? Fwiw some recently-

[PATCH] D86000: Add an unsigned shift base sanitizer

vsk added a comment. It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback. =

[PATCH] D86000: Add an unsigned shift base sanitizer

jfb created this revision. jfb added a reviewer: vsk. Herald added subscribers: Sanitizers, cfe-commits, ributzka, dexonsmith, jkorous. Herald added projects: clang, Sanitizers. jfb requested review of this revision. It's not undefined behavior for an unsigned left shift to overflow (i.e. to shif