[PATCH] D48959: [compiler-rt][ubsan] Implicit Cast Sanitizer - integer truncation - compiler-rt part

Mon Jul 9 07:33:26 PDT 2018

lebedev.ri added inline comments.

================
Comment at: lib/ubsan/ubsan_checks.inc:33
 UBSAN_CHECK(InvalidBuiltin, "invalid-builtin-use", "invalid-builtin-use")
+UBSAN_CHECK(ImplicitCast, "implicit-cast", "implicit-cast")
+UBSAN_CHECK(ImplicitIntegerTruncation, "implicit-integer-truncation",
----------------
If there is no this grouping, then i would need to default to `GenericUB`,
and that is not really correct, since this is not really UB.
What harm is in having this check-that-is-just-a-group in here?

================
Comment at: lib/ubsan/ubsan_handlers.cc:454
+  SourceLocation Loc = Data->Loc.acquire();
+  ErrorType ET = ErrorType::ImplicitCast;
+
----------------
filcab wrote:
> lebedev.ri wrote:
> > filcab wrote:
> > > lebedev.ri wrote:
> > > > filcab wrote:
> > > > > There's only one enumerator in `ImplicitCastCheckKind`, why do we have these two `ErrorType` enumerators? It seems we only have one error we can diagnose. What is meant by `ImplicitCast` and `ImplicitIntegerTruncation`?
> > > > `ImplicitCast` is, well, implicit cast.
> > > > Implicit cast can do many things.
> > > > It can truncate to the smaller bit width (what we are currently testing)
> > > > It can be a NOP cast that turns large number into a negative number.
> > > > It can cast between floating points/integers, with loss of precision.
> > > > 
> > > > In other words, there are some more things it will potentially check.
> > > But won't that just be like the `undefined`, `shift`, or `nullability` groups? On the clang side you're not adding any other types of checks either, just their grouping. I think we should do like in the shift group: Have just the one `implicit-integer-truncation` on the compiler-rt side, and have the clang side deal with the groupings.
> > I'm not sure i follow.
> > 
> > To add `-fsanitize=implicit-sign-change`, on clang side, i will need a new, different logic to detect it.
> > I think, i won't need to have a new `__ubsan_handle_` for each of those groups.
> > One would also like to to be able to control these groups separately,
> > at compile-time, and run-time blacklist time, i expect.
> > 
> > So i don't see why only one side should know about the grouping.
> How is this different from the `shift` grouping? There is no concept of `shift` grouping in compiler-rt, just on clang. Clang will emit its handler calls however it wants, and on the compiler-rt side you just need knowledge about the checks themselves, not the grouping. Creation of ubsan handlers doesn't need to correspond to enumerators on ubsan check kinds. type_mismatch is the handler used for many, many unrelated checks, for example.
> 
> I've annotated the `shift-*` examples in the code above. Your ubsan_checks.inc additions should only have the actual check you have today, not the grouping.
I'm not convinced, and i don't see any logic in that. I'm going to keep this as-is.

================
Comment at: lib/ubsan/ubsan_handlers.cc:460
+    break;
+  }
+
----------------
filcab wrote:
> This code should either be something like this:
> ```
> // TODO(your_username): Add implicit sign change and similar checks
> ErrorType ET = ErrorType::ImplicitIntegerTruncation;
> ```
> 
> or, if you want to setup some boilerplate right now (I'd rather avoid it until we have a second check):
> ```
> ErrorType ET;
> switch (Data->Kind) {
>   default:
>     CHECK(0 && "invalid implicit cast check kind");
>   case ICCK_IntegerTruncation:
>     ET = ErrorType::ImplicitIntegerTruncation;
>     break;
>   }
> ```
So when a mismatch between the clang and compiler-rt happens (or bad IR),
we will either crash, or report obviously-wrong check name,
instead of defaulting to sane 'implicit cast'.

I'm sorry, but is this really the best course of action?

================
Comment at: lib/ubsan/ubsan_handlers.h:139
+/// \brief Implict cast that changed the value.
+RECOVERABLE(implicit_cast, ImplicitCastData *Data, ValueHandle Src,
+            ValueHandle Dst)
----------------
eugenis wrote:
> Please add a handler to ubsan_minimal, too.
Yep, i was planning on doing that, just didn't go it in the first go.

================
Comment at: test/ubsan/TestCases/ImplicitCast/integer-truncation.c:1
+// RUN: %clang -Wno-constant-conversion -fsanitize=implicit-integer-truncation %s -o %t && %run %t 2>&1 | FileCheck %s --check-prefixes=CHECK
+
----------------
filcab wrote:
> lebedev.ri wrote:
> > filcab wrote:
> > > No need to have the warning flag.
> > > I'd rather have the commands split since we only really need `FileCheck` to run on the output of `%run`, not the compiler output.
> > It's kinda noisy without the warning..
> If we don't capture the output, we won't care ;-)
> Other ubsan tests have the same issues. But this is a minor nit. ok to keep if you prefer.
I have dropped the warning flag.

But i really don't see why we would want to split this into two run lines.
Don't we want the compile to succeed, before running the supposedly-built executable?

================
Comment at: test/ubsan/TestCases/ImplicitCast/integer-truncation.c:17
+  // Explicit and-ing of bits will silence it.
+  uint8_t nc0 = ((~((uint32_t)(0))) & 255);
+
----------------
filcab wrote:
> Nit: Here I think `... = (~(uint32_t)0) & 255` is unambiguous (Added parens around the LHS so no one needs to remember that casts bind more tightly) and doesn't require parsing as many parenthesis.
Dropped some parenthesis, but not all.

================
Comment at: test/ubsan/TestCases/ImplicitCast/integer-truncation.c:34
+// CHECK: {{.*}}integer-truncation.c:[[@LINE-1]]:15: runtime error: implicit cast from type 'int' of value 128 (32-bit, signed) to type 'int8_t' (aka 'signed char') changed the value to -128 (8-bit, signed)
+
+  return 0;
----------------
filcab wrote:
> Please add a few from 64 bit to 32 bit (128 bit to 64 bit might be a bit too non-portable). That would show that it also works when there's no (on most/all of the current platforms we support) integer promotions involved.
I have added tests, but i'm wary of "it also works when there's no <..> integer promotions involved." part.
I suspect this will cause test failures and reverts.

Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D48959