Page MenuHomePhabricator

[lldb][modern-type-lookup] No longer import temporary declarations into the persistent AST
ClosedPublic

Authored by teemperor on Oct 2 2019, 2:38 AM.

Details

Summary

As we figured out in D67803, importing declarations from a temporary ASTContext that were originally from a persistent ASTContext
causes a bunch of duplicated declarations where we end up having declarations in the target AST that have no associated ASTImporter that
can complete them.

I haven't figured out how/if we can solve this in the current way we do things in LLDB, but in the modern-type-lookup this is solvable
as we have a saner architecture with the ExternalASTMerger. As we can (hopefully) make modern-type-lookup the default mode in the future,
I would say we try fixing this issue here. As we don't use the hack that was reinstated in D67803 during modern-type-lookup, the test case for this
is essentially just printing any kind of container in std:: as we would otherwise run into the issue that required a hack like D67803.

What this patch is doing in essence is that instead of importing a declaration from a temporary ASTContext, we instead check if the
declaration originally came from a persistent ASTContext (e.g. the debug information) and we directly import from there. The ExternalASTMerger
is already connected with ASTImporters to these different sources, so this patch is essentially just two parts:

  1. Mark our temporary ASTContext/ImporterSource as temporary when we import from the expression AST.
  2. If the ExternalASTMerger sees we import from the expression AST, instead of trying to import these temporary declarations, check if we

can instead import from the persistent ASTContext that is already connected. This ensures that all records from the persistent source actually
come from the persistent source and are minimally imported in a way that allows them to be completed later on in the target AST.

The next step is to run the ASTImporter for these temporary expressions with the MinimalImport mode disabled, but that's a follow up patch.

This patch fixes most test failures with modern-type-lookup enabled by default (down to 73 failing tests, which includes the 22 import-std-module tests
which need special treatment).

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Oct 2 2019, 2:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2019, 2:38 AM

With [modern-type-lookup], we completely evade the use of ASTImporterDelegate? That would be a wonderful thing to use only the the ExternalASTMerger on a long term...

... a bunch of duplicated declarations

This popped a few thoughts into my head:
One way to discover duplicated declarations is to set the ODRViolation handling strategy to "conservative" in clang::ASTImporter. However, minimal imported decls will be structurally non-equivalent with those which are imported in normal mode. Thus, it may make sense only if the normal import mode is used exclusively.

The next step is to run the ASTImporter for these temporary expressions with the MinimalImport mode disabled, but that's a follow up patch.

👍 I think this is a very good direction.

clang/include/clang/AST/ExternalASTMerger.h
120 ↗(On Diff #222773)

typo: functions -> function
FromD -> D ?

clang/lib/AST/DeclBase.cpp
95 ↗(On Diff #222773)

Left over debug printout?

clang/lib/AST/ExternalASTMerger.cpp
108 ↗(On Diff #222773)

I was thinking about that ToOrigin could be in the SharedState, but then I realized it is better to have it here, because the merger is the one which encapsulates the handling of several importers.

141 ↗(On Diff #222773)

This line/sentence is hard to parse for me.
I get this part:

This way the ExternalASTMerger can safely do a minimal import that doesn't cause having minimally imported declarations in the target ASTContext.

But this I don't:

that no connected ASTImporter has imported.
149 ↗(On Diff #222773)

👍
I like this approach.

aprantl added inline comments.Oct 2 2019, 9:03 AM
clang/include/clang/AST/ExternalASTMerger.h
87 ↗(On Diff #222773)

super-nit: there's supposed to be a comma before and after "i.e." and "e.g." :-)

clang/lib/AST/ExternalASTMerger.cpp
171 ↗(On Diff #222773)

&& "error message" ... this will help with debugging in a couple of months!

shafik added inline comments.Oct 2 2019, 10:12 AM
clang/include/clang/AST/ExternalASTMerger.h
95 ↗(On Diff #222773)

Identifiers the begin with an underscore and followed by a capital letter are reserved see lex.name/p3.1:

Each identifier that contains a double underscore __ or begins with an underscore followed by an uppercase letter is reserved to the implementation for any use.

clang/lib/AST/ExternalASTMerger.cpp
141 ↗(On Diff #222773)

I agree this is hard to parse, although I am happy that you are explaining the rationale.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
183 ↗(On Diff #222773)

So we are removing this b/c we are now doing a minimal import from the temporary source and then in the next patch you will change that?

teemperor marked 11 inline comments as done.Oct 3 2019, 6:55 AM
teemperor added inline comments.
clang/lib/AST/DeclBase.cpp
95 ↗(On Diff #222773)

Whoops, thanks.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
183 ↗(On Diff #222773)

We always did a minimal import here and the completer just tried to turn this a non-minimal import by iterating over the AST and then copying a bunch of declarations over. It currently only seems to cause errors in the existing tests. Also, we anyway don't test the feature it's actually supposed to implement anywhere in LLLDB as this is a bug in the original LLDB. So this just removes this code until we can properly implement and test it (which will hopefully just be turning off the MinimalImport when importing from temporary sources).

teemperor updated this revision to Diff 223007.Oct 3 2019, 6:57 AM
teemperor marked 2 inline comments as done.
  • Addressed feedback (Thanks Gabor, Adrian & Shafik!)
martong accepted this revision.Oct 3 2019, 9:12 AM

LGTM!

This revision is now accepted and ready to land.Oct 3 2019, 9:12 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2019, 1:25 AM