[clang][ubsan] Implicit Conversion Sanitizer - integer sign change - …

…clang part

This is the second half of Implicit Integer Conversion Sanitizer.
It completes the first half, and finally makes the sanitizer
fully functional! Only the bitfield handling is missing.

Summary:
C and C++ are interesting languages. They are statically typed, but weakly.
The implicit conversions are allowed. This is nice, allows to write code
while balancing between getting drowned in everything being convertible,
and nothing being convertible. As usual, this comes with a price:

```
void consume(unsigned int val);

void test(int val) {
  consume(val);
  // The 'val' is `signed int`, but `consume()` takes `unsigned int`.
  // If val is negative, then consume() will be operating on a large
  // unsigned value, and you may or may not have a bug.

  // But yes, sometimes this is intentional.
  // Making the conversion explicit silences the sanitizer.
  consume((unsigned int)val);
}
```

Yes, there is a `-Wsign-conversion`` diagnostic group, but first, it is kinda
noisy, since it warns on everything (unlike sanitizers, warning on an
actual issues), and second, likely there are cases where it does **not** warn.

The actual detection is pretty easy. We just need to check each of the values
whether it is negative, and equality-compare the results of those comparisons.
The unsigned value is obviously non-negative. Zero is non-negative too.
https://godbolt.org/g/w93oj2

We do not have to emit the check *always*, there are obvious situations
where we can avoid emitting it, since it would **always** get optimized-out.
But i do think the tautological IR (`icmp ult %x, 0`, which is always false)
should be emitted, and the middle-end should cleanup it.

This sanitizer is in the `-fsanitize=implicit-conversion` group,
and is a logical continuation of D48958 `-fsanitize=implicit-integer-truncation`.
As for the ordering, i'we opted to emit the check **after**
`-fsanitize=implicit-integer-truncation`. At least on these simple 16 test cases,
this results in 1 of the 12 emitted checks being optimized away,
as compared to 0 checks being optimized away if the order is reversed.

This is a clang part.
The compiler-rt part is D50251.

Finishes fixing [[ https://bugs.llvm.org/show_bug.cgi?id=21530 | PR21530 ]], [[ https://bugs.llvm.org/show_bug.cgi?id=37552 | PR37552 ]], [[ https://bugs.llvm.org/show_bug.cgi?id=35409 | PR35409 ]].
Finishes partially fixing [[ https://bugs.llvm.org/show_bug.cgi?id=9821 | PR9821 ]].
Finishes fixing google/sanitizers#940.

Only the bitfield handling is missing.

Reviewers: vsk, rsmith, rjmccall, #sanitizers, erichkeane

Reviewed By: rsmith

Subscribers: chandlerc, filcab, cfe-commits, regehr

Tags: #sanitizers, #clang

Differential Revision: https://reviews.llvm.org/D50250

llvm-svn: 345660

clang/docs/ReleaseNotes.rst

@@ -198,6 +198,29 @@ Static Analyzer
 Undefined Behavior Sanitizer (UBSan)
 ------------------------------------
 
+* The Implicit Conversion Sanitizer (``-fsanitize=implicit-conversion``) group
+  was extended. One more type of issues is caught - implicit integer sign change.
+  (``-fsanitize=implicit-integer-sign-change``).
+  This makes the Implicit Conversion Sanitizer feature-complete,
+  with only missing piece being bitfield handling.
+  While there is a ``-Wsign-conversion`` diagnostic group that catches this kind
+  of issues, it is both noisy, and does not catch **all** the cases.
+
+  .. code-block:: c++
+
+      bool consume(unsigned int val);
+
+      void test(int val) {
+        (void)consume(val); // If the value was negative, it is now large positive.
+        (void)consume((unsigned int)val); // OK, the conversion is explicit.
+      }
+
+  Like some other ``-fsanitize=integer`` checks, these issues are **not**
+  undefined behaviour. But they are not *always* intentional, and are somewhat
+  hard to track down. This group is **not** enabled by ``-fsanitize=undefined``,
+  but the ``-fsanitize=implicit-integer-sign-change`` check
+  is enabled by ``-fsanitize=integer``.
+  (as is ``-fsanitize=implicit-integer-truncation`` check)
 
 Core Analysis Improvements
 ==========================

clang/docs/UndefinedBehaviorSanitizer.rst

@@ -100,6 +100,12 @@ Available checks are:
      conversions - when either one, or both of the types are signed.
      Issues caught by these sanitizers are not undefined behavior,
      but are often unintentional.
+  -  ``-fsanitize=implicit-integer-sign-change``: Implicit conversion between
+     integer types, if that changes the sign of the value. That is, if the the
+     original value was negative and the new value is positive (or zero),
+     or the original value was positive, and the new value is negative.
+     Issues caught by this sanitizer are not undefined behavior,
+     but are often unintentional.
   -  ``-fsanitize=integer-divide-by-zero``: Integer division by zero.
   -  ``-fsanitize=nonnull-attribute``: Passing null pointer as a function
      parameter which is declared to never be null.
@@ -161,17 +167,24 @@ You can also use the following check groups:
      ``nullability-*`` group of checks.
   -  ``-fsanitize=undefined-trap``: Deprecated alias of
      ``-fsanitize=undefined``.
+  -  ``-fsanitize=implicit-integer-truncation``: Catches lossy integral
+     conversions. Enables ``implicit-signed-integer-truncation`` and
+     ``implicit-unsigned-integer-truncation``.
+  -  ``-fsanitize=implicit-integer-arithmetic-value-change``: Catches implicit
+     conversions that change the arithmetic value of the integer. Enables
+     ``implicit-signed-integer-truncation`` and ``implicit-integer-sign-change``.
+  -  ``-fsanitize=implicit-conversion``: Checks for suspicious
+     behaviour of implicit conversions. Enables
+     ``implicit-unsigned-integer-truncation``,
+     ``implicit-signed-integer-truncation`` and
+     ``implicit-integer-sign-change``.
   -  ``-fsanitize=integer``: Checks for undefined or suspicious integer
      behavior (e.g. unsigned integer overflow).
      Enables ``signed-integer-overflow``, ``unsigned-integer-overflow``,
-     ``shift``, ``integer-divide-by-zero``, and ``implicit-integer-truncation``.
-  -  ``fsanitize=implicit-integer-truncation``: Checks for implicit integral
-     conversions that result in data loss.
-     Enables ``implicit-unsigned-integer-truncation`` and
-     ``implicit-signed-integer-truncation``.
-  -  ``-fsanitize=implicit-conversion``: Checks for suspicious behaviours of
-     implicit conversions.
-     Currently, only ``-fsanitize=implicit-integer-truncation`` is implemented.
+     ``shift``, ``integer-divide-by-zero``,
+     ``implicit-unsigned-integer-truncation``,
+     ``implicit-signed-integer-truncation`` and
+     ``implicit-integer-sign-change``.
   -  ``-fsanitize=nullability``: Enables ``nullability-arg``,
      ``nullability-assign``, and ``nullability-return``. While violating
      nullability does not have undefined behavior, it is often unintentional,

clang/include/clang/Basic/Sanitizers.def

@@ -142,11 +142,25 @@ SANITIZER_GROUP("implicit-integer-truncation", ImplicitIntegerTruncation,
                 ImplicitUnsignedIntegerTruncation |
                     ImplicitSignedIntegerTruncation)
 
+SANITIZER("implicit-integer-sign-change", ImplicitIntegerSignChange)
+
+SANITIZER_GROUP("implicit-integer-arithmetic-value-change",
+                ImplicitIntegerArithmeticValueChange,
+                ImplicitIntegerSignChange | ImplicitSignedIntegerTruncation)
+
+// FIXME:
+//SANITIZER_GROUP("implicit-integer-conversion", ImplicitIntegerConversion,
+//                ImplicitIntegerArithmeticValueChange |
+//                    ImplicitUnsignedIntegerTruncation)
+//SANITIZER_GROUP("implicit-conversion", ImplicitConversion,
+//                ImplicitIntegerConversion)
+
 SANITIZER_GROUP("implicit-conversion", ImplicitConversion,
-                ImplicitIntegerTruncation)
+                ImplicitIntegerArithmeticValueChange |
+                    ImplicitUnsignedIntegerTruncation)
 
 SANITIZER_GROUP("integer", Integer,
-                ImplicitIntegerTruncation | IntegerDivideByZero | Shift |
+                ImplicitConversion | IntegerDivideByZero | Shift |
                     SignedIntegerOverflow | UnsignedIntegerOverflow)
 
 SANITIZER("local-bounds", LocalBounds)