Page MenuHomePhabricator

[Sema] Fix an assertion failure in constant expression evaluation of calls to functions with default arguments
ClosedPublic

Authored by ahatanak on Jan 31 2018, 6:24 PM.

Details

Summary

The assertion fails when a function with a default argument that materializes a temporary is called more than once in an expression. The assertion fails in CallStackFrame::createTemporary when it searches map Temporaries using the default argument's expression (which is a MaterializeTemporaryExpr) as the key and it discovers that there is already an element with that key that has been initialized.

constexpr bool equals(const float& arg = 1.0f) {
  return arg == 1.0f;
}

constexpr bool test_default_arg() {
  return equals() && equals();
}

This patch removes the assertion and makes CallStackFrame::createTemporary reset the existing element and return it if the element is found in the map.

rdar://problem/36505742

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak created this revision.Jan 31 2018, 6:24 PM
lebedev.ri added inline comments.
test/SemaCXX/constexpr-default-arg.cpp
3 ↗(On Diff #132300)

Down the line, it won't be obvious *what* this testcase is checking.
At the very least wrap it into namespace rdar_problem_36505742 {}

Hi Akira, thanks for working on this!

lib/AST/ExprConstant.cpp
1165–1173 ↗(On Diff #132300)

I think that the problem is more subtle than this. This static assert errors (previously clang would assert) when it really it should be fine.

constexpr const int &x(const int &p = 0) { return p; }
static_assert(&x() != &x());

Because default arguments are allocated on the caller side, both the calls to x() call createTemporary for the same MaterializeTemporaryExpr in the same CallStackFrame, when really that MTE should correspond to two distinct values. This patch just hides that underlying problem by recycling the value created during the first call during the second call.

Maybe we could have a fancier key that incorporates a node on the caller side, such as the CXXDefaultArgExpr as well at the MTE, and store that fancy key in APValue::LValueBases? That would allow us generate distinct values for these MTEs, and also remember what expression originated it. What do you think about that?

There is small discussion about this problem here: https://bugs.llvm.org/show_bug.cgi?id=33140

ahatanak updated this revision to Diff 133947.Feb 12 2018, 3:06 PM
ahatanak marked 2 inline comments as done.

Address Erik's and Roman's review comments.

lib/AST/ExprConstant.cpp
1165–1173 ↗(On Diff #132300)

Thank you Erik for explaining the problem and pointing me to the PR.

I ended up using a version number that is updated every time VisitCXXDefaultArgExpr is called. I initially tried using CXXDefaultArgExpr and the void* pointer as the key, but discovered that that wouldn't fix the assertion failure when compiling the first test case in PR33140 since InitListExpr uses the same CXXConstructExpr (and hence the same CXXDefaultArgExpr) to initialize the array.

Let me know if there are other cases I haven't thought about.

test/SemaCXX/constexpr-default-arg.cpp
3 ↗(On Diff #132300)

I added comments that explain what it's trying to check.

rsmith added inline comments.Feb 12 2018, 3:36 PM
lib/AST/APValue.cpp
27 ↗(On Diff #133947)

Move this to the end so it can share space with IsNullPtr.

lib/AST/ExprConstant.cpp
589–598 ↗(On Diff #133947)

This seems extremely similar to the purpose of the existing CallIndex parameter. Have you thought about ways the two might be unified?

If there's no reasonable path to unifying the two (which I suspect there may not be), it would make more sense to me to store a current version number on the CallStackFrame rather than globally in the EvalInfo, and to restart the numbering in each new call frame. That also emphasizes something else: this version number should be kept with the CallIndex. (We could achieve that by either moving the CallIndex into the LValueBase or moving the Version out of it.)

597 ↗(On Diff #133947)

This is wrong: these scopes can nest, so you can't just reset the number to 0 when you're done. You should restore the prior number when you're done here, to divide up evaluation into CXXDefaultArgExpr scopes. (Technically, I think it would also be correct to leave the number alone when you leave one of these scopes, but only because a scope for a particular parameter's default argument can't nest within another scope for the same default argument expression -- and even that might not be true in the presence of template instantiation.)

4595 ↗(On Diff #133947)

We need the same handling for CXXDefaultInitExpr, too.

ahatanak added inline comments.Feb 12 2018, 6:39 PM
lib/AST/ExprConstant.cpp
597 ↗(On Diff #133947)

Do you mean VisitCXXDefaultArgExpr can be called the second time before the first call returns? Do you have an example code that would cause that to happen (which I can perhaps add as a test case)?

It seemed to me that that would happen only if you used a lambda expression for the default argument, but I thought the current standard doesn't allow using lambda expressions in constant expressions.

OK, I see. It's pretty easy to come up with an example.

constexpr int foo1(int a = 12) {
  return a * a;
}

constexpr int foo2(int a = foo1()) {
  return a - 12;
}
ahatanak updated this revision to Diff 134560.Feb 15 2018, 11:32 PM
ahatanak marked 3 inline comments as done.

Address review comments.

lib/AST/ExprConstant.cpp
589–598 ↗(On Diff #133947)

It couldn't come up with a way to unify CallIndex and Version. I think it's better to keep them separate because you need the CallIndex to get the frame that owns the temporary and you need Version to distinguish between two temporaries created by the same MTE.

I moved CallIndex into LValueBase as you suggested.

Thank you, this looks like a great direction.

As noted, there are a bunch of other cases that we should cover with this approach. I'm really happy about the number of related bugs we get to fix with this change.

lib/AST/ExprConstant.cpp
3186–3187 ↗(On Diff #134560)

This should take the version into account.

5236 ↗(On Diff #134560)

Hmm. We should be versioning local variables as well. Currently we'll accept invalid code such as:

constexpr int f() {
  int *p = nullptr;
  for (int k = 0; k != 2; ++k) {
    int local_var = 0;
    if (k == 0)
      p = &local_var;
    else
      return *p;
  }
}
static_assert(f() == 0);
5275–5278 ↗(On Diff #134560)

Can you combine these, so we have a single function to create a temporary and produce both an LValue denoting it and an APValue* to hold its evaluated value?

5772 ↗(On Diff #134560)

This should create a versioned temporary object.

6540 ↗(On Diff #134560)

This should create a versioned temporary object.

8031 ↗(On Diff #134560)

You already checked this above. It'd make sense to check the call index and version in the same place here, but we should only need one check for each :)

9974–9997 ↗(On Diff #134560)

These temporaries should all be versioned.

ahatanak updated this revision to Diff 136124.Feb 27 2018, 11:48 AM
ahatanak marked 7 inline comments as done.

Address review comments.

lib/AST/ExprConstant.cpp
5236 ↗(On Diff #134560)

I made changes to the constructor and destructor of ScopeRAII so that a version is pushed at the start of a new iteration. This fixes the FIXME in test/SemaCXX/constant-expression-cxx1y.cpp.

ahatanak updated this revision to Diff 139797.Mar 26 2018, 9:03 AM

Assign an uninitialized APValue to a temporary instead of removing it from the temporary map when the temporary's lifetime has ended.

rsmith accepted this revision.Apr 2 2018, 6:19 PM

I think this can be cleaned up a bit further, but I'm fine with that happening after the patch lands. Thanks!

lib/AST/ExprConstant.cpp
455–456 ↗(On Diff #139797)

Have you considered using a pair<const void*, unsigned> as the key rather than a map-of-maps? It'd be preferable to only allocate one heap node rather than two for the (hopefully common) case where there is only one version of an object.

466 ↗(On Diff #139797)

If we're using this for variables in loops too, we should generalize the name and the description to not talk about temporaries.

This revision is now accepted and ready to land.Apr 2 2018, 6:19 PM
ahatanak updated this revision to Diff 141788.Apr 9 2018, 10:16 PM
ahatanak marked an inline comment as done.
ahatanak set the repository for this revision to rC Clang.

Use pair<const void*, unsigned> as the key.

This revision was automatically updated to reflect the committed changes.