[Diagnostics] Check for integer overflow in array size expressions

[xbolva00]

Summary

Fixes PR27439

[xbolva00]

Seems it crashes with test suite. Looking at it.

[xbolva00]

Fixed crash

[rsmith]

We're going to try evaluating the array size anyway (in isArraySizeVLA). It would be much better to produce the overflow warnings as part of that evaluation rather than adding an extra evaluation step to produce them.

[xbolva00]

• Move code as suggested

[rsmith]

This is still evaluating the expression twice. To evaluate it only once, you'll need to sink the checks into Sema::VerifyIntegerConstantExpression's call to EvaluateKnownConstInt. (That should also remove the redundant diagnostics noise in the language modes where signed overflow renders an expression non-constant.)

[xbolva00]

I have not found a way yet since EvaluateForOverflow returns nothing so I don't know how to check whether there was overflow or not.

Uploaded alternative patch which passes test suite and has no double warning issue.

[xbolva00]

fixed extra new line

[xbolva00]

lib/Sema/SemaType.cpp

2231 ↗

(On Diff #167858)

@rsmith what about this place for check?

[xbolva00]

Ping :)

[Rakete1111]

lib/Sema/SemaType.cpp

2232 ↗

(On Diff #167858)

What's up with this statement? Why is it needed? This won't handle overflows for unary expression for example.

[xbolva00]

lib/Sema/SemaType.cpp

2232 ↗

(On Diff #167858)

Ok, I should use Sema::CheckForIntOverflow

But anyway, CheckForIntOverflow does not care about UnaryOperator either currently :)

[xbolva00]

• Use Sema::CheckForIntOverflow

[xbolva00]

lib/Sema/SemaType.cpp

2232 ↗

(On Diff #167858)

Anyway, the negative array size is handled a few lines above.

[xbolva00]

@Rakete1111 plesse take a look :)

[Rakete1111]

The array size is still evaluated twice. Try to incorporate the check in Sema::VerifyIntegerConstantExpression.

[xbolva00]

Thanks!

But I think I need to change EvaluateForOverflow method to return bool to indicate overflowing.

[Rakete1111]

Nah, you don't even need to call EvaluateForOverflow I believe. :) Have a look overflow evaluation is done.

[xbolva00]

In D52750#1261746, @Rakete1111 wrote:

Nah, you don't even need to call EvaluateForOverflow I believe. :) Have a look overflow evaluation is done.

Well..

if (!getLangOpts().CPlusPlus11 && E->isIntegerConstantExpr(Context)) {
  if (Result) {
    *Result = E->EvaluateKnownConstInt(Context); // here

}

and
char a[2147483642 * 3];

Result->getBitWidth() reports 32. I don't know how to detect there if overflow or not :/ I have already spent some time to solve this, but still no good solution. Possibly I would abandon this patch.

[rsmith]

lib/Sema/SemaType.cpp

2231 ↗

(On Diff #167858)

This is still evaluating the expression twice. To avoid that, you need to change the existing code that calls the evaluator to ask it to produce overflow warnings as a side-effect rather than adding a new call to CheckForIntOverflow.

[xbolva00]

• check for overflow when evaluating

[xbolva00]

• Undo extra newline

[Rakete1111]

This doesn't produce a warning in C++11 and up.

[xbolva00]

In D52750#1263461, @Rakete1111 wrote:

This doesn't produce a warning in C++11 and up.

But see Richard's comment: https://reviews.llvm.org/D52750#125175 so I am not sure :/

[Rakete1111]

In D52750#1263466, @xbolva00 wrote:

In D52750#1263461, @Rakete1111 wrote:

This doesn't produce a warning in C++11 and up.

But see Richard's comment: https://reviews.llvm.org/D52750#125175 so I am not sure :/

I guess I can see why it makes sense to suppress the warning in those cases. Sorry.

[xbolva00]

That's fine :) btw, thanks for review comments!

Hopefully, this solution would be now acceptable :)

[rsmith]

Yeah, this looks better.

Though this really highlights that the "warn on undefined behavior" behavior of the constant evaluator should be orthogonal to the evaluation mode... we don't really need/want to evaluate past a side-effect or non-constant expression here.

[xbolva00]

This comment was removed by xbolva00.

[xbolva00]

Reping :)

[rsmith]

We can leave the cleanup of the expression evaluator to another change.

cfe/trunk/include/clang/AST/Expr.h

public:
 
   /// Determine whether this expression involves a call to any function
   /// that is not trivial.
   bool hasNonTrivialCall(const ASTContext &Ctx) const;
 
   /// EvaluateKnownConstInt - Call EvaluateAsRValue and return the folded
   /// integer. This must be called on an expression that constant folds to an
   /// integer.
-  llvm::APSInt EvaluateKnownConstInt(const ASTContext &Ctx,
-                    SmallVectorImpl<PartialDiagnosticAt> *Diag = nullptr) const;
+  llvm::APSInt EvaluateKnownConstInt(
+      const ASTContext &Ctx,
+      SmallVectorImpl<PartialDiagnosticAt> *Diag = nullptr) const;
+
+  llvm::APSInt EvaluateKnownConstIntCheckOverflow(
+      const ASTContext &Ctx,
 
   void EvaluateForOverflow(const ASTContext &Ctx) const;
 
   /// EvaluateAsLValue - Evaluate an expression to see if we can fold it to an
   /// lvalue with link time known address, with no side-effects.
   bool EvaluateAsLValue(EvalResult &Result, const ASTContext &Ctx) const;
 
   /// EvaluateAsInitializer - Evaluate an expression as if it were the

cfe/trunk/lib/AST/ExprConstant.cpp

APSInt Expr::EvaluateKnownConstInt(const ASTContext &Ctx,
   bool Result = EvaluateAsRValue(EvalResult, Ctx);
   (void)Result;
   assert(Result && "Could not evaluate expression");
   assert(EvalResult.Val.isInt() && "Expression did not evaluate to integer");
 
   return EvalResult.Val.getInt();
 }
 
+APSInt Expr::EvaluateKnownConstIntCheckOverflow(
+    const ASTContext &Ctx, SmallVectorImpl<PartialDiagnosticAt> *Diag) const {
+  EvalResult EvalResult;
+  EvalResult.Diag = Diag;
+  EvalInfo Info(Ctx, EvalResult, EvalInfo::EM_EvaluateForOverflow);
+  bool Result = ::EvaluateAsRValue(Info, this, EvalResult.Val);
+  (void)Result;
+  assert(Result && "Could not evaluate expression");
+  assert(EvalResult.Val.isInt() && "Expression did not evaluate to integer");
+
+  return EvalResult.Val.getInt();
+}
+
 void Expr::EvaluateForOverflow(const ASTContext &Ctx) const {
   bool IsConst;
   EvalResult EvalResult;
   if (!FastEvaluateAsRValue(this, EvalResult, Ctx, IsConst)) {
     EvalInfo Info(Ctx, EvalResult, EvalInfo::EM_EvaluateForOverflow);
     (void)::EvaluateAsRValue(Info, this, EvalResult.Val);
   }
 }

cfe/trunk/lib/Sema/SemaExpr.cpp

if (!Diagnoser.Suppress)
       Diagnoser.diagnoseNotICE(*this, DiagLoc, E->getSourceRange());
     return ExprError();
   }
 
   // Circumvent ICE checking in C++11 to avoid evaluating the expression twice
   // in the non-ICE case.
   if (!getLangOpts().CPlusPlus11 && E->isIntegerConstantExpr(Context)) {
     if (Result)
-      *Result = E->EvaluateKnownConstInt(Context);
+      *Result = E->EvaluateKnownConstIntCheckOverflow(Context);
     return E;
   }
 
   Expr::EvalResult EvalResult;
   SmallVector<PartialDiagnosticAt, 8> Notes;
   EvalResult.Diag = &Notes;
 
   // Try to evaluate the expression, and produce diagnostics explaining why it's

cfe/trunk/test/Sema/integer-overflow.c

 
 // expected-warning@+2 {{overflow in expression; result is 536870912 with type 'int'}}
   uint64_t (*f0_ptr)(uint64_t) = &f0;
   (void)(*f0_ptr)(4608 * 1024 * 1024);
 
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   (void)f2(0, f0(4608 * 1024 * 1024));
 }
+void check_integer_overflows_in_array_size() {
+  int arr[4608 * 1024 * 1024]; // expected-warning {{overflow in expression; result is 536870912 with type 'int'}}
+}
 
 struct s {
   unsigned x;
   unsigned y;
 } s = {
   .y = 5,
   .x = 4 * 1024 * 1024 * 1024  // expected-warning {{overflow in expression; result is 0 with type 'int'}}
 };