This revision was automatically updated to reflect the committed changes.
Closed by commit rL344759: [Diagnostics] Check for integer overflow in array
size expressions (authored by xbolva00, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
We can leave the cleanup of the expression evaluator to another change.
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
xbolva00 added a comment.
Reping :)
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
xbolva00 added a comment.
Thanks. ... So anything else to be done? Or it is ok?
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rsmith added a comment.
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 added a comment.
That's fine :) btw, thanks for review comments!
Hopefully, this solution would be now acceptable :)
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Rakete added a comment.
In https://reviews.llvm.org/D52750#1263466, @xbolva00 wrote:
> In https://reviews.llvm.org/D52750#1263461, @Rakete 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
>
xbolva00 added a comment.
In https://reviews.llvm.org/D52750#1263461, @Rakete wrote:
> This doesn't produce a warning in C++11 and up.
But see Richard's comment: https://reviews.llvm.org/D52750#1251759
https://reviews.llvm.org/D52750
___
Rakete added a comment.
This doesn't produce a warning in C++11 and up.
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
xbolva00 updated this revision to Diff 169409.
xbolva00 added a comment.
- Undo extra newline
https://reviews.llvm.org/D52750
Files:
include/clang/AST/Expr.h
lib/AST/ExprConstant.cpp
lib/Sema/SemaExpr.cpp
test/Sema/integer-overflow.c
Index: test/Sema/integer-overflow.c
xbolva00 updated this revision to Diff 169408.
https://reviews.llvm.org/D52750
Files:
include/clang/AST/Expr.h
lib/AST/ExprConstant.cpp
lib/Sema/SemaExpr.cpp
test/Sema/integer-overflow.c
Index: test/Sema/integer-overflow.c
xbolva00 updated this revision to Diff 169407.
xbolva00 added a comment.
- check for overflow when evaluating
https://reviews.llvm.org/D52750
Files:
include/clang/AST/Expr.h
lib/AST/ExprConstant.cpp
lib/Sema/SemaExpr.cpp
test/Sema/integer-overflow.c
Index:
rsmith added inline comments.
Comment at: lib/Sema/SemaType.cpp:2231
}
+
+ if (isa(ArraySize))
xbolva00 wrote:
> @rsmith what about this place for check?
This is still evaluating the expression twice. To avoid that, you need to
change the existing
xbolva00 added a comment.
In https://reviews.llvm.org/D52750#1261746, @Rakete 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
Rakete added a comment.
Nah, you don't even need to call `EvaluateForOverflow` I believe. :) Have a
look overflow evaluation is done.
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
xbolva00 added a comment.
Thanks!
But I think I need to change EvaluateForOverflow method to return bool to
indicate overflowing.
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Rakete added a comment.
The array size is still evaluated twice. Try to incorporate the check in
`Sema::VerifyIntegerConstantExpression`.
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
xbolva00 added a comment.
@Rakete plesse take a look :)
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
xbolva00 added inline comments.
Comment at: lib/Sema/SemaType.cpp:2232
+
+ if (isa(ArraySize))
+ArraySize->EvaluateForOverflow(Context);
xbolva00 wrote:
> Rakete wrote:
> > What's up with this statement? Why is it needed? This won't handle
> >
xbolva00 updated this revision to Diff 168575.
xbolva00 added a comment.
- Use Sema::CheckForIntOverflow
https://reviews.llvm.org/D52750
Files:
lib/Sema/SemaType.cpp
test/Sema/integer-overflow.c
Index: test/Sema/integer-overflow.c
xbolva00 added inline comments.
Comment at: lib/Sema/SemaType.cpp:2232
+
+ if (isa(ArraySize))
+ArraySize->EvaluateForOverflow(Context);
Rakete wrote:
> What's up with this statement? Why is it needed? This won't handle overflows
> for unary
Rakete added inline comments.
Comment at: lib/Sema/SemaType.cpp:2232
+
+ if (isa(ArraySize))
+ArraySize->EvaluateForOverflow(Context);
What's up with this statement? Why is it needed? This won't handle overflows
for unary expression for
xbolva00 added a comment.
Ping :)
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
xbolva00 added inline comments.
Comment at: lib/Sema/SemaType.cpp:2231
}
+
+ if (isa(ArraySize))
@rsmith what about this place for check?
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
xbolva00 updated this revision to Diff 167858.
xbolva00 added a comment.
fixed extra new line
https://reviews.llvm.org/D52750
Files:
lib/Sema/SemaType.cpp
test/Sema/integer-overflow.c
Index: test/Sema/integer-overflow.c
===
xbolva00 added a comment.
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.
https://reviews.llvm.org/D52750
xbolva00 updated this revision to Diff 167857.
https://reviews.llvm.org/D52750
Files:
lib/Sema/SemaExpr.cpp
lib/Sema/SemaType.cpp
test/Sema/integer-overflow.c
Index: test/Sema/integer-overflow.c
===
---
rsmith added a comment.
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
xbolva00 updated this revision to Diff 167840.
xbolva00 added a comment.
- Move code as suggested
https://reviews.llvm.org/D52750
Files:
lib/Sema/SemaType.cpp
test/Sema/integer-overflow.c
Index: test/Sema/integer-overflow.c
rsmith added a comment.
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.
https://reviews.llvm.org/D52750
xbolva00 updated this revision to Diff 167835.
xbolva00 added a comment.
Fixed crash
https://reviews.llvm.org/D52750
Files:
lib/Sema/SemaType.cpp
test/Sema/integer-overflow.c
Index: test/Sema/integer-overflow.c
===
---
xbolva00 added a comment.
Seems it crashes with test suite. Looking at it.
Repository:
rC Clang
https://reviews.llvm.org/D52750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
xbolva00 created this revision.
xbolva00 added a reviewer: rsmith.
Herald added a subscriber: cfe-commits.
Fixes PR27439
Repository:
rC Clang
https://reviews.llvm.org/D52750
Files:
lib/Sema/SemaType.cpp
test/Sema/integer-overflow.c
Index: test/Sema/integer-overflow.c
33 matches
Mail list logo
