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

[John McCall via cfe-commits]

rjmccall added a comment.

Thanks for doing this; this is a great start.


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

================
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.");
----------------
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.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:314
@@ +313,3 @@
+static std::pair<unsigned, bool>
+IntegerWidthAndSignedness(const clang::ASTContext &context,
+                          const clang::QualType Type) {
----------------
getIntegerWidthAndSignedness, please.

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

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1610
@@ +1609,3 @@
+    default:
+      llvm_unreachable("Unknown security overflow builtin id.");
+    case Builtin::BI__builtin_add_overflow:
----------------
These aren't really security-specific; they're just arbitrary-precision.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:1630
@@ +1629,3 @@
+    // Extend each operand to the encompassing type.
+    if (XQTy->isSignedIntegerType()) {
+      X = Builder.CreateSExt(X, ELTy);
----------------
This is Builder.CreateIntegerCast(X, ELTy, XITy.second).

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


Repository:
  rL LLVM

http://reviews.llvm.org/D12793