This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Note when we've actually encountered a failure in ExprConstant, and take that into account when looking up objects.
ClosedPublic

Authored by george.burgess.iv on Mar 28 2016, 9:27 PM.

Details

Summary

This patch aims to fix a bug encountered by compiling code with C++14.

Specifically, when evaluating p in __builtin_object_size(p, n), we start speculatively evaluating. Currently, clang pretends that side-effects have always occurred when we start speculatively evaluating. This prevents us from reading any locals in C++14. So, the following code compiles fine in C++11 mode, but fails to compile with C++14:

void foo(char *p) __attribute__((enable_if(__builtin_object_size(p, 0) != -1, "")));

void runFoo() {
  char buf[5];
  foo(buf);
}

This patch makes ExprConstant note when we've actually failed (read: may have an unmodeled side-effect), and makes us act as conservatively before if a failure happened. If a failure hasn't happened, we can be more aggressive.

Diff Detail

Repository
rL LLVM

Event Timeline

george.burgess.iv retitled this revision from to [Sema] Note when we've actually encountered a failure in ExprConstant, and take that into account when looking up objects..
george.burgess.iv updated this object.
george.burgess.iv added a reviewer: rsmith.
george.burgess.iv added a subscriber: cfe-commits.
test/SemaCXX/constant-expression-cxx1y.cpp
182 ↗(On Diff #51872)

Aside: This test was fixed because we no longer give up on trying to look up foo, because we know an unmodeled side-effect hasn't happened.

rsmith added inline comments.Mar 29 2016, 4:48 PM
include/clang/AST/Expr.h
541–546 ↗(On Diff #51872)

I don't think this provides enough guarantees to be useful as part of the external EvalStatus results. Can you sink this into EvalInfo? Alternatively, can you merge this with HasSideEffects? I don't think we need to maintain a distinction between the two different ways we can get an evaluation result after skipping a subexpression.

george.burgess.iv marked an inline comment as done.

Addressed all feedback.

include/clang/AST/Expr.h
541–546 ↗(On Diff #51872)

I don't think this provides enough guarantees to be useful as part of the external EvalStatus results. Can you sink this into EvalInfo

WFM

Alternatively, can you merge this with HasSideEffects.

The result is a bit more subtle (because some things depend on HasSideEffects being accurate, and not being set whenever we fail), but sure.

rsmith added inline comments.Apr 4 2016, 11:31 AM
lib/AST/ExprConstant.cpp
782 ↗(On Diff #52167)

unchange -> unchanged

784–785 ↗(On Diff #52167)

I think this is the right behavior regardless of the current expectations of any particular caller. After this patch:

  • HasSideEffects == false indicates that the evaluation (if evaluation was successful, then up to the current point, otherwise up to the point where evaluation failed) has definitely not skipped any side-effects
  • HasSideEffects == true means nothing is known about side-effects (in particular, it does not guarantee that a side-effect has definitely happened, just that one /might/ have happened)

(If any caller expects these flags to mean something else, they are wrong -- and note that HasSideEffects is meaningless if evaluation fails.) Therefore, if we transition from an "evaluation has failed" to an "evaluation was successful" state, we must set HasSideEffects to true to model the subexpressions we might have skipped. And if we just unwind (because keepEvaluatingAfterFailure() returns false), then we don't need to set HasSideEffects at all.

Perhaps you could say something like this:

"Failure when evaluating some expression often means there is some subexpression whose evaluation was skipped. Therefore, (because we don't track whether we skipped an expression when unwinding after an evaluation failure) every evaluation failure that bubbles up from a subexpression implies that a side-effect has potentially happened. We skip setting the HasSideEffects flag to true until we decide to continue evaluating after that point, which happens here."

853–854 ↗(On Diff #52167)

I think this comment is now redundant / incorrect.

2643 ↗(On Diff #52167)

IsSpeculativelyEvaluating is always false. Presumably it should be set to true in the SpeculativeEvaluationRAII constructor. (Please also add a testcase to catch this...)

3280 ↗(On Diff #52167)

Can we give this function a name that reads more naturally at the call site? It's locally unclear what this condition is testing. Maybe keep the old name?

4074–4076 ↗(On Diff #52167)

These RAII objects are cheap; maybe just create two of them regardless?

4104–4105 ↗(On Diff #52167)

I think this should be an if rather than an assert. If we hit something that's known to be non-constant while evaluating the condition of a ?:, we should bail out of evaluation in checkingPotentialConstantExpression mode. keepEvaluatingAfterFailure should return false in that case (even though it doesn't do so today).

7374–7375 ↗(On Diff #52167)

The side-effect here should be delayed until after we've evaluated the left and right hand sides of the assignment.

george.burgess.iv marked 7 inline comments as done.

Addressed feedback

lib/AST/ExprConstant.cpp
7103 ↗(On Diff #52834)

I'm happy to make SpeculativeEvalRAII a type that optionally restores state, if you think that would be better. That way, we don't need to duplicate this logic...

Also, I can't figure out how to test this, because it's only ever run after HasSideEffects = true, and the only thing that (I think) checks for if we're speculatively evaluating *also* fails in exactly the same way if we have side effects.

853–854 ↗(On Diff #52167)

My bad :)

3280 ↗(On Diff #52167)

The best alternative I can come up with is noteFailureAndGetShouldContinueEvaluating(), which occupies 44 characters.

...I think I'll go back to the old name.

4104–4105 ↗(On Diff #52167)

I'm not sure that I fully understand what you mean by this. Are you saying that we (ultimately) only want to check the arms of a conditional when we're evaluating for overflow?

Either way, took a stab at fixing it.

rsmith accepted this revision.May 24 2016, 6:26 PM
rsmith edited edge metadata.

LGTM with or without the SpeculativeEvaluationRAII refactor (which is ultimately pre-existing duplication).

Please commit the rename of keepEvaluatingAfterFailure -> noteFailure separately.

lib/AST/ExprConstant.cpp
4104–4105 ↗(On Diff #52834)

The relevant case is when we're checking the body of a constexpr function to see whether it could possibly be a constant expression. In that case, what we do next should depend on whether we've already found something that is known to never be constant. If so (which is indicated by us having a diagnostic), we should bail out. If not, we should check both arms of the conditional operator and report a problem if neither of them can be constant.

I think the upshot of all of that is that keepEvaluatingAfterFailure should return false for EM_PotentialConstantExpression* if we have already produced a diagnostic. But that's not relevant for this particular patch, so let's leave it as you have it for now. We can revisit this.

7103 ↗(On Diff #52834)

Yes, if we can reuse SpeculativeEvaluationRAII here, or otherwise factor out the duplication, that would be better. Also I think you're probably right that this is not observable right now.

test/SemaCXX/constant-expression-cxx1y.cpp
942 ↗(On Diff #52834)

Unused?

This revision is now accepted and ready to land.May 24 2016, 6:26 PM
This revision was automatically updated to reflect the committed changes.