This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Fix warnings on bad shifts.
Needs ReviewPublic

Authored by chestnykh on Jan 7 2023, 2:55 AM.

Details

Summary

Those warnings were gone because DiagRuntimeBehavior
doesn't emit warnings in ConstantEvaluated case.
In c++11 mode it were gone because we have to return false
to make them visible.
PR#59863

Diff Detail

Event Timeline

chestnykh created this revision.Jan 7 2023, 2:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2023, 2:55 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
chestnykh requested review of this revision.Jan 7 2023, 2:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2023, 2:55 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
shafik added a subscriber: shafik.Jan 7 2023, 10:03 AM

So it looks like in handleIntIntBinOp we do hit this code:

unsigned SA = (unsigned) RHS.getLimitedValue(LHS.getBitWidth()-1);
 if (SA != RHS) {
   Info.CCEDiag(E, diag::note_constexpr_large_shift)
     << RHS << E->getType() << LHS.getBitWidth();

So maybe we should figure out why we decide not to emit this diagnostic and fix it there.

chestnykh added a comment.EditedJan 7 2023, 10:17 AM

So it looks like in handleIntIntBinOp we do hit this code:

unsigned SA = (unsigned) RHS.getLimitedValue(LHS.getBitWidth()-1);
 if (SA != RHS) {
   Info.CCEDiag(E, diag::note_constexpr_large_shift)
     << RHS << E->getType() << LHS.getBitWidth();

So maybe we should figure out why we decide not to emit this diagnostic and fix it there.

In the comment above:
"// C++11 [expr.shift]p1: Shift width must be less than the bit width of the shifted type."
Maybe only since C++11 there is the restriction to shift width? Don't you know the standard about it?

So it looks like in handleIntIntBinOp we do hit this code:

unsigned SA = (unsigned) RHS.getLimitedValue(LHS.getBitWidth()-1);
 if (SA != RHS) {
   Info.CCEDiag(E, diag::note_constexpr_large_shift)
     << RHS << E->getType() << LHS.getBitWidth();

So maybe we should figure out why we decide not to emit this diagnostic and fix it there.

We return true anyway and so the diagnostics are not printed. This is reached from a call to VerifyIntegerConstantExpression() IIRC.

chestnykh updated this revision to Diff 487168.Jan 8 2023, 5:17 AM
chestnykh edited the summary of this revision. (Show Details)
chestnykh updated this revision to Diff 487174.Jan 8 2023, 5:33 AM
chestnykh retitled this revision from [Clang] Add warnings on bad shifts inside enums. to [Clang] Fix warnings on bad shifts..

I've modified handleIntIntBinOp so now it emits message about bad shift value.
DiagnoseBadShiftValues changes are also needed because handleIntIntBinOp cant properly cover for example the case we are not in c++11 mode.

So it looks like in handleIntIntBinOp we do hit this code:

unsigned SA = (unsigned) RHS.getLimitedValue(LHS.getBitWidth()-1);
 if (SA != RHS) {
   Info.CCEDiag(E, diag::note_constexpr_large_shift)
     << RHS << E->getType() << LHS.getBitWidth();

So maybe we should figure out why we decide not to emit this diagnostic and fix it there.

We return true anyway and so the diagnostics are not printed. This is reached from a call to VerifyIntegerConstantExpression() IIRC.

Yes, but it seems there is no proper way to handle, for example, C code compiling in this place, so i left DiagnoseBadShiftValues changes.

chestnykh updated this revision to Diff 487183.Jan 8 2023, 7:04 AM
chestnykh updated this revision to Diff 487200.Jan 8 2023, 9:05 AM
Allen added a subscriber: Allen.Jan 8 2023, 5:11 PM

Ping, can someone review my changes?

Sorry for the delay in reviewing, I was out for standards meetings last week and couldn't get to this one. For future patches, can you be sure to upload them with more diff context (-U9999 is what I usually use).

You should also add a release note for the fix.

clang/lib/AST/ExprConstant.cpp
2837–2843

These can be combined into one if with &&.

clang/lib/Sema/SemaExpr.cpp
11762–11763

because we already done diag -> because we already emitted the diagnostic

12023–12025

You can use ExprEvalContexts.back().isConstantEvaluated() instead.

clang/test/C/drs/dr0xx.c
429

The new comment doesn't match the comment above. I'm not certain when WG14 changed the behavior from implementation-defined to undefined, but we should capture that information in the test because the DR has been superseded. I suspect it was from the twos complement changes in C2x though, so that'd be a good place to start looking.

clang/test/Sema/shift-count-negative.c
7

When the shift is a negative *constant* value (as opposed to a signed variable type), should we diagnose this as an error rather than a warning? (Perhaps worth thinking about as a follow-up patch?)

clang/test/Sema/shift-count-overflow.c
1–2

No need for the -x c in these.

5

This depends on the target architecture, doesn't it? You might want to add some target triples to the RUN line to ensure you've picked targets where this property holds.

7–8

Spurious newlines?

clang/test/Sema/shift-negative-value.c
7

This is another case we might want to consider if we can turn into an error (in a follow-up patch).

@chestnykh Are you still working on this issue? If not, I'm interested to continue working on issue.

Ping.

Given the lack of response, I think it's safe to assume this patch has been abandoned by the author. I'd recommend starting a new patch review for it on GitHub rather than trying to commandeer this review out of Phab.

Created Pull Request. Addressed all coments except ones addressing target architecture and considering emitting error instead of warning.