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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

@erichkeane, @rsmith thanks for taking a look!



================
Comment at: lib/CodeGen/CGExprScalar.cpp:1036
+    return;
+  // That's it. We can't rule out any more cases with the data we have.
+
----------------
rsmith wrote:
> I don't like the overlap between the implicit truncation check and this check. I think you should aim for exactly one of those checks to fire for any given integer conversion. There are the following cases:
> 
>  * Dst is smaller than Src: if the value changes at all (with sign change or without), then the truncation check already catches it, and catching it here does not seem useful
>  * Dst is the same size as Src or larger: sign change is the only problem, and is only possible if exactly one of Src and Dst is signed
> 
> So I think you should bail out of this function if either Src and Dst are both unsigned or both are signed, and also if Src is larger than Dst (because we treat that case as a lossy truncation rather than as a sign change).
> 
> And when you do emit a check here, the only thing you need to check is if the signed value is negative (if so, you definitely changed the sign, and if not, you definitely didn't -- except in the truncation cases that the truncation sanitizer catches).
To be clear: we want to skip emitting in those cases if the other check (truncation) is enabled, right?
It does seem to make sense, (and i did thought about that a bit), but i need to think about it more..


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1050-1051
+    // NOTE: if it is unsigned, then the comparison is naturally always 'false'.
+    llvm::ICmpInst::Predicate Pred =
+        VSigned ? llvm::ICmpInst::ICMP_SLT : llvm::ICmpInst::ICMP_ULT;
+    // Get the zero of the same type with which we will be comparing.
----------------
rsmith wrote:
> If `!VSigned`, the result is a constant `false`; you don't need to emit an `icmp` to work that out.
Ok, if you insist.
I didn't do that in the first place because we will now have an `icmp`
where one operand being a constant, so we can simplify it further.
And i don't want to complicate this logic if middle-end already handles it :)


Repository:
  rC Clang

https://reviews.llvm.org/D50250