This is an archive of the discontinued LLVM Phabricator instance.

clang/lib/Sema/SemaExpr.cpp
11418	... to early here instead, and it should also check `isSignedOverflowDefined()` (note: we should probably fix `SignedOverflowBehavior` in LangOptions.def to be set based on the C++ language mode instead of needing to check for C++20 specifically)
11418–11428	and then we can remove the check for signed overflow defined or C++20 here because we've already bailed out.
11458–11459	I think this should move up...

erichkeane added a subscriber: erichkeane.Jun 13 2022, 7:34 AM

erichkeane added inline comments.

clang/lib/Sema/SemaExpr.cpp
11418–11428	I think we'd want to remove the CPlusPlus20 check here, right? Signed-overflow should cover this case as well, and the check for C++20 below is no longer necessary?

Address comments

Harbormaster completed remote builds in B169559: Diff 436557.Jun 13 2022, 4:20 PM

aaron.ballman added inline comments.Jun 14 2022, 3:56 AM

clang/lib/Sema/SemaExpr.cpp
11419–11421	I think this should also be checking `\|\| isSignedOverflowDefined()` -- if signed overflow is defined, then all the rest of the diagnostics shouldn't be triggered because the behavior is defined. (Eventually, we should remove the check for `CPlusPlus20` and use ONLY `isSignedOverflowDefined()` -- that should account for the current language mode instead of requiring us to check additional predicates. But that doesn't have to be fixed up as part of this patch.)
11419–11428	Then the `isSignedOverflowDefined()` check can be removed here.

%2

This revision is now accepted and ready to land.Jun 14 2022, 6:55 AM

LGTM! Please add a release note about the issue that was fixed when landing.

Harbormaster completed remote builds in B169693: Diff 436746.Jun 14 2022, 6:59 AM

In D127518#3581620, @aaron.ballman wrote:

LGTM! Please add a release note about the issue that was fixed when landing.

Done, Thank you for reviewing.

Committed in https://reviews.llvm.org/rG48e1829874df15fd57e731d4824604f28e177585.

Revision Contents

Path

Size

clang/

lib/

Sema/

SemaExpr.cpp

14 lines

test/

CXX/

expr/

expr.const/

p2-0x.cpp

6 lines

SemaCXX/

constant-expression-cxx2a.cpp

4 lines

shift.cpp

6 lines

Diff 436746

clang/lib/Sema/SemaExpr.cpp

This file is larger than 256 KB, so syntax highlighting is disabled by default.

