[Clang] Fix warnings on bad shifts.

[chestnykh]

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

[shafik]

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]

In D141192#4033591, @shafik wrote:

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?

[tbaeder]

In D141192#4033591, @shafik wrote:

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]

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.

[chestnykh]

In D141192#4033962, @tbaeder wrote:

In D141192#4033591, @shafik wrote:

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]

Ping, can someone review my changes?

[chestnykh]

Ping

[chestnykh]

Ping

[aaron.ballman]

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–2844

These can be combined into one if with &&.

clang/lib/Sema/SemaExpr.cpp

11762–11765

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

8

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

2–3

No need for the -x c in these.

6

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.

8–9

Spurious newlines?

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

8

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

[budimirarandjelovicsyrmia]

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

[budimirarandjelovicsyrmia]

Ping.

[aaron.ballman]

In D141192#4647977, @budimirarandjelovicsyrmia wrote:

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.

[budimirarandjelovicsyrmia]

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

clang/lib/AST/ExprConstant.cpp

     // the shifted type.
     unsigned SA = (unsigned) RHS.getLimitedValue(LHS.getBitWidth()-1);
     if (SA != RHS) {
-      Info.CCEDiag(E, diag::note_constexpr_large_shift)
-        << RHS << E->getType() << LHS.getBitWidth();
+      if (Info.getLangOpts().CPlusPlus11) {
+        Info.CCEDiag(E, diag::note_constexpr_large_shift)
+            << RHS << E->getType() << LHS.getBitWidth();
+        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++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)
-      Info.CCEDiag(E, diag::note_constexpr_large_shift)
-        << RHS << E->getType() << LHS.getBitWidth();
+    if (SA != RHS) {
+      if (Info.getLangOpts().CPlusPlus11) {
+        Info.CCEDiag(E, diag::note_constexpr_large_shift)
+            << RHS << E->getType() << LHS.getBitWidth();
+        return false;
+      }
+    }
     Result = LHS >> SA;
     return true;
   }

clang/lib/Sema/SemaExpr.cpp

   return false;
 }
 
-static void DiagnoseBadShiftValues(Sema& S, ExprResult &LHS, ExprResult &RHS,
-                                   SourceLocation Loc, BinaryOperatorKind Opc,
-                                   QualType LHSType) {
+enum BadShiftValueKind {
+  BSV_Ok,
+  BSV_ShiftIsNegative,
+  BSV_ShiftLHSIsNegative,
+  BSV_ShiftSizeGTTypeWidth,
+};
+
+static BadShiftValueKind DiagnoseBadShiftValues(Sema &S, ExprResult &LHS,
+                                                ExprResult &RHS,
+                                                SourceLocation Loc,
+                                                BinaryOperatorKind Opc,
+                                                QualType LHSType) {
   // OpenCL 6.3j: shift values are effectively % word size of LHS (more defined),
   // so skip remaining warnings as we don't want to modify values within Sema.
   if (S.getLangOpts().OpenCL)
-    return;
+    return BSV_Ok;
 
   // Check right/shifter operand
   Expr::EvalResult RHSResult;
   if (RHS.get()->isValueDependent() ||
       !RHS.get()->EvaluateAsInt(RHSResult, S.Context))
-    return;
+    return BSV_Ok;
   llvm::APSInt Right = RHSResult.Val.getInt();
 
   if (Right.isNegative()) {
     S.DiagRuntimeBehavior(Loc, RHS.get(),
                           S.PDiag(diag::warn_shift_negative)
                             << RHS.get()->getSourceRange());
-    return;
+    return BSV_ShiftIsNegative;
   }
 
   QualType LHSExprType = LHS.get()->getType();

     S.DiagRuntimeBehavior(Loc, RHS.get(),
                           S.PDiag(diag::warn_shift_gt_typewidth)
                             << RHS.get()->getSourceRange());
-    return;
+    return BSV_ShiftSizeGTTypeWidth;
   }
 
   // FIXME: We probably need to handle fixed point types specially here.
   if (Opc != BO_Shl || LHSExprType->isFixedPointType())
-    return;
+    return BSV_Ok;
 
   // When left shifting an ICE which is signed, we can check for overflow which
   // according to C++ standards prior to C++2a has undefined behavior

   if (LHS.get()->isValueDependent() ||
       LHSType->hasUnsignedIntegerRepresentation() ||
       !LHS.get()->EvaluateAsInt(LHSResult, S.Context))
-    return;
+    return BSV_Ok;
   llvm::APSInt Left = LHSResult.Val.getInt();
 
   // Don't warn if signed overflow is defined, then all the rest of the

   // Also don't warn in C++20 mode (and newer), as signed left shifts
   // always wrap and never overflow.
   if (S.getLangOpts().isSignedOverflowDefined() || S.getLangOpts().CPlusPlus20)
-    return;
+    return BSV_Ok;
 
   // If LHS does not have a non-negative value then, the
   // behavior is undefined before C++2a. Warn about it.

     S.DiagRuntimeBehavior(Loc, LHS.get(),
                           S.PDiag(diag::warn_shift_lhs_negative)
                             << LHS.get()->getSourceRange());
-    return;
+    return BSV_ShiftLHSIsNegative;
   }
 
   llvm::APInt ResultBits =
       static_cast<llvm::APInt&>(Right) + Left.getMinSignedBits();
   if (LeftBits.uge(ResultBits))
