Page MenuHomePhabricator

[Modules] Fix creating fake definition data for lambdas.
ClosedPublic

Authored by vsapsai on Feb 19 2018, 4:43 PM.

Details

Summary

During reading C++ definition data for lambda we can access
CXXRecordDecl representing lambda before we finished reading the
definition data. This can happen by reading a captured variable which is
VarDecl, then reading its decl context which is CXXMethodDecl operator(),
then trying to merge redeclarable methods and accessing
enclosing CXXRecordDecl. The call stack looks roughly like

VisitCXXRecordDecl
  ReadCXXRecordDefinition
    VisitVarDecl
      VisitCXXMethodDecl
        mergeRedeclarable
          getPrimaryContextForMerging

If we add fake definition data at this point, later we'll hit the assertion

Assertion failed: (!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda definition?"), function MergeDefinitionData, file clang/lib/Serialization/ASTReaderDecl.cpp, line 1675.

The fix is to check if we are still reading real definition data and to
avoid adding a fake one in this case. Fixes PR32556.

rdar://problem/37461072

Diff Detail

Repository
rC Clang

Event Timeline

vsapsai created this revision.Feb 19 2018, 4:43 PM
aprantl added inline comments.
clang/lib/Serialization/ASTReaderDecl.cpp
2969 ↗(On Diff #134991)

Perhaps add a comment explaining what's going on in this early exit?

vsapsai added inline comments.Feb 20 2018, 10:59 AM
clang/lib/Serialization/ASTReaderDecl.cpp
2969 ↗(On Diff #134991)

While I was trying to explain this early exit, I realized one of my assumptions was incorrect. Now I'm not sure returning nullptr is the right fix, maybe assigning incomplete definition data like

D->DefinitionData = DD;
ReadCXXDefinitionData(*DD, D);

is better.

I am trying to come up with a test case showing different behaviour for these 2 different fixes. But so far nothing yet. If anybody can tell which one is correct: null context for merging or not-yet-populated definition data, that would be helpful.

vsapsai added inline comments.Feb 20 2018, 2:53 PM
clang/lib/Serialization/ASTReaderDecl.cpp
2969 ↗(On Diff #134991)

Looks like both fix approaches should work as in ASTDeclReader::ReadCXXDefinitionData we read decl only for lambda. If I'm not mistaken, for lambdas merging doesn't really make sense, so no context for merging doesn't have repercussions.

rsmith added inline comments.Feb 20 2018, 4:36 PM
clang/lib/Serialization/ASTReaderDecl.cpp
2969 ↗(On Diff #134991)

Merging for lambdas does make sense in the context of inline functions (where we can import two definitions of the same inline function from two different modules, and the definition can contain a lambda).

I think setting D->DefinitionData early would be the preferable solution. We should also disable the "all the bits must match" checks in MergeDefinitionData if one of the DefinitionData objects has IsBeingDefined set (or, ideally, defer those checks until recursive deserialization is complete).

vsapsai updated this revision to Diff 135179.Feb 20 2018, 5:36 PM
  • Set D->DefinitionData early.

We already call MergeDefinitionData after definition data deserialization is
complete. And I removed previously added IsBeingDefined, so left the "all the
bits must match" checks in MergeDefinitionData as-is.

As for testing, requirement for Canon->DefinitionData != DD check is already
enforced by existing tests.

rsmith accepted this revision.Mar 6 2018, 12:10 PM
rsmith added inline comments.
clang/lib/Serialization/ASTReaderDecl.cpp
1795 ↗(On Diff #135179)

It's a little odd that we'll temporarily have two different declarations that think they're the definition in this case, but I don't actually know of anything that'll go wrong as a result.

(This seems easy to avoid, though, by checking whether there already is a definition earlier.)

This revision is now accepted and ready to land.Mar 6 2018, 12:10 PM
vsapsai updated this revision to Diff 137299.Mar 6 2018, 5:14 PM
  • Don't set to-be-deserialized definition data if there is another decl with definition data.
vsapsai added inline comments.Mar 6 2018, 5:17 PM
clang/lib/Serialization/ASTReaderDecl.cpp
1795 ↗(On Diff #135179)

Uploaded a new patch. Is this what you had in mind by not having two different declarations that think they're the definition?

rsmith added inline comments.Mar 8 2018, 3:57 PM
clang/lib/Serialization/ASTReaderDecl.cpp
1790 ↗(On Diff #137299)

I think it would make sense to "claim" the definition more eagerly here, by also updating Canon->DefinitionData in this case, prior to reading the definition data (and in the case where we already have a canonical definition, pointing D->DefinitionData at that eagerly).

vsapsai updated this revision to Diff 137801.Mar 9 2018, 11:07 AM
  • Claim the definition data more eagerly.

Not sure that added "different" in the existing comment is actually useful.
It makes sense to me but don't know about others.

rsmith added inline comments.Mar 9 2018, 11:48 AM
clang/lib/Serialization/ASTReaderDecl.cpp
1789 ↗(On Diff #137801)

No braces around single-line if bodies, please.

1798 ↗(On Diff #137801)

Canon->DefinitionData can't be null here, so the first half of this check is redundant. If you're concerned about that, you can add an assert that it's equal to D->DefinitionData (and that both are non-null).

1800 ↗(On Diff #137801)

This store is dead.

vsapsai added inline comments.Mar 9 2018, 3:56 PM
clang/lib/Serialization/ASTReaderDecl.cpp
1811 ↗(On Diff #137801)

This store seems to be dead too. Need to spend more time in debugger to make sure that the code works the way I think it works.

vsapsai updated this revision to Diff 138059.Mar 12 2018, 11:11 AM
  • Some more cleanup. NFC.
vsapsai marked 6 inline comments as done.Mar 12 2018, 11:17 AM
vsapsai added inline comments.
clang/lib/Serialization/ASTReaderDecl.cpp
1798 ↗(On Diff #137801)

MergeDefinitionData already has assertion Canon->DefinitionData is not null so adding assertion here doesn't add safety. And I don't think it would improve readability or make it easier to understand the code.

I think we have agreement on this change. If there are no objections, I plan to commit it on Wednesday, March 21.

This revision was automatically updated to reflect the committed changes.