Show First 20 Lines • Show All 11,409 Lines • ▼ Show 20 Lines static void DiagnoseBadShiftValues(Sema& S, ExprResult &LHS, ExprResult &RHS,

// warn for those. (FIXME: Unsigned left-shift overflow in a constant // warn for those. (FIXME: Unsigned left-shift overflow in a constant

// expression is still probably a bug.) // expression is still probably a bug.)

Expr::EvalResult LHSResult; Expr::EvalResult LHSResult;

if (LHS.get()->isValueDependent() || if (LHS.get()->isValueDependent() ||

LHSType->hasUnsignedIntegerRepresentation() || LHSType->hasUnsignedIntegerRepresentation() ||

!LHS.get()->EvaluateAsInt(LHSResult, S.Context)) !LHS.get()->EvaluateAsInt(LHSResult, S.Context))

return; return;

llvm::APSInt Left = LHSResult.Val.getInt(); llvm::APSInt Left = LHSResult.Val.getInt();

aaron.ballmanUnsubmitted

Not Done

... to early here instead, and it should also check isSignedOverflowDefined() (note: we should probably fix SignedOverflowBehavior in LangOptions.def to be set based on the C++ language mode instead of needing to check for C++20 specifically)

aaron.ballman: ... to early here instead, and it should also check `isSignedOverflowDefined()` (note: we…

// If LHS does not have a signed type and non-negative value // Don't warn if signed overflow is defined, then all the rest of the

// then, the behavior is undefined before C++2a. Warn about it. // diagnostics will not be triggered because the behavior is defined.

if (Left.isNegative() && !S.getLangOpts().isSignedOverflowDefined() && // Also don't warn in C++20 mode (and newer), as signed left shifts

aaron.ballmanUnsubmitted

Not Done

llvm::APSInt Left = LHSResult.Val.getInt();

- // Don't warn in C++2A mode (and newer), as signed left shifts

+ // Don't warn in C++20 mode (and newer), as signed left shifts

// always wrap and never overflow.

if (S.getLangOpts().CPlusPlus20)

return;

I think this should also be checking || isSignedOverflowDefined() -- if signed overflow is defined, then all the rest of the diagnostics shouldn't be triggered because the behavior is defined.

(Eventually, we should remove the check for CPlusPlus20 and use ONLY isSignedOverflowDefined() -- that should account for the current language mode instead of requiring us to check additional predicates. But that doesn't have to be fixed up as part of this patch.)

aaron.ballman: I think this should also be checking `|| isSignedOverflowDefined()` -- if signed overflow is…

!S.getLangOpts().CPlusPlus20) { // always wrap and never overflow.

if (S.getLangOpts().isSignedOverflowDefined() || S.getLangOpts().CPlusPlus20)

return;

// If LHS does not have a non-negative value then, the

// behavior is undefined before C++2a. Warn about it.

if (Left.isNegative()) {

aaron.ballmanUnsubmitted

Not Done

and then we can remove the check for signed overflow defined or C++20 here because we've already bailed out.

aaron.ballman: and then we can remove the check for signed overflow defined or C++20 here because we've…

erichkeaneUnsubmitted

Not Done

I think we'd want to remove the CPlusPlus20 check here, right? Signed-overflow should cover this case as well, and the check for C++20 below is no longer necessary?

erichkeane: I think we'd want to remove the CPlusPlus20 check here, right? Signed-overflow should cover…

aaron.ballmanUnsubmitted

Not Done

Then the isSignedOverflowDefined() check can be removed here.

aaron.ballman: Then the `isSignedOverflowDefined()` check can be removed here.

S.DiagRuntimeBehavior(Loc, LHS.get(), S.DiagRuntimeBehavior(Loc, LHS.get(),

S.PDiag(diag::warn_shift_lhs_negative) S.PDiag(diag::warn_shift_lhs_negative)

<< LHS.get()->getSourceRange()); << LHS.get()->getSourceRange());

return; return;

} }

llvm::APInt ResultBits = llvm::APInt ResultBits =

static_cast<llvm::APInt&>(Right) + Left.getMinSignedBits(); static_cast<llvm::APInt&>(Right) + Left.getMinSignedBits();

Show All 13 Lines static void DiagnoseBadShiftValues(Sema& S, ExprResult &LHS, ExprResult &RHS,

// turned off separately if needed. // turned off separately if needed.

if (LeftBits == ResultBits - 1) { if (LeftBits == ResultBits - 1) {

S.Diag(Loc, diag::warn_shift_result_sets_sign_bit) S.Diag(Loc, diag::warn_shift_result_sets_sign_bit)

<< HexResult << LHSType << HexResult << LHSType

<< LHS.get()->getSourceRange() << RHS.get()->getSourceRange(); << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();

return; return;

} }

S.Diag(Loc, diag::warn_shift_result_gt_typewidth) S.Diag(Loc, diag::warn_shift_result_gt_typewidth)

<< HexResult.str() << Result.getMinSignedBits() << LHSType << HexResult.str() << Result.getMinSignedBits() << LHSType

aaron.ballmanUnsubmitted

Not Done

I think this should move up...

aaron.ballman: I think this should move up...

<< Left.getBitWidth() << LHS.get()->getSourceRange() << Left.getBitWidth() << LHS.get()->getSourceRange()

<< RHS.get()->getSourceRange(); << RHS.get()->getSourceRange();

} }

/// Return the resulting type when a vector is shifted /// Return the resulting type when a vector is shifted

/// by a scalar or vector shift amount. /// by a scalar or vector shift amount.

static QualType checkVectorShift(Sema &S, ExprResult &LHS, ExprResult &RHS, static QualType checkVectorShift(Sema &S, ExprResult &LHS, ExprResult &RHS,

SourceLocation Loc, bool IsCompAssign) { SourceLocation Loc, bool IsCompAssign) {

▲ Show 20 Lines • Show All 9,246 Lines • Show Last 20 Lines

clang/test/CXX/expr/expr.const/p2-0x.cpp

Show First 20 Lines • Show All 156 Lines • ▼ Show 20 Lines	namespace UndefinedBehavior {
constexpr int shl_32 = 0 << 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type 'int' (32}}		constexpr int shl_32 = 0 << 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type 'int' (32}}
constexpr int shl_unsigned_negative = unsigned(-3) << 1; // ok		constexpr int shl_unsigned_negative = unsigned(-3) << 1; // ok
constexpr int shl_unsigned_into_sign = 1u << 31; // ok		constexpr int shl_unsigned_into_sign = 1u << 31; // ok
constexpr int shl_unsigned_overflow = 1024u << 31; // ok		constexpr int shl_unsigned_overflow = 1024u << 31; // ok
constexpr int shl_signed_negative = (-3) << 1; // cxx11-error {{constant expression}} cxx11-note {{left shift of negative value -3}}		constexpr int shl_signed_negative = (-3) << 1; // cxx11-error {{constant expression}} cxx11-note {{left shift of negative value -3}}
constexpr int shl_signed_ok = 1 << 30; // ok		constexpr int shl_signed_ok = 1 << 30; // ok
constexpr int shl_signed_into_sign = 1 << 31; // ok (DR1457)		constexpr int shl_signed_into_sign = 1 << 31; // ok (DR1457)
constexpr int shl_signed_into_sign_2 = 0x7fffffff << 1; // ok (DR1457)		constexpr int shl_signed_into_sign_2 = 0x7fffffff << 1; // ok (DR1457)
constexpr int shl_signed_off_end = 2 << 31; // cxx11-error {{constant expression}} cxx11-note {{signed left shift discards bits}} expected-warning {{signed shift result (0x100000000) requires 34 bits to represent, but 'int' only has 32 bits}}		constexpr int shl_signed_off_end = 2 << 31; // cxx11-error {{constant expression}} cxx11-note {{signed left shift discards bits}} cxx11-warning {{signed shift result (0x100000000) requires 34 bits to represent, but 'int' only has 32 bits}}
constexpr int shl_signed_off_end_2 = 0x7fffffff << 2; // cxx11-error {{constant expression}} cxx11-note {{signed left shift discards bits}} expected-warning {{signed shift result (0x1FFFFFFFC) requires 34 bits to represent, but 'int' only has 32 bits}}		constexpr int shl_signed_off_end_2 = 0x7fffffff << 2; // cxx11-error {{constant expression}} cxx11-note {{signed left shift discards bits}} cxx11-warning {{signed shift result (0x1FFFFFFFC) requires 34 bits to represent, but 'int' only has 32 bits}}
constexpr int shl_signed_overflow = 1024 << 31; // cxx11-error {{constant expression}} cxx11-note {{signed left shift discards bits}} expected-warning {{requires 43 bits to represent}}		constexpr int shl_signed_overflow = 1024 << 31; // cxx11-error {{constant expression}} cxx11-note {{signed left shift discards bits}} cxx11-warning {{requires 43 bits to represent}}
constexpr int shl_signed_ok2 = 1024 << 20; // ok		constexpr int shl_signed_ok2 = 1024 << 20; // ok

constexpr int shr_m1 = 0 >> -1; // expected-error {{constant expression}} expected-note {{negative shift count -1}}		constexpr int shr_m1 = 0 >> -1; // expected-error {{constant expression}} expected-note {{negative shift count -1}}
constexpr int shr_0 = 0 >> 0; // ok		constexpr int shr_0 = 0 >> 0; // ok
constexpr int shr_31 = 0 >> 31; // ok		constexpr int shr_31 = 0 >> 31; // ok
constexpr int shr_32 = 0 >> 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type}}		constexpr int shr_32 = 0 >> 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type}}

