[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1008
+  // We ignore conversions to/from pointer and/or bool.
+  if (!(SrcType->isIntegerType() && DstType->isIntegerType()))
+    return;
----------------
erichkeane wrote:
> I'd rather !SrcType->isInt || !DestType->isInt
This i'd prefer to keep this as-is, since this is copied verbatim from `ScalarExprEmitter::EmitIntegerTruncationCheck()`.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1011
+
+  assert(isa<llvm::IntegerType>(SrcTy) && isa<llvm::IntegerType>(DstTy) &&
+         "clang integer type lowered to non-integer llvm type");
----------------
erichkeane wrote:
> This seems like a silly assert, since you did the check above.
If this assertion doesn't hold, we'll (hopefully!) hit an assertion somewhere down in the [IR] Builder.
I think it would be best to be proactive here.
(Similarly, this is copied verbatim from `ScalarExprEmitter::EmitIntegerTruncationCheck()`.)


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1030
+  {
+    // At least one of the values needs to have signed type.
+    // If both are unsigned, then obviously, neither of them can be negative.
----------------
erichkeane wrote:
> Does this really need its own scope?
Yeah, they got out of hand here..


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1032
+    // If both are unsigned, then obviously, neither of them can be negative.
+    if (!(SrcSigned || DstSigned))
+      return;
----------------
erichkeane wrote:
> Again, I'd rather we distribute the '!'.
Here - ok.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1044
+
+  assert(!DstType->isBooleanType() && "we should not get here with booleans.");
+
----------------
erichkeane wrote:
> Curious what prevents this?
Right now this was simply copied from `ScalarExprEmitter::EmitIntegerTruncationCheck()`,
where it made sense (since conversion to bool should be comparison with 0, not truncation).
I'm not quite sure about booleans here. I think i should just drop it, at least for now.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1054
+                                   const char *Name) -> Value * {
+    // Does this Value has signed type?
+    bool VSigned = VType->isSignedIntegerOrEnumerationType();
----------------
erichkeane wrote:
> 
> // Is this value a signed type?
That reads strange, but i don't have a better idea.


Repository:
  rC Clang

https://reviews.llvm.org/D50250