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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
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
----------------
vsk wrote:
> 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.
Is this better?


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


================
Comment at: lib/CodeGen/CodeGenFunction.h:388
+
+    llvm::SmallVector<const CastExpr *, 2> GuardedCasts;
+
----------------
vsk wrote:
> 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 *>.
No longer relevant.


================
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
----------------
vsk wrote:
> There's no need to check the profile metadata here.
I was checking it because otherwise `HANDLER_IMPLICIT_CAST` would have over-eagerly consumed `, !prof !3` too.
But there is actually a way around that..


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


Repository:
  rC Clang

https://reviews.llvm.org/D48958