struct S {		struct S {
▲ Show 20 Lines • Show All 479 Lines • Show Last 20 Lines

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

Show First 20 Lines • Show All 524 Lines • ▼ Show 20 Lines	namespace Union {
}		}
}		}

namespace TwosComplementShifts {		namespace TwosComplementShifts {
using uint32 = __UINT32_TYPE__;		using uint32 = __UINT32_TYPE__;
using int32 = __INT32_TYPE__;		using int32 = __INT32_TYPE__;
static_assert(uint32(int32(0x1234) << 16) == 0x12340000);		static_assert(uint32(int32(0x1234) << 16) == 0x12340000);
static_assert(uint32(int32(0x1234) << 19) == 0x91a00000);		static_assert(uint32(int32(0x1234) << 19) == 0x91a00000);
static_assert(uint32(int32(0x1234) << 20) == 0x23400000); // expected-warning {{requires 34 bits}}		static_assert(uint32(int32(0x1234) << 20) == 0x23400000);
static_assert(uint32(int32(0x1234) << 24) == 0x34000000); // expected-warning {{requires 38 bits}}		static_assert(uint32(int32(0x1234) << 24) == 0x34000000);
static_assert(uint32(int32(-1) << 31) == 0x80000000);		static_assert(uint32(int32(-1) << 31) == 0x80000000);

static_assert(-1 >> 1 == -1);		static_assert(-1 >> 1 == -1);
static_assert(-1 >> 31 == -1);		static_assert(-1 >> 31 == -1);
static_assert(-2 >> 1 == -1);		static_assert(-2 >> 1 == -1);
static_assert(-3 >> 1 == -2);		static_assert(-3 >> 1 == -2);
static_assert(-4 >> 1 == -2);		static_assert(-4 >> 1 == -2);
}		}
▲ Show 20 Lines • Show All 933 Lines • Show Last 20 Lines

