Page MenuHomePhabricator

[AST] Enhance the const expression evaluator to support error-dependent exprs.
ClosedPublic

Authored by hokein on Jul 27 2020, 5:07 AM.

Details

Summary

Fix a crash when evaluating a constexpr function which contains
recovery-exprs. https://bugs.llvm.org/show_bug.cgi?id=46837

Would be nice to have constant expression evaluator support general template
value-dependent expressions, but it requires more work.

This patch is a good start I think, to handle the error-only
value-dependent expressions.

Diff Detail

Event Timeline

hokein created this revision.Jul 27 2020, 5:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2020, 5:07 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Based on richard's suggestions, that seems about right. However I believe all the asserts should have a message to go along with them as we typicaly do.

rsmith added inline comments.Jul 30 2020, 4:32 PM
clang/lib/AST/ExprConstant.cpp
5110

This should not result in failure when checking for a potential constant expression; if an expression contains errors we should treat it as being potentially constant, because (depending on how the error is resolved) it could be constant. We should noteSideEffect() here and continue unless it asks us to stop.

I guess I'm thinking of something like:

if (E->isValueDependent()) {
  if (E->noteSideEffect())
    return ESR_Succeeded;
  assert(E->containsErrors() && "non-value-dependent expression should never reach valid value-dependent expression");
  return ESR_Failed;
}

It might also be interesting to produce a diagnostic saying that evaluation failed due to a prior error on the ESR_Failed path; we could special-case that diagnostic when we diagnose expressions that are supposed to be constant but aren't in order to fully suppress the follow-on diagnostic.

(Same comment on all the below instances of this same pattern; adding a helper function for the above seems warranted.)

Based on richard's suggestions, that seems about right. However I believe all the asserts should have a message to go along with them as we typicaly do.

In this case, the asserts are enforcing AST invariants, so I think they're OK without diagnostics.

hokein updated this revision to Diff 282916.Aug 4 2020, 7:19 AM

address review comments:

  • treat contains-errors expression as being potentially constant;
  • handle the value-dependent expressions for EvaluateInPlace;
  • remove the bailing out for constructor initializers that contains errors;
hokein marked an inline comment as done.Aug 4 2020, 7:39 AM
hokein added inline comments.
clang/lib/AST/ExprConstant.cpp
5187

The if stmt (the same to while, for) is tricky -- if the condition is value-dependent, then we don't know which branch we should run into.

  • returning a ESR_Succeeded may lead to a bogus diagnostic ("reach the end of the function");
  • returning a ESR_Failed may lead to a bogus diagnostic ("never produce a constexpr");

I guess what we want is to stop the evaluation, and indicate that we hit a value-dependent expression and we don't know how to evaluate it, but still treat the constexpr function as potential constexpr (but no extra diagnostics being emitted), but the current EvalStmtResult is not sufficient, maybe we need a new enum.

friendly ping @rsmith, the patch is ready for another round of review. Would be nice to have it in clang11.

friendly ping @rsmith.

rsmith added inline comments.Sep 8 2020, 1:04 PM
clang/lib/AST/ExprConstant.cpp
4803

The initializer might also be null because the variable is type-dependent (eg, X<contains_errors> x;), in which case assuming default-init is wrong. We should check for that and treat it like a value-dependent initializer.

5187

We should only produce the "never produce a constant expression" diagnostic if we also produce a CCEDiag/FFDiag, so I think returning ESR_Failed here should work.

5187

Should this check live in EvaluateCond instead?

5273–5274

Missing value dependence check here.

8246

How does this happen? I would expect the dependence of the subexpression to be reflected in the MaterializeTemporaryExpr.

8796

How does this happen?

9509

How does this happen? Do we not propagate value-dependence from initializers to new-expressions?

hokein updated this revision to Diff 290754.Sep 9 2020, 9:13 AM
hokein marked an inline comment as done.

address review comments.

hokein added inline comments.Sep 9 2020, 9:16 AM
clang/lib/AST/ExprConstant.cpp
4803

I think you're right, added the check here, but unfortunately I didn't see noticeable behavior changes, and didn't manage to construct one (I confirmed that decltype(invalid()) x; will trigger this path).

5187

Should this check live in EvaluateCond instead?

there is a subtle difference -- doing it outside (not matter succeed or fail) will stop any execution of the following code, while doing it in EvaluateCond may continue the remaining code path if it succeeds, which may introduce bogus diagnostics, e.g. emitting a diagnostic constexpr evaluation hit maximum step limit; possible infinite loop? for the following example.

