[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.


================
Comment at: docs/UndefinedBehaviorSanitizer.rst:97
+     is not equal to the original value before the downcast. This kind of issues
+     may often be caused by an implicit integer promotions.
   -  ``-fsanitize=integer-divide-by-zero``: Integer division by zero.
----------------
Nitpicks:
kind of issues -> issue
promotions -> conversions


================
Comment at: docs/UndefinedBehaviorSanitizer.rst:134
+     integer promotions, as those may result in an unexpected computation
+     results, even though no overflow happens (signed or unsigned).
   -  ``-fsanitize=unreachable``: If control flow reaches an unreachable
----------------
Could you make this more explicit? It would help to point out that this check does not diagnose lossy implicit integer conversions, but that the new check does. Ditto for the comment in the unsigned-integer-overflow section.


================
Comment at: lib/CodeGen/CodeGenFunction.h:383
+  // This stack is used/maintained exclusively by the implicit cast sanitizer.
+  llvm::SmallVector<const CastExpr *, 8> CastExprStack;
+
----------------
Why not 0 instead of 8, given that in the common case, this stack is unused?


================
Comment at: lib/CodeGen/CodeGenFunction.h:388
+
+    llvm::SmallVector<const CastExpr *, 2> GuardedCasts;
+
----------------
I'm not sure the cost of maintaining an extra vector is worth the benefit of the added assertion. Wouldn't it be cheaper to just store the number of pushed casts? You'd only need one constructor which accepts an ArrayRef<const CastExpr *>.


================
Comment at: test/CodeGen/catch-implicit-integer-truncations.c:29
+  // CHECK-SANITIZE-NEXT: %[[TRUNCHECK:.*]] = icmp eq i32 %[[ANYEXT]], %[[SRC]], !nosanitize
+  // CHECK-SANITIZE-ANYRECOVER-NEXT: br i1 %[[TRUNCHECK]], label %[[CONT:.*]], label %[[HANDLER_IMPLICIT_CAST:.*]], !prof ![[WEIGHT_MD:.*]], !nosanitize
+  // CHECK-SANITIZE-TRAP-NEXT: br i1 %[[TRUNCHECK]], label %[[CONT:.*]], label %[[HANDLER_IMPLICIT_CAST:.*]], !nosanitize
----------------
There's no need to check the profile metadata here.


================
Comment at: test/CodeGen/catch-implicit-integer-truncations.c:159
+// ========================================================================== //
+// The expected false-negatives.
+// ========================================================================== //
----------------
nit, aren't these true-negatives because we expect to see no errors?


Repository:
  rC Clang

https://reviews.llvm.org/D48958