-    return;
+    return BSV_Ok;
   llvm::APSInt Result = Left.extend(ResultBits.getLimitedValue());
   Result = Result.shl(Right);
 

     S.Diag(Loc, diag::warn_shift_result_sets_sign_bit)
         << HexResult << LHSType
         << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
-    return;
+
+    // We return BSV_Ok because we already done diag.
+    return BSV_Ok;
   }
 
   S.Diag(Loc, diag::warn_shift_result_gt_typewidth)
     << HexResult.str() << Result.getMinSignedBits() << LHSType
     << Left.getBitWidth() << LHS.get()->getSourceRange()
     << RHS.get()->getSourceRange();
+
+  return BSV_Ok;
 }
 
 /// Return the resulting type when a vector is shifted

       isScopedEnumerationType(RHSType)) {
     return InvalidOperands(Loc, LHS, RHS);
   }
-  DiagnoseBadShiftValues(*this, LHS, RHS, Loc, Opc, LHSType);
+
+  BadShiftValueKind BSVKind =
+      DiagnoseBadShiftValues(*this, LHS, RHS, Loc, Opc, LHSType);
+  ExpressionEvaluationContext Context = ExprEvalContexts.back().Context;
+  if (Context == ExpressionEvaluationContext::ConstantEvaluated ||
+      Context == ExpressionEvaluationContext::ImmediateFunctionContext) {
+    switch (BSVKind) {
+    case BSV_ShiftSizeGTTypeWidth:
+      if (!getLangOpts().CPlusPlus11)
+        Diag(Loc, diag::warn_shift_gt_typewidth) << RHS.get()->getSourceRange();
+      break;
+    case BSV_ShiftIsNegative:
+      Diag(Loc, diag::warn_shift_negative) << RHS.get()->getSourceRange();
+      break;
+    case BSV_ShiftLHSIsNegative:
+      Diag(Loc, diag::warn_shift_lhs_negative) << RHS.get()->getSourceRange();
+      break;
+    default:
+      break;
+    }
+  }
 
   // "The type of the result is that of the promoted left operand."
   return LHSType;

clang/test/AST/Interp/shifts.cpp

                            // FIXME: 'implicit conversion' warning missing in the new interpreter. \
                            // cxx17-warning {{shift count >= width of type}} \
                            // ref-warning {{shift count >= width of type}} \
-                           // ref-warning {{implicit conversion}} \
-                           // ref-cxx17-warning {{shift count >= width of type}} \
-                           // ref-cxx17-warning {{implicit conversion}}
+                           // ref-cxx17-warning {{shift count >= width of type}}
     c = 1 >> (unsigned)-1; // expected-warning {{shift count >= width of type}} \
                            // cxx17-warning {{shift count >= width of type}} \
                            // ref-warning {{shift count >= width of type}} \

clang/test/C/drs/dr0xx.c

   /* Demonstrate that we don't crash when left shifting a signed value; that's
    * implementation defined behavior.
    */
- _Static_assert(-1 << 1 == -2, "fail"); /* Didn't shift a zero into the "sign bit". */
+ _Static_assert(-1 << 1 == -2, "fail");  /* expected-warning {{shifting a negative signed value is undefined}} */
  _Static_assert(1 << 3 == 1u << 3u, "fail"); /* Shift of a positive signed value does sensible things. */
 }
 

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

+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wshift-count-negative %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wshift-count-negative %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+
+enum shiftof {
+    X = (1<<-29) //expected-warning {{shift count is negative}}
+};
+

clang/test/Sema/shift-count-overflow.c

+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wshift-count-overflow %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wshift-count-overflow %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+
+enum shiftof {
+    X = (1<<32) //expected-warning {{shift count >= width of type}}
+};
+

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

+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wshift-negative-value %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wshift-negative-value %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+
+enum shiftof {
+    X = (-1<<29) //expected-warning {{shifting a negative signed value is undefined}}
+};
+

clang/test/Sema/vla-2.c

 // a different codepath when we have already emitted an error.)
 
 int PotentiallyEvaluatedSizeofWarn(int n) {
-  return (int)sizeof *(0 << 32,(int(*)[n])0); // expected-warning {{left operand of comma operator has no effect}} expected-warning {{shift count >= width of type}}
+  return (int)sizeof *(0 << 32,(int(*)[n])0); /* expected-warning {{shift count >= width of type}}
+                                                 expected-warning {{left operand of comma operator has no effect}} */
 }
 
 void PotentiallyEvaluatedTypeofWarn(int n) {
-  __typeof(*(0 << 32,(int(*)[n])0)) x; // expected-warning {{left operand of comma operator has no effect}} expected-warning {{shift count >= width of type}}
+  __typeof(*(0 << 32,(int(*)[n])0)) x; /*expected-warning {{shift count >= width of type}}
+                                         expected-warning {{left operand of comma operator has no effect}} */
   (void)x;
 }
 
 void PotentiallyEvaluatedArrayBoundWarn(int n) {
-  (void)*(int(*)[(0 << 32,n)])0; // expected-warning {{left operand of comma operator has no effect}}
+  (void)*(int(*)[(0 << 32,n)])0; /* expected-warning {{shift count >= width of type}}
+                                    expected-warning {{left operand of comma operator has no effect}} */
 }

clang/test/SemaCXX/cxx2a-explicit-bool.cpp

 template<int a>
 struct A {
 // expected-note@-1+ {{candidate constructor}}
-  explicit(1 << a)
+  explicit(1 << a) // expected-warning {{shift count is negative}}
 // expected-note@-1 {{negative shift count -1}}
 // expected-error@-2 {{explicit specifier argument is not a constant expression}}
   A(int);