[ExprConstant] Handle shift overflow the same way as other kinds of overflow #99579

efriedma-quic · 2024-07-18T22:00:19Z

We have a mechanism to allow folding expressions that aren't ICEs as an extension; use it more consistently.

This ends up causing bad effects on diagnostics in a few cases, but that's not specific to shifts; it's a general issue with the way those uses handle overflow diagnostics.

…verflow We have a mechanism to allow folding expressions that aren't ICEs as an extension; use it more consistently. This ends up causing bad effects on diagnostics in a few cases, but that's not specific to shifts; it's a general issue with the way those uses handle overflow diagnostics.

llvmbot · 2024-07-18T22:00:50Z

@llvm/pr-subscribers-clang

Author: Eli Friedman (efriedma-quic)

Changes

We have a mechanism to allow folding expressions that aren't ICEs as an extension; use it more consistently.

This ends up causing bad effects on diagnostics in a few cases, but that's not specific to shifts; it's a general issue with the way those uses handle overflow diagnostics.

CC @budimirarandjelovichtec

Full diff: https://github.com/llvm/llvm-project/pull/99579.diff

8 Files Affected:

(modified) clang/lib/AST/ExprConstant.cpp (+15-9)
(modified) clang/lib/AST/Interp/Interp.h (+13-8)
(modified) clang/test/AST/Interp/shifts.cpp (+3-10)
(modified) clang/test/CXX/basic/basic.types/p10.cpp (+1-1)
(modified) clang/test/Sema/constant-builtins-2.c (+4-8)
(modified) clang/test/Sema/shift-count-negative.c (+8-7)
(modified) clang/test/Sema/shift-count-overflow.c (+7-6)
(modified) clang/test/Sema/shift-negative-value.c (+8-10)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 0aeac9d03eed3..bd69dc54a93dc 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2849,19 +2849,23 @@ static bool handleIntIntBinOp(EvalInfo &Info, const BinaryOperator *E,
     if (SA != RHS) {
       Info.CCEDiag(E, diag::note_constexpr_large_shift)
         << RHS << E->getType() << LHS.getBitWidth();
+      if (!Info.noteUndefinedBehavior())
+        return false;
     } else if (LHS.isSigned() && !Info.getLangOpts().CPlusPlus20) {
       // C++11 [expr.shift]p2: A signed left shift must have a non-negative
       // operand, and must not overflow the corresponding unsigned type.
       // C++2a [expr.shift]p2: E1 << E2 is the unique value congruent to
       // E1 x 2^E2 module 2^N.
-      if (LHS.isNegative())
+      if (LHS.isNegative()) {
         Info.CCEDiag(E, diag::note_constexpr_lshift_of_negative) << LHS;
-      else if (LHS.countl_zero() < SA)
+        if (!Info.noteUndefinedBehavior())
+          return false;
+      } else if (LHS.countl_zero() < SA) {
         Info.CCEDiag(E, diag::note_constexpr_lshift_discards);
+        if (!Info.noteUndefinedBehavior())
+          return false;
+      }
     }
-    if (Info.EvalStatus.Diag && !Info.EvalStatus.Diag->empty() &&
-        Info.getLangOpts().CPlusPlus11)
-      return false;
     Result = LHS << SA;
     return true;
   }
@@ -2875,6 +2879,8 @@ static bool handleIntIntBinOp(EvalInfo &Info, const BinaryOperator *E,
       // During constant-folding, a negative shift is an opposite shift. Such a
       // shift is not a constant expression.
       Info.CCEDiag(E, diag::note_constexpr_negative_shift) << RHS;
+      if (!Info.noteUndefinedBehavior())
+        return false;
       RHS = -RHS;
       goto shift_left;
     }
@@ -2882,13 +2888,13 @@ static bool handleIntIntBinOp(EvalInfo &Info, const BinaryOperator *E,
     // C++11 [expr.shift]p1: Shift width must be less than the bit width of the
     // shifted type.
     unsigned SA = (unsigned) RHS.getLimitedValue(LHS.getBitWidth()-1);
-    if (SA != RHS)
+    if (SA != RHS) {
       Info.CCEDiag(E, diag::note_constexpr_large_shift)
         << RHS << E->getType() << LHS.getBitWidth();
+      if (!Info.noteUndefinedBehavior())
+        return false;
+    }
 
-    if (Info.EvalStatus.Diag && !Info.EvalStatus.Diag->empty() &&
-        Info.getLangOpts().CPlusPlus11)
-      return false;
     Result = LHS >> SA;
     return true;
   }
