Page MenuHomePhabricator

[ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)
ClosedPublic

Authored by shafik on Apr 12 2020, 10:18 PM.

Details

Summary

In ImportContext(…) we may call into CompleteDecl(…) which if FromRecrord is not defined will start the definition of a ToRecord but from what I can tell at least one of the paths though here don't ensure we complete the definition.
For a RecordDecl this can be problematic since this means we won’t import base classes and we won’t have any of the methods or types we inherit from these bases.

One such path is through VisitTypedefNameDecl(…) which is exercised by the reproducer.

For LLDB expression parsing this results in expressions failing but sometimes succeeding in subsequent attempts e.g.:

(lldb) p BB->dump()
error: no member named 'dump' in 'B'
(lldb) p BB->dump()
(lldb)

This happens because the first time through FromRecord is not defined but in subsequent attempts through it may be.

Diff Detail

Event Timeline

shafik created this revision.Apr 12 2020, 10:18 PM

I am open to different approaches to this problem, this is opening attempt at fixing it but I may be missing other interactions.

AFAICT starting the definition of ToRecord the way CompleteDecl(…) does when we know FromRecord is not defined is just broken for the RecordDecl case if we have bases.

It took me a lot of time to come up with this reproducer from a real life issue debugging llc.

teemperor requested changes to this revision.Apr 13 2020, 4:39 AM

From what I understand the whole idea here is to just ask the external AST source to complete the record before we import them? If yes, then this seems like the right idea to me.

Also this seems to be testable via a Clang unit test, so I think this patch should have one.

But otherwise this LGTM. Thanks for figuring this out!

clang/lib/AST/ASTImporter.cpp
8198

I'm not sure how it can be that ASTImporter::CompleteDecl starts but never finishes the decl as it does both things at once unconditionally?

TD->startDefinition();
TD->setCompleteDefinition(true);

FWIW, this whole comment could just be Ask the external source if this is actually a complete record that just hasn't been completed yet or something like this. I think if we reach this point then it makes a complete sense to ask the external source for more info. The explanation about the base classes seems to be just a smaller detail of the whole picture here.

8205

I assume we should check here that FromRecord is now a complete definition before trying to import it's definition?

lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
26

You need to make this a "//%" as otherwise this test fails (which it does right now).

This revision now requires changes to proceed.Apr 13 2020, 4:39 AM
shafik updated this revision to Diff 257552.Apr 14 2020, 4:24 PM
shafik marked 4 inline comments as done.

Addressing comments

Looks okay to me (other than the redundant import code that I have a comment about).

Also this seems to be testable via a Clang unit test, so I think this patch should have one.

Yeah, would be nice to have a Clang unit test. I believe we have the infrastructure for that in place. E.g. in LLDBLookupTest we have an lldb specific setup, that could be a good starting point.

clang/lib/AST/ASTImporter.cpp
8198

Ask the external source if this is actually a complete record that just hasn't been completed yet

FWIW this seems to be a recurring pattern, so maybe it would be worth to do this not just with RecordDecls but with other kind of decls. E.g. an EnumDecl could have an external source and not completed, couldn't it?

8204

We could merge this if with the else if at line 8164, because their body is exactly the same.
But to do that, we should move the external storage query and type completion above the enclosing if (above line 8162 and just after line 8161).

shafik updated this revision to Diff 258084.Apr 16 2020, 10:28 AM
  • Added unit test
  • Modified condition for defining FromRecord to whether it has an ExternalSource
shafik marked an inline comment as done.Apr 16 2020, 10:30 AM
shafik added inline comments.
clang/lib/AST/ASTImporter.cpp
8198

TD->setCompleteDefinition(true); just sets a flag but it does not import the bases, which it can not do since From is not defined. See ASTNodeImporter::ImportDefinition(RecordDecl *From, RecordDecl *To, ImportDefinitionKind Kind) which does this after it does To->startDefinition();. So we can't really consider the definition finished if we don't do this.

Do you have a better way of describing this situation?

I think it is important to describe why we don't use CompleteDecl since the other cases do.

8198

This is definitely more costly, for the EnumDecl case I don't see how we could get in a similar situation since we don't have inheritance. I looked over the EnumDecl members and I don't see anything that would require it be defined.

Thanks for the test Shafik, that is pretty self explanatory!

clang/lib/AST/ASTImporter.cpp
8178

What if we did this a bit differently? We could simply complete the From type if it is not completed, before getting into ImportDefinition.

if (ToRecord->isCompleteDefinition())
  return ToDC;
auto *FromRecord = cast<RecordDecl>(FromDC);
if (FromRecord->hasExternalLexicalStorage() &&
          !FromRecord->isCompleteDefinition())
        FromRecord->getASTContext().getExternalSource()->CompleteType(
            FromRecord);

This way we could get rid of the redundant calls of ImportDefinition. As we have now a call for the case when we don't have external storage and for the case when we have.

Beside Gabors comment I think I'm happy with this. Thanks!

clang/unittests/AST/ASTImporterTest.cpp
5948

"is completed" -> "it completed"

5963

You can remove this as you changed the check in the ASTImporter to only check for the existence of an ExternalASTSource.

shafik updated this revision to Diff 259110.Apr 21 2020, 2:54 PM
shafik marked 2 inline comments as done.
  • Simplifying the changes in ASTImporter::ImportContext
  • Removing DC->setHasExternalLexicalStorage(true); from unit test since we are checking getExternalSource()
martong accepted this revision.Apr 22 2020, 7:44 AM

LGTM! Thanks!

a_sidorin accepted this revision.Apr 22 2020, 1:33 PM

Hello Shafik!
The patch looks good, I only have a few stylish nits.

clang/unittests/AST/ASTImporterTest.cpp
5973

Context (starting with capital).

5989

Err (starting with capital).

teemperor accepted this revision.Apr 23 2020, 8:01 AM
This revision is now accepted and ready to land.Apr 23 2020, 8:01 AM
shafik updated this revision to Diff 259703.Apr 23 2020, 1:53 PM
shafik marked 2 inline comments as done.

Capitalization fixes.

This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 23 2020, 3:48 PM
abhinavgaba added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
5974

Overloading CompleteType(TagDecl*) is causing a warning:
virtual void clang::ExternalASTSource::CompleteType(clang::ObjCInterfaceDecl*)’ was hidden [-Woverloaded-virtual]

One way to get rid of it would be to add this here :

private:
  using ExternalASTSource::CompleteType;