[PATCH] D53949: [clang][CodeGen] Implicit Conversion Sanitizer: discover the world of CompoundAssign operators

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: rsmith, regehr, vsk, rjmccall, Sanitizers.
lebedev.ri added projects: clang, Sanitizers.

Not yet for an //actual// review, needs tests.

As reported by @regehr (thanks!) on twitter (https://twitter.com/johnregehr/status/1057681496255815686),
we (me) has completely forgot about the binary assignment operator.
In AST, it isn't represented as separate `ImplicitCastExpr`'s,
but as a single `CompoundAssignOperator`, that does all the casts internally.
Which means, out of these two, only the first one is diagnosed:

  auto foo() {
      unsigned char c = 255;
      c = c + 1;
      return c;
  }
  auto bar() {
      unsigned char c = 255;
      c += 1;
      return c;
  }

https://godbolt.org/z/JNyVc4

This patch does handle the `CompoundAssignOperator`:

  int main() {
    unsigned char c = 255;
    c += 1;
    return c;
  }

  $ ./bin/clang -g -fsanitize=integer /tmp/test.c && ./a.out 
  /tmp/test.c:3:5: runtime error: implicit conversion from type 'int' of value 256 (32-bit, signed) to type 'unsigned char' changed the value to 0 (8-bit, unsigned)
      #0 0x2392b8 in main /tmp/test.c:3:5
      #1 0x7fec4a612b16 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x22b16)
      #2 0x214029 in _start (/build/llvm-build-GCC-release/a.out+0x214029)


Repository:
  rC Clang

https://reviews.llvm.org/D53949

Files:
  lib/CodeGen/CGExprScalar.cpp


Index: lib/CodeGen/CGExprScalar.cpp
===================================================================
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -332,6 +332,13 @@
         : TreatBooleanAsSigned(false),
           EmitImplicitIntegerTruncationChecks(false),
           EmitImplicitIntegerSignChangeChecks(false) {}
+
+    ScalarConversionOpts(clang::SanitizerSet SanOpts)
+        : TreatBooleanAsSigned(false),
+          EmitImplicitIntegerTruncationChecks(
+              SanOpts.hasOneOf(SanitizerKind::ImplicitIntegerTruncation)),
+          EmitImplicitIntegerSignChangeChecks(
+              SanOpts.has(SanitizerKind::ImplicitIntegerSignChange)) {}
   };
   Value *
   EmitScalarConversion(Value *Src, QualType SrcTy, QualType DstTy,
@@ -2198,13 +2205,8 @@
   case CK_IntegralCast: {
     ScalarConversionOpts Opts;
     if (auto *ICE = dyn_cast<ImplicitCastExpr>(CE)) {
-      if (CGF.SanOpts.hasOneOf(SanitizerKind::ImplicitConversion) &&
-          !ICE->isPartOfExplicitCast()) {
-        Opts.EmitImplicitIntegerTruncationChecks =
-            CGF.SanOpts.hasOneOf(SanitizerKind::ImplicitIntegerTruncation);
-        Opts.EmitImplicitIntegerSignChangeChecks =
-            CGF.SanOpts.has(SanitizerKind::ImplicitIntegerSignChange);
-      }
+      if (!ICE->isPartOfExplicitCast())
+        Opts = ScalarConversionOpts(CGF.SanOpts);
     }
     return EmitScalarConversion(Visit(E), E->getType(), DestTy,
                                 CE->getExprLoc(), Opts);
@@ -2872,9 +2874,10 @@
   // Expand the binary operator.
   Result = (this->*Func)(OpInfo);
 
-  // Convert the result back to the LHS type.
-  Result =
-      EmitScalarConversion(Result, E->getComputationResultType(), LHSTy, Loc);
+  // Convert the result back to the LHS type,
+  // potentially with Implicit Conversion sanitizer check.
+  Result = EmitScalarConversion(Result, E->getComputationResultType(), LHSTy,
+                                Loc, ScalarConversionOpts(CGF.SanOpts));
 
   if (atomicPHI) {
     llvm::BasicBlock *opBB = Builder.GetInsertBlock();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D53949.171986.patch
Type: text/x-patch
Size: 2075 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181031/7f17b355/attachment.bin>