diff --git a/clang/lib/AST/Interp/Interp.h b/clang/lib/AST/Interp/Interp.h
index c7d8604c7dc2a..1b17db61d7bac 100644
--- a/clang/lib/AST/Interp/Interp.h
+++ b/clang/lib/AST/Interp/Interp.h
@@ -137,7 +137,8 @@ bool CheckShift(InterpState &S, CodePtr OpPC, const LT &LHS, const RT &RHS,
   if (RHS.isNegative()) {
     const SourceInfo &Loc = S.Current->getSource(OpPC);
     S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt();
-    return false;
+    if (!S.noteUndefinedBehavior())
+      return false;
   }
 
   // C++11 [expr.shift]p1: Shift width must be less than the bit width of
@@ -147,17 +148,23 @@ bool CheckShift(InterpState &S, CodePtr OpPC, const LT &LHS, const RT &RHS,
     const APSInt Val = RHS.toAPSInt();
     QualType Ty = E->getType();
     S.CCEDiag(E, diag::note_constexpr_large_shift) << Val << Ty << Bits;
-    return !(S.getEvalStatus().Diag && !S.getEvalStatus().Diag->empty() && S.getLangOpts().CPlusPlus11);
+    if (!S.noteUndefinedBehavior())
+      return false;
   }
 
   if (LHS.isSigned() && !S.getLangOpts().CPlusPlus20) {
     const Expr *E = S.Current->getExpr(OpPC);
     // C++11 [expr.shift]p2: A signed left shift must have a non-negative
     // operand, and must not overflow the corresponding unsigned type.
-    if (LHS.isNegative())
+    if (LHS.isNegative()) {
       S.CCEDiag(E, diag::note_constexpr_lshift_of_negative) << LHS.toAPSInt();
-    else if (LHS.toUnsigned().countLeadingZeros() < static_cast<unsigned>(RHS))
+      if (!S.noteUndefinedBehavior())
+        return false;
+    } else if (LHS.toUnsigned().countLeadingZeros() < static_cast<unsigned>(RHS)) {
       S.CCEDiag(E, diag::note_constexpr_lshift_discards);
+      if (!S.noteUndefinedBehavior())
+        return false;
+    }
   }
 
   // C++2a [expr.shift]p2: [P0907R4]:
@@ -2225,8 +2232,7 @@ inline bool DoShift(InterpState &S, CodePtr OpPC, LT &LHS, RT &RHS) {
     // shift is not a constant expression.
     const SourceInfo &Loc = S.Current->getSource(OpPC);
     S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt();
-    if (S.getLangOpts().CPlusPlus11 && S.getEvalStatus().Diag &&
-        !S.getEvalStatus().Diag->empty())
+    if (!S.noteUndefinedBehavior())
       return false;
     RHS = -RHS;
     return DoShift < LT, RT,
@@ -2242,8 +2248,7 @@ inline bool DoShift(InterpState &S, CodePtr OpPC, LT &LHS, RT &RHS) {
       // E1 x 2^E2 module 2^N.
       const SourceInfo &Loc = S.Current->getSource(OpPC);
       S.CCEDiag(Loc, diag::note_constexpr_lshift_of_negative) << LHS.toAPSInt();
-      if (S.getLangOpts().CPlusPlus11 && S.getEvalStatus().Diag &&
-          !S.getEvalStatus().Diag->empty())
+      if (!S.noteUndefinedBehavior())
         return false;
     }
   }
diff --git a/clang/test/AST/Interp/shifts.cpp b/clang/test/AST/Interp/shifts.cpp
index 360b87b7ee04f..2873d6ec1bbfb 100644
--- a/clang/test/AST/Interp/shifts.cpp
+++ b/clang/test/AST/Interp/shifts.cpp
@@ -200,14 +200,7 @@ namespace LongInt {
 };
 
 enum shiftof {
-    X = (1<<-29), // all-error {{expression is not an integral constant expression}} \
-                  // all-note {{negative shift count -29}}
-
-    X2 = (-1<<29), // cxx17-error {{expression is not an integral constant expression}} \
-                   // cxx17-note {{left shift of negative value -1}} \
-                   // ref-cxx17-error {{expression is not an integral constant expression}} \
-                   // ref-cxx17-note {{left shift of negative value -1}}
-
-    X3 = (1<<32) // all-error {{expression is not an integral constant expression}} \
-                 // all-note {{shift count 32 >= width of type 'int'}}
+    X = (1<<-29),
+    X2 = (-1<<29),
+    X3 = (1<<32),
 };
diff --git a/clang/test/CXX/basic/basic.types/p10.cpp b/clang/test/CXX/basic/basic.types/p10.cpp
index a543f248e5371..92d6da0035ea5 100644
--- a/clang/test/CXX/basic/basic.types/p10.cpp
+++ b/clang/test/CXX/basic/basic.types/p10.cpp
@@ -142,7 +142,7 @@ constexpr int arb(int n) { // expected-note {{declared here}}
                expected-note {{function parameter 'n' with unknown value cannot be used in a constant expression}}
 }
 constexpr long Overflow[(1 << 30) << 2]{}; // expected-warning {{requires 34 bits to represent}} \
-                                              expected-warning {{variable length array folded to constant array as an extension}} \
+                                              expected-error {{variable length array declaration not allowed at file scope}} \
                                               expected-warning {{variable length arrays in C++ are a Clang extension}} \
                                               expected-note {{signed left shift discards bits}}
 
diff --git a/clang/test/Sema/constant-builtins-2.c b/clang/test/Sema/constant-builtins-2.c
index 00767267cd6c2..37b63cf4f6b32 100644
--- a/clang/test/Sema/constant-builtins-2.c
+++ b/clang/test/Sema/constant-builtins-2.c
@@ -265,10 +265,8 @@ char clz52[__builtin_clzg((unsigned __int128)0x1) == BITSIZE(__int128) - 1 ? 1 :
 char clz53[__builtin_clzg((unsigned __int128)0x1, 42) == BITSIZE(__int128) - 1 ? 1 : -1];
 char clz54[__builtin_clzg((unsigned __int128)0xf) == BITSIZE(__int128) - 4 ? 1 : -1];
 char clz55[__builtin_clzg((unsigned __int128)0xf, 42) == BITSIZE(__int128) - 4 ? 1 : -1];
-char clz56[__builtin_clzg((unsigned __int128)(1 << (BITSIZE(__int128) - 1))) == 0 ? 1 : -1]; // expected-warning {{variable length array folded to constant array as an extension}}
-                                                                                             // expected-note@-1 {{shift count 127 >= width of type 'int' (32 bits)}}
-char clz57[__builtin_clzg((unsigned __int128)(1 << (BITSIZE(__int128) - 1)), 42) == 0 ? 1 : -1]; // expected-warning {{variable length array folded to constant array as an extension}}
-                                                                                                 // expected-note@-1 {{shift count 127 >= width of type 'int' (32 bits)}}
+char clz56[__builtin_clzg((unsigned __int128)(1 << (BITSIZE(__int128) - 1))) == 0 ? 1 : -1]; // expected-error {{variable length array declaration not allowed at file scope}}
+char clz57[__builtin_clzg((unsigned __int128)(1 << (BITSIZE(__int128) - 1)), 42) == 0 ? 1 : -1]; // expected-error {{variable length array declaration not allowed at file scope}}
 #endif
 int clz58 = __builtin_clzg((unsigned _BitInt(128))0); // expected-error {{not a compile-time constant}}
 char clz59[__builtin_clzg((unsigned _BitInt(128))0, 42) == 42 ? 1 : -1];
@@ -276,10 +274,8 @@ char clz60[__builtin_clzg((unsigned _BitInt(128))0x1) == BITSIZE(_BitInt(128)) -
 char clz61[__builtin_clzg((unsigned _BitInt(128))0x1, 42) == BITSIZE(_BitInt(128)) - 1 ? 1 : -1];
 char clz62[__builtin_clzg((unsigned _BitInt(128))0xf) == BITSIZE(_BitInt(128)) - 4 ? 1 : -1];
 char clz63[__builtin_clzg((unsigned _BitInt(128))0xf, 42) == BITSIZE(_BitInt(128)) - 4 ? 1 : -1];
-char clz64[__builtin_clzg((unsigned _BitInt(128))(1 << (BITSIZE(_BitInt(128)) - 1))) == 0 ? 1 : -1]; // expected-warning {{variable length array folded to constant array as an extension}}
-                                                                                                     // expected-note@-1 {{shift count 127 >= width of type 'int' (32 bits)}}
-char clz65[__builtin_clzg((unsigned _BitInt(128))(1 << (BITSIZE(_BitInt(128)) - 1)), 42) == 0 ? 1 : -1]; // expected-warning {{variable length array folded to constant array as an extension}}
-                                                                                                         // expected-note@-1 {{shift count 127 >= width of type 'int' (32 bits)}}
+char clz64[__builtin_clzg((unsigned _BitInt(128))(1 << (BITSIZE(_BitInt(128)) - 1))) == 0 ? 1 : -1]; // expected-error {{variable length array declaration not allowed at file scope}}
+char clz65[__builtin_clzg((unsigned _BitInt(128))(1 << (BITSIZE(_BitInt(128)) - 1)), 42) == 0 ? 1 : -1]; // expected-error {{variable length array declaration not allowed at file scope}}
 
 char ctz1[__builtin_ctz(1) == 0 ? 1 : -1];
 char ctz2[__builtin_ctz(8) == 3 ? 1 : -1];
diff --git a/clang/test/Sema/shift-count-negative.c b/clang/test/Sema/shift-count-negative.c
index 84c7625187a68..a89d7630d0de4 100644
--- a/clang/test/Sema/shift-count-negative.c
+++ b/clang/test/Sema/shift-count-negative.c
@@ -1,11 +1,12 @@
-// RUN: %clang_cc1 -x c -fsyntax-only -verify=expected,c -pedantic %s
-// RUN: %clang_cc1 -x c++ -fsyntax-only -verify=expected,cpp %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -pedantic %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -pedantic %s
 
-// RUN: %clang_cc1 -x c -fsyntax-only -verify=expected,c -pedantic %s -fexperimental-new-constant-interpreter
-// RUN: %clang_cc1 -x c++ -fsyntax-only -verify=expected,cpp %s -fexperimental-new-constant-interpreter
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -pedantic %s -fexperimental-new-constant-interpreter
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -pedantic %s -fexperimental-new-constant-interpreter
+
+// cpp-no-diagnostics
 
 enum shiftof {
-    X = (1<<-29) // c-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}}
-                 // cpp-error@-1 {{expression is not an integral constant expression}}
-                 // expected-note@-2 {{negative shift count -29}}
+    X = (1<<-29) // expected-warning {{folding it to a constant is a GNU extension}}
+                 // expected-note@-1 {{negative shift count -29}}
 };
diff --git a/clang/test/Sema/shift-count-overflow.c b/clang/test/Sema/shift-count-overflow.c
index b5186586c2272..a7169fc4294f3 100644
--- a/clang/test/Sema/shift-count-overflow.c
+++ b/clang/test/Sema/shift-count-overflow.c
@@ -1,9 +1,10 @@
-// RUN: %clang_cc1 -fsyntax-only -verify=expected,c -pedantic %s
-// RUN: %clang_cc1 -x c++ -std=c++98 -fsyntax-only -verify=expected,cpp %s
-// RUN: %clang_cc1 -x c++ -std=c++11 -fsyntax-only -verify=expected,cpp %s
+// RUN: %clang_cc1 -fsyntax-only -verify -pedantic %s
+// RUN: %clang_cc1 -x c++ -std=c++98 -fsyntax-only -verify=cxx98 -pedantic %s
+// RUN: %clang_cc1 -x c++ -std=c++11 -fsyntax-only -verify -pedantic %s
 
 enum shiftof {
-    X = (1<<32) // c-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}}
-                // cpp-error@-1 {{expression is not an integral constant expression}}
-                // expected-note@-2 {{shift count 32 >= width of type 'int'}}
+    X = (1<<32) // expected-warning {{folding it to a constant is a GNU extension}}
+                // expected-note@-1 {{shift count 32 >= width of type 'int'}}
+                // cxx98-error@-2 {{expression is not an integral constant expression}}
+                // cxx98-note@-3 {{shift count 32 >= width of type 'int'}}
 };
diff --git a/clang/test/Sema/shift-negative-value.c b/clang/test/Sema/shift-negative-value.c
index e7b749c77d88a..fe5e9e91634e2 100644
--- a/clang/test/Sema/shift-negative-value.c
+++ b/clang/test/Sema/shift-negative-value.c
@@ -1,13 +1,11 @@
-// RUN: %clang_cc1 -x c -fsyntax-only -verify=expected,c -pedantic %s
-// RUN: %clang_cc1 -x c++ -fsyntax-only -verify=expected,cpp -Wshift-negative-value %s
-// RUN: %clang_cc1 -x c++ -fsyntax-only -verify=expected,cpp -Wall %s
-// RUN: %clang_cc1 -x c++ -std=c++98 -fsyntax-only -verify=expected,cpp -Wshift-negative-value %s
-// RUN: %clang_cc1 -x c++ -std=c++98 -fsyntax-only -verify=expected,cpp -Wall %s
-// RUN: %clang_cc1 -x c++ -std=c++11 -fsyntax-only -verify=expected,cpp -Wshift-negative-value %s
-// RUN: %clang_cc1 -x c++ -std=c++11 -fsyntax-only -verify=expected,cpp -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -pedantic %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -pedantic %s
+// RUN: %clang_cc1 -x c++ -std=c++98 -fsyntax-only -verify=cpp98 %s
+// RUN: %clang_cc1 -x c++ -std=c++11 -fsyntax-only -verify -pedantic %s
 
 enum shiftof {
-    X = (-1<<29) // c-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}}
-                 // cpp-error@-1 {{expression is not an integral constant expression}}
-                 // expected-note@-2 {{left shift of negative value -1}}
+    X = (-1<<29) // expected-warning {{folding it to a constant is a GNU extension}}
+                 // expected-note@-1 {{left shift of negative value -1}}
+                 // cpp98-error@-2 {{expression is not an integral constant expression}}
+                 // cpp98-note@-3 {{left shift of negative value -1}}
 };

github-actions · 2024-07-18T22:04:12Z

✅ With the latest revision this PR passed the C/C++ code formatter.

zygoloid · 2024-07-18T22:57:16Z

clang/lib/AST/ExprConstant.cpp

      // C++2a [expr.shift]p2: E1 << E2 is the unique value congruent to
      // E1 x 2^E2 module 2^N.
-      if (LHS.isNegative())
+      if (LHS.isNegative()) {
        Info.CCEDiag(E, diag::note_constexpr_lshift_of_negative) << LHS;
-      else if (LHS.countl_zero() < SA)
+        if (!Info.noteUndefinedBehavior())
+          return false;
+      } else if (LHS.countl_zero() < SA) {


(In passing, not related to this PR:) it's weird that the comment mentions the C++2a rule but the code doesn't implement it. This final overflow check should not be performed in C++20 onwards.

This code is under a LHS.isSigned() && !Info.getLangOpts().CPlusPlus20 guard.

zygoloid · 2024-07-18T23:04:57Z

clang/test/AST/Interp/shifts.cpp

+    X = (1<<-29),
+    X2 = (-1<<29),
+    X3 = (1<<32),


It's concerning that we don't produce a warning by default for constant-folding this, especially given that GCC does not constant-fold bad shifts. I'm not sure it's OK that we start silently accepting this invalid code by default. I wonder if we could reasonably refuse to constant-fold if the evaluation had UB?

We could try messing with the behavior of VerifyIntegerConstantExpression, I guess.

zygoloid · 2024-07-18T23:20:29Z

clang/test/CXX/basic/basic.types/p10.cpp

@@ -142,7 +142,7 @@ constexpr int arb(int n) { // expected-note {{declared here}}
               expected-note {{function parameter 'n' with unknown value cannot be used in a constant expression}}
 }
 constexpr long Overflow[(1 << 30) << 2]{}; // expected-warning {{requires 34 bits to represent}} \
-                                              expected-warning {{variable length array folded to constant array as an extension}} \
+                                              expected-error {{variable length array declaration not allowed at file scope}} \


Hm. Why do we stop folding here but start folding in enumerator values? (I assume this now consistent with how other kinds of UB in constant folding scenarios are treated, but it seems surprising that we appear to have different rules for different contexts.)

It depends on what the caller asks for. TryToFixInvalidVariablyModifiedType calls EvaluateAsInt directly in a mode that doesn't allow undefined behavior, so it's just treated as an unevaluatable expression. A few other places use CheckConvertedConstantExpression, which is also strict. Enumerators go through VerifyIntegerConstantExpression, which is not strict.

Perhaps we should file an issue to come back and improve consistency here in the future? The behavior is likely mysterious from a user perspective.

In terms of losing the extension, I think it's unlikely to cause significant disruption for anyone so I don't think it needs to be addressed immediately.

The diagnostic shouldn't be hard to fix; it's just a matter of using the form of Evaluate() that returns notes.

For the actual behavior here... we could pass the flag to allow undefined behavior when we do this folding, I guess, but I'd rather not. Or if we do, it should be a DefaultError diagnostic.

zygoloid · 2024-07-18T23:31:01Z

As noted in one of the inline comments, it doesn't seem good to be losing a diagnostic for ill-formed code. But this all makes sense to me as a step to make our behavior more consistent. I wonder if it'd make sense to make our folding behavior stricter by default as a prerequisite.

efriedma-quic · 2024-07-19T01:09:47Z

Pushed new version with an update to make Sema::VerifyIntegerConstantExpression slightly more strict. That handles the cases where the errors were getting dropped. The tradeoff is that we're more strict about arithmetic overflow in certain integer constant expressions (clang/test/SemaCXX/enum.cpp).

shafik

I think I am bit surprised more tests are not affected by this change considering you modified the handling in several places. It seems we should be adding more test coverage with this change.

AaronBallman

Thank you for working on this! In general, this is heading in the right direction. Though I do agree with @shafik that it would be nice to add additional test coverage for the changes which didn't modify existing test behavior. Also, we should have a release note for the changes and probably a mention about it being a potentially breaking change because we're rejecting code we used to explicitly accept as an extension.

AaronBallman · 2024-07-19T12:46:29Z

clang/test/CXX/basic/basic.types/p10.cpp

@@ -142,7 +142,7 @@ constexpr int arb(int n) { // expected-note {{declared here}}
               expected-note {{function parameter 'n' with unknown value cannot be used in a constant expression}}
 }
 constexpr long Overflow[(1 << 30) << 2]{}; // expected-warning {{requires 34 bits to represent}} \
-                                              expected-warning {{variable length array folded to constant array as an extension}} \
+                                              expected-error {{variable length array declaration not allowed at file scope}} \


Perhaps we should file an issue to come back and improve consistency here in the future? The behavior is likely mysterious from a user perspective.

In terms of losing the extension, I think it's unlikely to cause significant disruption for anyone so I don't think it needs to be addressed immediately.

efriedma-quic · 2024-07-19T18:42:26Z

Filed #99680 and #99684 for issues caused by custom codepaths for evaluating ICEs. Hopefully we can improve that at some point.

efriedma-quic · 2024-07-19T19:07:08Z

Added a few more tests; let me know if there are other tests I should be considering.

shafik

Thank you for the additional tests, so I am not sure if the coverage is sufficient I see changed under the following diagnostic:

note_constexpr_negative_shift
note_constexpr_large_shift
note_constexpr_lshift_of_negative
note_constexpr_lshift_discards

but I don't think I see tests that cover the negative shift changes, am I just missing it?

efriedma-quic · 2024-07-23T04:52:56Z

There's some existing coverage of the affected diagnostics, which shows we continue to produce those diagnostics... but I added a few more tests for in-class static member variables.

AaronBallman

LGTM!

efriedma-quic · 2024-07-24T19:39:47Z

/cherry-pick 20eff68

…verflow (llvm#99579) We have a mechanism to allow folding expressions that aren't ICEs as an extension; use it more consistently. This ends up causing bad effects on diagnostics in a few cases, but that's not specific to shifts; it's a general issue with the way those uses handle overflow diagnostics. (cherry picked from commit 20eff68)

llvmbot · 2024-07-24T19:44:41Z

/pull-request #100452

shafik · 2024-07-24T20:40:37Z

There's some existing coverage of the affected diagnostics, which shows we continue to produce those diagnostics... but I added a few more tests for in-class static member variables.

Thank you!

…verflow (#99579) Summary: We have a mechanism to allow folding expressions that aren't ICEs as an extension; use it more consistently. This ends up causing bad effects on diagnostics in a few cases, but that's not specific to shifts; it's a general issue with the way those uses handle overflow diagnostics. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250717

…verflow (llvm#99579) We have a mechanism to allow folding expressions that aren't ICEs as an extension; use it more consistently. This ends up causing bad effects on diagnostics in a few cases, but that's not specific to shifts; it's a general issue with the way those uses handle overflow diagnostics. (cherry picked from commit 20eff68)

efriedma-quic requested review from AaronBallman and shafik July 18, 2024 22:00

efriedma-quic requested a review from tbaederr as a code owner July 18, 2024 22:00

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 18, 2024

efriedma-quic requested a review from zygoloid July 18, 2024 22:02

efriedma-quic mentioned this pull request Jul 18, 2024

[clang] Emit bad shift warnings #70307

Merged

zygoloid reviewed Jul 18, 2024

View reviewed changes

efriedma-quic added 2 commits July 18, 2024 17:23

Fixup! add missing check for negative left shift.

788f33c

Handle overflow in C++ constant expressions more strictly.

b1f75cd

shafik reviewed Jul 19, 2024

View reviewed changes

AaronBallman reviewed Jul 19, 2024

View reviewed changes

fixup! add a few more tests.

c96b4ad

fixup! fix formatting

22a511c

shafik reviewed Jul 21, 2024

View reviewed changes

efriedma-quic added 2 commits July 22, 2024 21:46

fixup! Add release note. Add a few more tests.

644d32e

Merge branch 'main' into clang-shift-overflow-consistent

360800c

AaronBallman approved these changes Jul 24, 2024

View reviewed changes

efriedma-quic merged commit 20eff68 into llvm:main Jul 24, 2024
8 checks passed

efriedma-quic added this to the LLVM 19.X Release milestone Jul 24, 2024

philnik777 mentioned this pull request Aug 2, 2024

[Clang] Add a release note deprecating __is_nullptr #101638

Merged

mgabka mentioned this pull request Aug 22, 2024

Add release note about ABI mgabka/llvm-project#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ExprConstant] Handle shift overflow the same way as other kinds of overflow #99579

[ExprConstant] Handle shift overflow the same way as other kinds of overflow #99579

efriedma-quic commented Jul 18, 2024 •

edited

Loading

llvmbot commented Jul 18, 2024

github-actions bot commented Jul 18, 2024 •

edited

Loading

zygoloid Jul 18, 2024

efriedma-quic Jul 18, 2024

zygoloid Jul 18, 2024

efriedma-quic Jul 18, 2024

zygoloid Jul 18, 2024 •

edited

Loading

efriedma-quic Jul 18, 2024

AaronBallman Jul 19, 2024

efriedma-quic Jul 19, 2024

zygoloid commented Jul 18, 2024

efriedma-quic commented Jul 19, 2024

shafik left a comment

AaronBallman left a comment

AaronBallman Jul 19, 2024

efriedma-quic commented Jul 19, 2024

efriedma-quic commented Jul 19, 2024

shafik left a comment

efriedma-quic commented Jul 23, 2024

AaronBallman left a comment

efriedma-quic commented Jul 24, 2024

llvmbot commented Jul 24, 2024

shafik commented Jul 24, 2024

[ExprConstant] Handle shift overflow the same way as other kinds of overflow #99579

[ExprConstant] Handle shift overflow the same way as other kinds of overflow #99579

Conversation

efriedma-quic commented Jul 18, 2024 • edited Loading

llvmbot commented Jul 18, 2024

github-actions bot commented Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygoloid Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygoloid commented Jul 18, 2024

efriedma-quic commented Jul 19, 2024

shafik left a comment

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efriedma-quic commented Jul 19, 2024

efriedma-quic commented Jul 19, 2024

shafik left a comment

Choose a reason for hiding this comment

efriedma-quic commented Jul 23, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

efriedma-quic commented Jul 24, 2024

llvmbot commented Jul 24, 2024

shafik commented Jul 24, 2024

efriedma-quic commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

zygoloid Jul 18, 2024 •

edited

Loading