[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

@rsmith what about this place for check?

[xbolva00]

Ping :)

[Rakete1111]

lib/Sema/SemaType.cpp

2232

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

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

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

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.

lib/Sema/SemaType.cpp

 /// \param Entity The name of the entity that involves the array
 /// type, if known.
 ///
 /// \returns A suitable array type, if there are no errors. Otherwise,
 /// returns a NULL type.
 QualType Sema::BuildArrayType(QualType T, ArrayType::ArraySizeModifier ASM,
                               Expr *ArraySize, unsigned Quals,
                               SourceRange Brackets, DeclarationName Entity) {
-
+  ArraySize->EvaluateForOverflow(Context);
   SourceLocation Loc = Brackets.getBegin();
   if (getLangOpts().CPlusPlus) {
     // C++ [dcl.array]p1:
     //   T is called the array element type; this type shall not be a reference
     //   type, the (possibly cv-qualified) type void, a function type or an
     //   abstract class type.
     //
     // C++ [dcl.array]p3:
if (ConstVal == 0) {
       // Is the array too large?
       unsigned ActiveSizeBits
         = ConstantArrayType::getNumAddressingBits(Context, T, ConstVal);
       if (ActiveSizeBits > ConstantArrayType::getMaxSizeBits(Context)) {
         Diag(ArraySize->getBeginLoc(), diag::err_array_too_large)
             << ConstVal.toString(10) << ArraySize->getSourceRange();
         return QualType();
       }
     }
 
     T = Context.getConstantArrayType(T, ConstVal, ASM, Quals);
   }
 
   // OpenCL v1.2 s6.9.d: variable length arrays are not supported.
   if (getLangOpts().OpenCL && T->isVariableArrayType()) {
     Diag(Loc, diag::err_opencl_vla);
     return QualType();
   }

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'}}
 };