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

[David Grayson via cfe-commits]

DavidEGrayson added a comment.

Thanks for looking at this patch, John!  I disagreed with your comment about calculating the encompassing type, but I will incorporate everything else and make a new patch tonight or within a few days.


================
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.
 
----------------
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.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:289
@@ +288,3 @@
+static std::pair<unsigned, bool>
+EncompassingIntegerType(std::vector<std::pair<unsigned, bool>> Types) {
+  assert(Types.size() > 0 && "Empty list of types.");
----------------
rjmccall wrote:
> This should just take an ArrayRef.  Also, please at least introduce a typedef for your pair type, and consider just making a little struct for it.
A struct called IntegerWidthAndSignedness sounds good to me, thanks.  And I will learn how to use ArrayRef and use it here.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:314
@@ +313,3 @@
+static std::pair<unsigned, bool>
+IntegerWidthAndSignedness(const clang::ASTContext &context,
+                          const clang::QualType Type) {
----------------
rjmccall wrote:
> getIntegerWidthAndSignedness, please.
Sounds good.  That frees up the name IntegerWidthAndSignedness to be used for my new struct.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1601
@@ +1600,3 @@
+    auto RITy = IntegerWidthAndSignedness(CGM.getContext(), RQTy);
+    auto EITy = EncompassingIntegerType({XITy, YITy, RITy});
+
----------------
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.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1610
@@ +1609,3 @@
+    default:
+      llvm_unreachable("Unknown security overflow builtin id.");
+    case Builtin::BI__builtin_add_overflow:
----------------
rjmccall wrote:
> These aren't really security-specific; they're just arbitrary-precision.
I agree that these functions are not security-specific.  I just copied that message from a place a little bit lower in CGBuiltin.cpp where it was used to describe some other overflow builtins.  I am happy to remove the word "security" from both places.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1630
@@ +1629,3 @@
+    // Extend each operand to the encompassing type.
+    if (XQTy->isSignedIntegerType()) {
+      X = Builder.CreateSExt(X, ELTy);
----------------
rjmccall wrote:
> This is Builder.CreateIntegerCast(X, ELTy, XITy.second).
Thanks, I thought there had to be a function for that already but just could not find it.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1667
@@ +1666,3 @@
+    // we have to extend them before storing.
+    Q = Builder.CreateZExt(Q, OutPtr.getElementType());
+
----------------
rjmccall wrote:
> EmitToMemory.
Thanks again, will do.


Repository:
  rL LLVM

http://reviews.llvm.org/D12793