[PATCH] D86000: Add an unsigned shift base sanitizer

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

In D86000#2219322 <https://reviews.llvm.org/D86000#2219322>, @jfb wrote:

> 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 😄

I'd support this.
"integer" includes unsigned-integer-overflow, it's only recommended to the truly fearless developers :)



================
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:
> jfb wrote:
> > 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.
> The CHECK-implicit-conversion-NORECOVER-NOT regex looks fishy. There's more parens in there than I'd expect: perhaps that explains why the test passes?
> 
> Just above your change, I see something that's closer to what I expect:
> 
> ```
> // RUN: %clang -fsanitize=float-divide-by-zero %s -fno-sanitize-recover=float-divide-by-zero -### 2>&1 | FileCheck %s --check-prefixes=CHECK-DIVBYZERO,CHECK-DIVBYZERO-NORECOVER
> ...
> // CHECK-DIVBYZERO-NORECOVER-NOT: "-fsanitize-recover=float-divide-by-zero"
> ```
I think that + the following line mean that the frontend depends on the default behavior (neither recover nor no-recover).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86000