clang/test/SemaCXX/shift.cpp

Show All 35 Lines	void test() {
c <<= CHAR_BIT; // expected-warning {{shift count >= width of type}}		c <<= CHAR_BIT; // expected-warning {{shift count >= width of type}}
c >>= CHAR_BIT; // expected-warning {{shift count >= width of type}}		c >>= CHAR_BIT; // expected-warning {{shift count >= width of type}}
c <<= CHAR_BIT+1; // expected-warning {{shift count >= width of type}}		c <<= CHAR_BIT+1; // expected-warning {{shift count >= width of type}}
c >>= CHAR_BIT+1; // expected-warning {{shift count >= width of type}}		c >>= CHAR_BIT+1; // expected-warning {{shift count >= width of type}}
(void)((long)c << CHAR_BIT);		(void)((long)c << CHAR_BIT);

int i;		int i;
i = 1 << (WORD_BIT - 2);		i = 1 << (WORD_BIT - 2);
i = 2 << (WORD_BIT - 1); // expected-warning {{bits to represent, but 'int' only has}}		i = 2 << (WORD_BIT - 1); // cxx17-warning {{bits to represent, but 'int' only has}}
i = 1 << (WORD_BIT - 1); // expected-warning {{sets the sign bit of the shift expression}}		i = 1 << (WORD_BIT - 1); // cxx17-warning {{sets the sign bit of the shift expression}}
i = -1 << (WORD_BIT - 1); // cxx17-warning {{shifting a negative signed value is undefined}}		i = -1 << (WORD_BIT - 1); // cxx17-warning {{shifting a negative signed value is undefined}}
i = -1 << 0; // cxx17-warning {{shifting a negative signed value is undefined}}		i = -1 << 0; // cxx17-warning {{shifting a negative signed value is undefined}}
i = 0 << (WORD_BIT - 1);		i = 0 << (WORD_BIT - 1);
i = (char)1 << (WORD_BIT - 2);		i = (char)1 << (WORD_BIT - 2);

unsigned u;		unsigned u;
u = 1U << (WORD_BIT - 1);		u = 1U << (WORD_BIT - 1);
u = 5U << (WORD_BIT - 1);		u = 5U << (WORD_BIT - 1);

long long int lli;		long long int lli;
lli = INT_MIN << 2; // cxx17-warning {{shifting a negative signed value is undefined}} cxx2a-warning {{requires 34 bits to represent}}		lli = INT_MIN << 2; // cxx17-warning {{shifting a negative signed value is undefined}}
lli = 1LL << (sizeof(long long) * CHAR_BIT - 2);		lli = 1LL << (sizeof(long long) * CHAR_BIT - 2);
}		}

#define a 0		#define a 0
#define ashift 8		#define ashift 8
enum { b = (a << ashift) };		enum { b = (a << ashift) };

// Don't warn for negative shifts in code that is unreachable.		// Don't warn for negative shifts in code that is unreachable.
Show All 20 Lines

This is an archive of the discontinued LLVM Phabricator instance.

[Diagnostics] Fix inconsistent shift-overflow warnings in C++20ClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents

Diff 436746

clang/lib/Sema/SemaExpr.cpp

clang/test/CXX/expr/expr.const/p2-0x.cpp

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

clang/test/SemaCXX/shift.cpp

[Diagnostics] Fix inconsistent shift-overflow warnings in C++20
ClosedPublic