[PATCH] D86000: Add an unsigned shift base sanitizer

[JF Bastien via Phabricator via llvm-commits]

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