[clang][ubsan] Implicit Conversion Sanitizer - integer truncation - clang part

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:

```
unsigned char store = 0;

bool consume(unsigned int val);

void test(unsigned long val) {
  if (consume(val)) {
    // the 'val' is `unsigned long`, but `consume()` takes `unsigned int`.
    // If their bit widths are different on this platform, the implicit
    // truncation happens. And if that `unsigned long` had a value bigger
    // than UINT_MAX, then you may or may not have a bug.

    // Similarly, integer addition happens on `int`s, so `store` will
    // be promoted to an `int`, the sum calculated (0+768=768),
    // and the result demoted to `unsigned char`, and stored to `store`.
    // In this case, the `store` will still be 0. Again, not always intended.
    store = store + 768; // before addition, 'store' was promoted to int.
  }

  // But yes, sometimes this is intentional.
  // You can either make the conversion explicit
  (void)consume((unsigned int)val);
  // or mask the value so no bits will be *implicitly* lost.
  (void)consume((~((unsigned int)0)) & val);
}
```

Yes, there is a `-Wconversion`` diagnostic group, but first, it is kinda
noisy, since it warns on everything (unlike sanitizers, warning on an
actual issues), and second, there are cases where it does **not** warn.
So a Sanitizer is needed. I don't have any motivational numbers, but i know
i had this kind of problem 10-20 times, and it was never easy to track down.

The logic to detect whether an truncation has happened is pretty simple
if you think about it - https://godbolt.org/g/NEzXbb - basically, just
extend (using the new, not original!, signedness) the 'truncated' value
back to it's original width, and equality-compare it with the original value.

The most non-trivial thing here is the logic to detect whether this
`ImplicitCastExpr` AST node is **actually** an implicit conversion, //or//
part of an explicit cast. Because the explicit casts are modeled as an outer
`ExplicitCastExpr` with some `ImplicitCastExpr`'s as **direct** children.
https://godbolt.org/g/eE1GkJ

Nowadays, we can just use the new `part_of_explicit_cast` flag, which is set
on all the implicitly-added `ImplicitCastExpr`'s of an `ExplicitCastExpr`.
So if that flag is **not** set, then it is an actual implicit conversion.

As you may have noted, this isn't just named `-fsanitize=implicit-integer-truncation`.
There are potentially some more implicit conversions to be warned about.
Namely, implicit conversions that result in sign change; implicit conversion
between different floating point types, or between fp and an integer,
when again, that conversion is lossy.

One thing i know isn't handled is bitfields.

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

Fixes [[ 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 ]].
Partially fixes [[ https://bugs.llvm.org/show_bug.cgi?id=9821 | PR9821 ]].
Fixes https://github.com/google/sanitizers/issues/940. (other than sign-changing implicit conversions)

Reviewers: rjmccall, rsmith, samsonov, pcc, vsk, eugenis, efriedma, kcc, erichkeane

Reviewed By: rsmith, vsk, erichkeane

Subscribers: erichkeane, klimek, #sanitizers, aaron.ballman, RKSimon, dtzWill, filcab, danielaustin, ygribov, dvyukov, milianw, mclow.lists, cfe-commits, regehr

Tags: #sanitizers

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

llvm-svn: 338288

2 files changed, 56 insertions, 10 deletions

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 342fed3393b..1e45a13ef91 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -46,7 +46,9 @@ sections with improvements to Clang's support for those languages.
 Major New Features
 ------------------
 
--  ...
+- A new Implicit Conversion Sanitizer (``-fsanitize=implicit-conversion``) group
+  was added. Please refer to the :ref:`release-notes-ubsan` section of the
+  release notes for the details.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -280,10 +282,36 @@ Static Analyzer
 
 ...
 
+.. _release-notes-ubsan:
+
 Undefined Behavior Sanitizer (UBSan)
 ------------------------------------
 
-* ...
+* A new Implicit Conversion Sanitizer (``-fsanitize=implicit-conversion``) group
+  was added.
+
+  Currently, only one type of issues is caught - implicit integer truncation
+  (``-fsanitize=implicit-integer-truncation``), also known as integer demotion.
+  While there is a ``-Wconversion`` diagnostic group that catches this kind of
+  issues, it is both noisy, and does not catch **all** the cases.
+
+  .. code-block:: c++
+
+      unsigned char store = 0;
+
+      bool consume(unsigned int val);
+
+      void test(unsigned long val) {
+        if (consume(val)) // the value may have been silently truncated.
+          store = store + 768; // before addition, 'store' was promoted to int.
+        (void)consume((unsigned int)val); // OK, the truncation is explicit.
+      }
+
+  Just like 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-truncation`` check
+  is enabled by ``-fsanitize=integer``.
 
 Core Analysis Improvements
 ==========================
diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst b/clang/docs/UndefinedBehaviorSanitizer.rst
index 71a8ebd4bc6..6cf53f56833 100644
--- a/clang/docs/UndefinedBehaviorSanitizer.rst
+++ b/clang/docs/UndefinedBehaviorSanitizer.rst
@@ -89,6 +89,12 @@ Available checks are:
   -  ``-fsanitize=function``: Indirect call of a function through a
      function pointer of the wrong type (Darwin/Linux, C++ and x86/x86_64
      only).
+  -  ``-fsanitize=implicit-integer-truncation``: Implicit conversion from
+     integer of larger bit width to smaller bit width, if that results in data
+     loss. That is, if the demoted value, after casting back to the original
+     width, is not equal to the original value before the downcast.
+     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.
@@ -121,15 +127,21 @@ Available checks are:
      unsigned overflow in C++. You can use ``-fsanitize=shift-base`` or
      ``-fsanitize=shift-exponent`` to check only left-hand side or
      right-hand side of shift operation, respectively.
-  -  ``-fsanitize=signed-integer-overflow``: Signed integer overflow,
-     including all the checks added by ``-ftrapv``, and checking for
-     overflow in signed division (``INT_MIN / -1``).
+  -  ``-fsanitize=signed-integer-overflow``: Signed integer overflow, where the
+     result of a signed integer computation cannot be represented in its type.
+     This includes all the checks covered by ``-ftrapv``, as well as checks for
+     signed division overflow (``INT_MIN/-1``), but not checks for
+     lossy implicit conversions performed after the computation
+     (see ``-fsanitize=implicit-conversion``). Both of these two issues are
+     handled by ``-fsanitize=implicit-conversion`` group of checks.
   -  ``-fsanitize=unreachable``: If control flow reaches an unreachable
      program point.
-  -  ``-fsanitize=unsigned-integer-overflow``: Unsigned integer
-     overflows. Note that unlike signed integer overflow, unsigned integer
-     is not undefined behavior. However, while it has well-defined semantics,
-     it is often unintentional, so UBSan offers to catch it.
+  -  ``-fsanitize=unsigned-integer-overflow``: Unsigned integer overflow, where
+     the result of an unsigned integer computation cannot be represented in its
+     type. Unlike signed integer overflow, this is not undefined behavior, but
+     it is often unintentional. This sanitizer does not check for lossy implicit
+     conversions performed after such a computation
+     (see ``-fsanitize=implicit-conversion``).
   -  ``-fsanitize=vla-bound``: A variable-length array whose bound
      does not evaluate to a positive value.
   -  ``-fsanitize=vptr``: Use of an object whose vptr indicates that it is of
@@ -140,11 +152,17 @@ Available checks are:
 
 You can also use the following check groups:
   -  ``-fsanitize=undefined``: All of the checks listed above other than
-     ``unsigned-integer-overflow`` and the ``nullability-*`` checks.
+     ``unsigned-integer-overflow``, ``implicit-conversion`` and the
+     ``nullability-*`` group of checks.
   -  ``-fsanitize=undefined-trap``: Deprecated alias of
      ``-fsanitize=undefined``.
   -  ``-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-conversion``: Checks for suspicious behaviours of
+     implicit conversions.
+     Currently, only ``-fsanitize=implicit-integer-truncation`` is implemented.
   -  ``-fsanitize=nullability``: Enables ``nullability-arg``,
      ``nullability-assign``, and ``nullability-return``. While violating
      nullability does not have undefined behavior, it is often unintentional,