[PATCH] D86000: Add an unsigned shift base sanitizer

Fri Aug 14 16:05:41 PDT 2020

jfb added a comment.

In D86000#2219288 <https://reviews.llvm.org/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-added implicit conversion checks were folded in without much/any pushback.

`integer` does "not actually UB checks", right? I can certainly put it in there if you think I won't get yelled at 😄

================
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-NORECOVER-NOT: "-fsanitize-recover=unsigned-shift-base"
----------------
vsk wrote:
> Not sure I follow this one. Why is 'NORECOVER' not expecting to see "-fno-sanitize-recover=unsigned-shift-base"?
I have no idea! Other parts of this file do this:
```
// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}} // ???
// CHECK-implicit-integer-arithmetic-value-change-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-signed-integer-truncation|implicit-integer-sign-change),?){2}"}} // ???
// CHECK-implicit-integer-truncation-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}} // ???
```
I was hoping someone who's touched this before would know.

================
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
+
----------------
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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86000/new/

https://reviews.llvm.org/D86000