[Clang] Fix handling of Max from getValueRange(...) in IntExprEvaluator::VisitCastExpr(...)

[shafik]

Summary

This is a follow-up to D130058 to fix how we handle the Max value we obtain from getValueRange(...) in IntExprEvaluator::VisitCastExpr(...) which in the case of an enum that contains an enumerator with the max integer value will overflow by one.

The fix is to decrement the value of Max and use slt and ult for comparison Vs sle and ule.

getValueRange(...) is also used by CodeGenFunction::EmitScalarRangeCheck(...) through getRangeForType(...) and this code does generate the correct IR for these values even though it appears to be logically doing the same operations that does not work in IntExprEvaluator::VisitCastExpr(...).

[shafik]

@thakis I created a PR for the fix to D130058 since I wanted to document this.

As long the pre-commit checks don't show anything off I will land this.

I added a test to also cover your case as well.

[erichkeane]

clang/lib/AST/ExprConstant.cpp

13540

I don't think this is the correct answer. Even though the other use of this seems to 'work', getValueRange is still returning wrapped values here. The fix is to figure out how to fix the math in getValueRange, and change the sanitizer's IR generation if necessary.

[shafik]

clang/lib/AST/ExprConstant.cpp

13540

So the other user of these values is getRangeForLoadFromType(...) through getRangeForType(...) which is used to generate the range metadata and in this case the range is exclusive on the right side:

The pair a,b represents the range [a,b).

and it is allowed to wrap:

The range is allowed to wrap.

So I believe the behavior will be correct.

Even though the range looks off in this example: https://godbolt.org/z/z7d9PKoMn

!{i32 0, i32 -2147483648}

I think it does the right thing.

Note the language ref also says:

The type must match the type loaded by the instruction.

and a quick experiment to use i33 confirms this breaks.

[erichkeane]

clang/lib/AST/ExprConstant.cpp

13540

Hmm... ok. Well, its quite unfortunate that 2 of our 3 uses of this are having to hack around this behavior a bit, but I guess this will have to be acceptable short-term then.

[shafik]

clang/lib/AST/ExprConstant.cpp

13540

I agree, maybe getValueRange(...) needs a flag for inclusive Vs exclusive. That would remove the hack and make it explicit that we have a different requirement at the call site.

clang/docs/ReleaseNotes.rst

 - Fixes an accepts-invalid bug in C when using a ``_Noreturn`` function
   specifier on something other than a function declaration. This fixes
   `Issue 56800 <https://github.com/llvm/llvm-project/issues/56800>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now correctly diagnose as ill-formed a constant expression where an
   enum without a fixed underlying type is set to a value outside the range of
-  of the enumerations values. Fixes
+  the enumeration's values. Fixes
   `Issue 50055: <https://github.com/llvm/llvm-project/issues/50055>`_.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 New Compiler Flags
 ------------------
 

clang/include/clang/Basic/DiagnosticASTKinds.td

 def note_constexpr_memory_leak : Note<
   "allocation performed here was not deallocated"
   "%plural{0:|: (along with %0 other memory leak%s0)}0">;
 def note_constexpr_unsupported_layout : Note<
   "type %0 has unexpected layout">;
 def note_constexpr_unsupported_flexible_array : Note<
   "flexible array initialization is not yet supported">;
 def note_constexpr_unscoped_enum_out_of_range : Note<
-  "integer value %0 is outside the valid range of values [%1, %2) for this "
+  "integer value %0 is outside the valid range of values [%1, %2] for this "
   "enumeration type">;
 def err_experimental_clang_interp_failed : Error<
   "the experimental clang interpreter failed to evaluate an expression">;
 
 def warn_integer_constant_overflow : Warning<
   "overflow in expression; result is %0 with type %1">,
   InGroup<DiagGroup<"integer-overflow">>;
 def warn_fixedpoint_constant_overflow : Warning<

clang/lib/AST/ExprConstant.cpp

