Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,8 @@ Improvements to Clang's diagnostics

UsingWithAttr<int> objUsingWA; // warning: 'UsingWithAttr' is deprecated

- Clang now diagnoses undefined behavior in constant expressions more consistently. This includes invalid shifts, and signed overflow in arithmetic.

Improvements to Clang's time-trace
----------------------------------

Expand Down
26 changes: 17 additions & 9 deletions clang/lib/AST/ExprConstant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2839,6 +2839,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_right;
}
Expand All @@ -2849,19 +2851,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) {
Comment on lines 2859 to +2865
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
}
Expand All @@ -2875,20 +2881,22 @@ 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;
}
shift_right:
// 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;
}
Expand Down
22 changes: 14 additions & 8 deletions clang/lib/AST/Interp/Interp.h
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,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
Expand All @@ -163,17 +164,24 @@ 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]:
Expand Down Expand Up @@ -2264,8 +2272,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,
Expand All @@ -2281,8 +2288,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;
}
}
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/Sema/SemaExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17027,7 +17027,8 @@ Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
// not a constant expression as a side-effect.
bool Folded =
E->EvaluateAsRValue(EvalResult, Context, /*isConstantContext*/ true) &&
EvalResult.Val.isInt() && !EvalResult.HasSideEffects;
EvalResult.Val.isInt() && !EvalResult.HasSideEffects &&
(!getLangOpts().CPlusPlus || !EvalResult.HasUndefinedBehavior);

if (!isa<ConstantExpr>(E))
E = ConstantExpr::Create(Context, E, EvalResult.Val);
Expand Down
2 changes: 1 addition & 1 deletion clang/test/CXX/basic/basic.types/p10.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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}} \
Copy link
Collaborator

@zygoloid zygoloid Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expected-warning {{variable length arrays in C++ are a Clang extension}} \
expected-note {{signed left shift discards bits}}

Expand Down
12 changes: 4 additions & 8 deletions clang/test/Sema/constant-builtins-2.c
Original file line number Diff line number Diff line change
Expand Up @@ -265,21 +265,17 @@ 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];
char clz60[__builtin_clzg((unsigned _BitInt(128))0x1) == BITSIZE(_BitInt(128)) - 1 ? 1 : -1];
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];
Expand Down
9 changes: 8 additions & 1 deletion clang/test/SemaCXX/class.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %clang_cc1 -fsyntax-only -verify -Wc++11-compat %s
// RUN: %clang_cc1 -fsyntax-only -verify=expected,cxx11 -Wc++11-compat %s
// RUN: %clang_cc1 -fsyntax-only -verify -Wc++11-compat %s -std=c++98
class C {
public:
Expand Down Expand Up @@ -55,6 +55,13 @@ class C {
// expected-error@-2 {{static const volatile data member must be initialized out of line}}
#endif
static const E evi = 0;
static const int overflow = 1000000*1000000; // cxx11-error {{in-class initializer for static data member is not a constant expression}}
// expected-warning@-1 {{overflow in expression}}
static const int overflow_shift = 1<<32; // cxx11-error {{in-class initializer for static data member is not a constant expression}}
static const int overflow_shift2 = 1>>32; // cxx11-error {{in-class initializer for static data member is not a constant expression}}
static const int overflow_shift3 = 1<<-1; // cxx11-error {{in-class initializer for static data member is not a constant expression}}
static const int overflow_shift4 = 1<<-1; // cxx11-error {{in-class initializer for static data member is not a constant expression}}
static const int overflow_shift5 = -1<<1; // cxx11-error {{in-class initializer for static data member is not a constant expression}}

void m() {
sx = 0;
Expand Down
24 changes: 17 additions & 7 deletions clang/test/SemaCXX/enum.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,14 @@ void PR8089() {
// This is accepted as a GNU extension. In C++98, there was no provision for
// expressions with UB to be non-constant.
enum { overflow = 123456 * 234567 };
#if __cplusplus >= 201103L
// expected-warning@-2 {{expression is not an integral constant expression; folding it to a constant is a GNU extension}}
// expected-note@-3 {{value 28958703552 is outside the range of representable values of type 'int'}}
#else
// expected-error@-5 {{expression is not an integral constant expression}}
// expected-note@-6 {{value 28958703552 is outside the range of representable values of type 'int'}}
// expected-warning@-7 {{overflow in expression; result is -1'106'067'520 with type 'int'}}
// expected-error@-1 {{expression is not an integral constant expression}}
// expected-note@-2 {{value 28958703552 is outside the range of representable values of type 'int'}}
#if __cplusplus < 201103L
// expected-warning@-4 {{overflow in expression; result is -1'106'067'520 with type 'int'}}
#endif
enum { overflow_shift = 1 << 32 };
// expected-error@-1 {{expression is not an integral constant expression}}
// expected-note@-2 {{shift count 32 >= width of type 'int' (32 bits)}}

// FIXME: This is not consistent with the above case.
enum NoFold : int { overflow2 = 123456 * 234567 };
Expand All @@ -123,6 +123,16 @@ enum NoFold : int { overflow2 = 123456 * 234567 };
// expected-error@-7 {{expression is not an integral constant expression}}
// expected-note@-8 {{value 28958703552 is outside the range of representable values of type 'int'}}
#endif
enum : int { overflow2_shift = 1 << 32 };
#if __cplusplus >= 201103L
// expected-error@-2 {{enumerator value is not a constant expression}}
// expected-note@-3 {{shift count 32 >= width of type 'int' (32 bits)}}
#else
// expected-error@-5 {{expression is not an integral constant expression}}
// expected-note@-6 {{shift count 32 >= width of type 'int' (32 bits)}}
// expected-warning@-7 {{enumeration types with a fixed underlying type are a C++11 extension}}
#endif


// PR28903
struct PR28903 {
Expand Down
Loading