[PATCH] D12793: Three new security overflow builtins with generic argument types

[John McCall via cfe-commits]

rjmccall added inline comments.

================
Comment at: docs/LanguageExtensions.rst:1720
@@ -1712,1 +1719,3 @@
+being stored there, and the function returns 1.  The behavior of these builtins
+is well-defined for all argument values.
 
----------------
DavidEGrayson wrote:
> rjmccall wrote:
> > Hmm.  It's not necessarily truncation; you could meaningfully use these intrinsics to add two small signed numbers and store the result in an unsigned number.  It's the unique representative value modulo 2^n, where n is the bit-width of the result.
> OK, I'll change the documentation because I like your way of phrasing it, but I think truncation always gives you the unique representative value modulo 2^n.  If I'm adding ((int8_t)-1) to ((int8_t)-1), the infinite-width result would be -2, which in infinite-width binary is 11...1111111111110.  Then we truncate it to 8 bits to get 11111110, which is 254.  And 254 is also the unique representative value of -2 modulo 256.
> 
When talking about mathematical abstractions, I think it's better to stick with mathematical presentations all the way through instead of mixing in the idea that the universe is 2's-complement. :)

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1601
@@ +1600,3 @@
+    auto RITy = IntegerWidthAndSignedness(CGM.getContext(), RQTy);
+    auto EITy = EncompassingIntegerType({XITy, YITy, RITy});
+
----------------
DavidEGrayson wrote:
> rjmccall wrote:
> > These are not the most evocative names you could have chosen. :)
> > 
> > Also, this strategy is correct, but it produces horrible code when the operands have the same signedness and the result is different, where the result would otherwise be an ordinary operation and a compare.  You really want to just merge the two operands and then max with the width of the result type.
> I don't think that would work.  It sounds like you want me to remove RITy from the call to EncompassingIntegerType.  That means the ecompassing type could be smaller than the result type, and the arithmetic intrinsic would report an overflow even though the mathematical result is representable in the result type.
> 
> For example, if I am multiplying ((uint8_t)100) by ((uint8_t)100) and storing the result in a (uint16_t), the result needs to be 10000 and there is no overflow.  To arrive at that result, we need to convert the operands to 16-bit unsigned integers before multiplying them.  If we multiply them as 8-bit unsigned integers, the multiplication intrinsic will report an overflow and give a result of 16.  That seems like a messy situation and I don't see how a max operation would fix it.
I think I was unclear.  I'm not saying that you should do a max as part of computing the dynamic result.  I'm saying that, when deciding the encompassing type, it should still be at least as wide as the result, but you should ignore the result type's signedness and then patch it up with a comparison at the end if necessary.  (I believe that in both cases, it's a signed comparison against zero.)

That is, when multiplying two uint8_t's and storing the result in an int16_t, we should just do an unsigned 16-bit multiply-with-overflow.  The computation overflows if the multiply overflows (which it provably doesn't, but we can let LLVM figure that out for now) or the result is signed-< 0.


Repository:
  rL LLVM

http://reviews.llvm.org/D12793