if (DestType->isEnumeralType()) {
       // otherwise, the behavior is undefined.
       //
       // This was resolved as part of DR2338 which has CD5 status.
       if (!ED->isFixed()) {
         llvm::APInt Min;
         llvm::APInt Max;
 
         ED->getValueRange(Max, Min);
+        --Max;
 
         if (ED->getNumNegativeBits() &&
-            (Max.sle(Result.getInt().getSExtValue()) ||
+            (Max.slt(Result.getInt().getSExtValue()) ||
              Min.sgt(Result.getInt().getSExtValue())))
           CCEDiag(E, diag::note_constexpr_unscoped_enum_out_of_range)
               << Result.getInt() << Min.getSExtValue() << Max.getSExtValue();
         else if (!ED->getNumNegativeBits() &&
-                 Max.ule(Result.getInt().getZExtValue()))
+                 Max.ult(Result.getInt().getZExtValue()))
           CCEDiag(E, diag::note_constexpr_unscoped_enum_out_of_range)
               << Result.getInt() << Min.getZExtValue() << Max.getZExtValue();
       }
     }
 
     return Success(HandleIntToIntCast(Info, E, DestType, SrcType,
                                       Result.getInt()), E);
   }

clang/test/SemaCXX/constant-expression-cxx11.cpp

 // Empty but as-if it had a single enumerator with value 0
 enum EEmpty {};
 
 // Enum with fixed underlying type because the underlying type is explicitly specified
 enum EFixed : int {efixed1=-4, efixed2=4};
 // Enum with fixed underlying type because it is scoped
 enum class EScoped {escoped1=-4, escoped2=4};
 
+enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
+
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast<E1>(-8);
   constexpr E1 x2 = static_cast<E1>(8); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value 8 is outside the valid range of values [-8, 8) for this enumeration type}}
+  // expected-note@-1 {{integer value 8 is outside the valid range of values [-8, 7] for this enumeration type}}
 
   constexpr E2 x3 = static_cast<E2>(-8); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value -8 is outside the valid range of values [0, 8) for this enumeration type}}
+  // expected-note@-1 {{integer value -8 is outside the valid range of values [0, 7] for this enumeration type}}
   constexpr E2 x4 = static_cast<E2>(0);
   constexpr E2 x5 = static_cast<E2>(8); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value 8 is outside the valid range of values [0, 8) for this enumeration type}}
+  // expected-note@-1 {{integer value 8 is outside the valid range of values [0, 7] for this enumeration type}}
 
   constexpr E3 x6 = static_cast<E3>(-2048);
   constexpr E3 x7 = static_cast<E3>(-8);
   constexpr E3 x8 = static_cast<E3>(0);
   constexpr E3 x9 = static_cast<E3>(8);
   constexpr E3 x10 = static_cast<E3>(2048); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value 2048 is outside the valid range of values [-2048, 2048) for this enumeration type}}
+  // expected-note@-1 {{integer value 2048 is outside the valid range of values [-2048, 2047] for this enumeration type}}
 
   constexpr E4 x11 = static_cast<E4>(0);
   constexpr E4 x12 = static_cast<E4>(1);
   constexpr E4 x13 = static_cast<E4>(2); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value 2 is outside the valid range of values [0, 2) for this enumeration type}}
+  // expected-note@-1 {{integer value 2 is outside the valid range of values [0, 1] for this enumeration type}}
 
   constexpr EEmpty x14 = static_cast<EEmpty>(0);
   constexpr EEmpty x15 = static_cast<EEmpty>(1);
   constexpr EEmpty x16 = static_cast<EEmpty>(2); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value 2 is outside the valid range of values [0, 2) for this enumeration type}}
+  // expected-note@-1 {{integer value 2 is outside the valid range of values [0, 1] for this enumeration type}}
 
   constexpr EFixed x17 = static_cast<EFixed>(100);
   constexpr EScoped x18 = static_cast<EScoped>(100);
+
+  constexpr EMaxInt x19 = static_cast<EMaxInt>(__INT_MAX__-1);
+  constexpr EMaxInt x20 = static_cast<EMaxInt>((long)__INT_MAX__+1); // expected-error {{must be initialized by a constant expression}}
+  // expected-note@-1 {{integer value 2147483648 is outside the valid range of values [-2147483648, 2147483647] for this enumeration type}}
 }
 }