[Diagnostics] Check for integer overflow in array size expressions
ClosedPublic

Authored by xbolva00 on Mon, Oct 1, 2:27 PM.

Diff Detail

Repository
rL LLVM
xbolva00 created this revision.Mon, Oct 1, 2:27 PM

Seems it crashes with test suite. Looking at it.

xbolva00 updated this revision to Diff 167835.Mon, Oct 1, 2:44 PM

Fixed crash

rsmith added a comment.Mon, Oct 1, 2:52 PM

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 updated this revision to Diff 167840.Mon, Oct 1, 3:07 PM
  • Move code as suggested
rsmith added a comment.Mon, Oct 1, 3:13 PM

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.)

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 updated this revision to Diff 167857.Mon, Oct 1, 5:10 PM
xbolva00 updated this revision to Diff 167858.

fixed extra new line

xbolva00 added inline comments.Tue, Oct 2, 4:22 AM
lib/Sema/SemaType.cpp
2231 ↗(On Diff #167858)

@rsmith what about this place for check?

Rakete1111 added inline comments.
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 added inline comments.Sat, Oct 6, 10:58 AM
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 updated this revision to Diff 168575.Sat, Oct 6, 11:03 AM
  • Use Sema::CheckForIntOverflow
xbolva00 added inline comments.Sat, Oct 6, 11:07 AM
lib/Sema/SemaType.cpp
2232 ↗(On Diff #167858)

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

@Rakete1111 plesse take a look :)

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

Thanks!

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

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

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 added inline comments.Thu, Oct 11, 3:45 PM
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 updated this revision to Diff 169407.Fri, Oct 12, 8:16 AM
  • check for overflow when evaluating
xbolva00 updated this revision to Diff 169408.Fri, Oct 12, 8:17 AM
xbolva00 updated this revision to Diff 169409.
  • Undo extra newline

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

xbolva00 added a comment.EditedFri, Oct 12, 8:35 AM

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 :/

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.

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

Hopefully, this solution would be now acceptable :)

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.

This comment was removed by xbolva00.
rsmith accepted this revision.Thu, Oct 18, 1:03 PM

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

This revision is now accepted and ready to land.Thu, Oct 18, 1:03 PM
This revision was automatically updated to reflect the committed changes.