constexpr int foo() {
  while (invalid()) {} 
  return 1;
}

Another point is that -- looks like if we just return false if it is a value-dependent expr in EvaluateCond, it improves diagnostics ( function never produces and reached end of diagnostics are suppressed) for the following example

 constexpr int test4() { // expected-error {{constexpr function never produces a constant expression}}
  if (invalid()) // expected-error {{use of undeclared identifier}}
    return 1;
  else
    return 0;
}  // expected-note {{control reached end of constexpr function}}
5273–5274

ah, right. Added one test.

8796

I think this is a similar escaped case to CXXNewExpr as well.

9509

this happens in the following case (during the constructor call), the dependence-bits of CXXNewExpr are correct -- it is value-dependent, instantiation-dependent, contains-errors.

However, we run into this codepath because the CXXDefaultInitExpr doesn't have any dependence-bits being set, in fact, it is always none, I think this is a bug, fixing in https://reviews.llvm.org/D87382.

With that fix, we don't need this check anymore (the same to other two places).

struct A {
  int* p = new int(invalid());
}
constexpr int test2() {
  A a;
  return 1;
}

@rsmith this patch should be ready for another round of review.

friendly ping @rsmith :)

There are a couple of cases where you're returning EvalStmtResult from a function with a bool return type, that I'd like to see fixed before this lands.

All the other comments are directed towards producing more precise behavior when evaluating a function containing errors / value-dependent constructs. I don't think there's any need to block fixing the crasher here on improving the diagnostics, so I'm happy if you ignore these and commit as-is (other than fixing the return type issue), but I think we'll want to look at these diagnostic improvements at some point.

clang/lib/AST/ExprConstant.cpp
4790

I think it would be slightly better if the dependent cases in this function returned noteSideEffect() instead of false.

4818–4830

This function should bail out as soon as it sees an error. Otherwise, I think

constexpr void f() { int a = error(), b = a; }

... will to produce a hard error when evaluating b, because its initializer appears to read from an uninitialized variable.

4832

Given the number of places below that seem to need to branch on the return value rather than returning it directly, and the use from contexts other than statement evaluation, I think it might be better for this function to return bool.

5071

If this returns ESR_Succeeded, we should keep going rather than returning that value. Otherwise we'll do the wrong thing for cases like:

constexpr bool f() {
  for (int n = 0; ; ++n, error()) { if (n == 1) return true; }
  throw "bad";
}

... because evaluation of the for loop (but not the enclosing function) will terminate at the value-dependent increment expression, so we'll think that all evaluations of this function unconditionally throw.

5235

If the condition is value-dependent, I think we should return ESR_Failed here: we don't know whether to keep going or terminate the loop, and either answer will be wrong in some cases.

5275

As above, I think we need to keep going on ESR_Succeeded here.

5331

We should return ESR_Failed unconditionally here, because we don't know which statement to execute next.

5359

As above, I think we need to keep going on ESR_Succeeded here.

6148–6150

This is returning an EvalStmtResult from a function with a bool return type. It'd also be a bit more precise to keep going after a value-dependent expression here, so that we can diagnose non-constant constructs in the body after a typo in the mem-initializer list.

6292–6293

Similarly, it'd be a bit more precise to keep going if EvaluateDependentExpr says it succeeded.

rsmith accepted this revision.Oct 21 2020, 11:44 AM

(Accepted subject to previous comment.)

This revision is now accepted and ready to land.Oct 21 2020, 11:44 AM
hokein updated this revision to Diff 306015.Wed, Nov 18, 2:23 AM
hokein marked 9 inline comments as done.

rebase and address review comments.

There are a couple of cases where you're returning EvalStmtResult from a function with a bool return type, that I'd like to see fixed before this lands.

All the other comments are directed towards producing more precise behavior when evaluating a function containing errors / value-dependent constructs. I don't think there's any need to block fixing the crasher here on improving the diagnostics, so I'm happy if you ignore these and commit as-is (other than fixing the return type issue), but I think we'll want to look at these diagnostic improvements at some point.

thanks for the useful comments, I think I have addressed most of them and added more tests.

This revision was landed with ongoing or failed builds.Wed, Nov 18, 6:48 AM
This revision was automatically updated to reflect the committed changes.