Fix constexpr __builtin_*_overflow issue when unsigned->signed operand.

[erichkeane]

Summary

As reported here https://bugs.llvm.org/show_bug.cgi?id=42000, it was
possible to get the constexpr version of __builtin_*_overflow to give
the wrong answer.

This was because when extending the operands to fit the largest type (so
that the math could be done), the decision on whether to sign/zero
extend the operands was based on the result signedness, not on the
operands signedness.

In the reported case, (unsigned char)255 - (int)100 needed
to have each extended to the int in order to do the math. However, when
extending the first operand to 'int', we incorrectly sign extended it
instead of zero extending. Thus, the result didnt fit back into the
unsigned char.

The fix for this was simply to choose zero/sign extension based on the
sign of the operand itself.

[efriedma]

clang/lib/AST/ExprConstant.cpp

9457 ↗

(On Diff #202189)

Can you just write LHS = APSInt(LHS.extOrTrunc(MaxBits), !IsSigned);, or something like that?

[erichkeane]

clang/lib/AST/ExprConstant.cpp

9457 ↗

(On Diff #202189)

I believe I can! I was unaware of this function.

[efriedma]

LGTM

cfe/trunk/lib/AST/ExprConstant.cpp

if (BuiltinOp == Builtin::BI__builtin_add_overflow ||
 
       // Add an additional bit if the signedness isn't uniformly agreed to. We
       // could do this ONLY if there is a signed and an unsigned that both have
       // MaxBits, but the code to check that is pretty nasty.  The issue will be
       // caught in the shrink-to-result later anyway.
       if (IsSigned && !AllSigned)
         ++MaxBits;
 
-      LHS = APSInt(IsSigned ? LHS.sextOrSelf(MaxBits) : LHS.zextOrSelf(MaxBits),
-                   !IsSigned);
-      RHS = APSInt(IsSigned ? RHS.sextOrSelf(MaxBits) : RHS.zextOrSelf(MaxBits),
-                   !IsSigned);
+      LHS = APSInt(LHS.extOrTrunc(MaxBits), !IsSigned);
+      RHS = APSInt(RHS.extOrTrunc(MaxBits), !IsSigned);
       Result = APSInt(MaxBits, !IsSigned);
     }
 
     // Find largest int.
     switch (BuiltinOp) {
     default:
       llvm_unreachable("Invalid value for BuiltinOp");
     case Builtin::BI__builtin_add_overflow:

cfe/trunk/test/SemaCXX/builtins-overflow.cpp

 // RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
 // expected-no-diagnostics
 
 #include <limits.h>
+#include <stdint.h>
 
 int a() {
   const int x = 3;
   static int z;
   constexpr int *y = &z;
   return []() { return __builtin_sub_overflow((int)x, (int)x, (int *)y); }();
 }
 int a2() {

 constexpr Result<RET> sub(LHS &&lhs, RHS &&rhs) {
   RET sum{};
   return {__builtin_sub_overflow(lhs, rhs, &sum), sum};
 }
 
 static_assert(sub<unsigned char>(static_cast<char>(0),static_cast<char>(1)) == Result<unsigned char>{true, UCHAR_MAX});
 static_assert(sub<char>(static_cast<unsigned char>(0),static_cast<unsigned char>(1)) == Result<char>{false, -1});
 static_assert(sub<unsigned short>(static_cast<short>(0),static_cast<short>(1)) == Result<unsigned short>{true, USHRT_MAX});
+static_assert(sub<uint8_t>(static_cast<uint8_t>(255),static_cast<int>(100)) == Result<uint8_t>{false, 155});
 
 static_assert(sub<int>(17,22) == Result<int>{false, -5});
 static_assert(sub<int>(INT_MAX - 22, -23) == Result<int>{true, INT_MIN});
 static_assert(sub<int>(INT_MIN + 22, 23) == Result<int>{true, INT_MAX});
 
 template <typename RET, typename LHS, typename RHS>
 constexpr Result<RET> mul(LHS &&lhs, RHS &&rhs) {
   RET sum{};

 constexpr Result<int> smul(int lhs, int rhs) {
   int sum{};
   return {__builtin_smul_overflow(lhs, rhs, &sum), sum};
 }
 
 static_assert(smul(17,22) == Result<int>{false, 374});
 static_assert(smul(INT_MAX / 22, 23) == Result<int>{true, -2049870757});
 static_assert(smul(INT_MIN / 22, -23) == Result<int>{true, -2049870757});
+