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*/ true), "shl.check"); - if (CGF.getLangOpts().CPlusPlus) { + if (SanitizeUnsignedBase || CGF.getLangOpts().CPlusPlus) { // In C99, we are not permitted to shift a 1 bit into the sign bit. // Under C++11's rules, shifting a 1 bit into the sign bit is ---------------- lebedev.ri wrote: > Why is this so complicated? Shouldn't this just be: > https://alive2.llvm.org/ce/z/scTqfX > ``` > $ /repositories/alive2/build-Clang-release/alive-tv /tmp/test.ll > --smt-to=100000 --disable-undef-input > > ---------------------------------------- > @2 = global 32 bytes, align 16 > > define i32 @src(i32 %arg, i32 %arg1) { > %bb: > %i = icmp ugt i32 %arg1, 31 > %i2 = sub nsw nuw i32 31, %arg1 ; NOPE > %i3 = lshr i32 %arg, %i2 ; NOPE > %i4 = icmp ult i32 %i3, 2 ; NOPE > %i5 = or i1 %i, %i4 > br i1 %i5, label %bb9, label %bb6 > > %bb6: > %i7 = zext i32 %arg to i64 > %i8 = zext i32 %arg1 to i64 > %__constexpr_0 = bitcast * @2 to * > call void @__ubsan_handle_shift_out_of_bounds(* %__constexpr_0, i64 %i7, > i64 %i8) > br label %bb9 > > %bb9: > %i10 = shl i32 %arg, %arg1 > ret i32 %i10 > } > => > @2 = global 32 bytes, align 16 > > define i32 @tgt(i32 %arg, i32 %arg1) { > %bb: > %i = icmp ugt i32 %arg1, 31 > %iZZ0 = shl i32 %arg, %arg1 ; HI! > %iZZ1 = lshr i32 %iZZ0, %arg1 ; OVER HERE > %i4 = icmp eq i32 %arg, %iZZ1 ; LOOK! > %i5 = or i1 %i, %i4 > br i1 %i5, label %bb9, label %bb6 > > %bb6: > %i7 = zext i32 %arg to i64 > %i8 = zext i32 %arg1 to i64 > %__constexpr_0 = bitcast * @2 to * > call void @__ubsan_handle_shift_out_of_bounds(* %__constexpr_0, i64 %i7, > i64 %i8) > br label %bb9 > > %bb9: > ret i32 %iZZ0 > } > Transformation seems to be correct! > > ``` > which will then be migrated to use `@llvm.ushl.with.overflow` once it's there. Sure, but that's pre-existing and I'd rather not change it in this patch. ================ Comment at: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp:2 +// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s +// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s + ---------------- vsk wrote: > jfb wrote: > > I don't understand this test... when I run it with lit it fails (and it > > seems like the bots agree), but manually it works. Am I doing it wrong? > Do you need to explicitly `return 1` or something to get a non-zero exit code? That should be implicit, but I added it regardless to make sure. It's happy now 🤷♂️ ================ 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: > Does the test not work without the volatiles? It seems that LLVM sees too much without volatile, yes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86000/new/ https://reviews.llvm.org/D86000 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits