This is an archive of the discontinued LLVM Phabricator instance.

Add support for remembering origins to ExternalASTMerger
AbandonedPublic

Authored by spyffe on Aug 10 2017, 12:51 PM.

Details

Summary

ExternalASTMerger has hitherto relied on being able to look up any Decl through its named DeclContext chain. This works for many cases, but causes problems for function-local structs, which cannot be looked up in their containing FunctionDecl. An example case is

void f() {
  { struct S { int a; }; }
  { struct S { bool b; }; }
}

It is not possible to lookup either of the two Ses individually (or even to provide enough information to disambiguate) after parsing is over; and there is typically no need to, since they are invisible to the outside world.

However, ExternalASTMerger needs to be able to complete either S on demand. This led to an XFAIL on test/Import/local-struct, which this patch removes. The way the patch works is:

  • It defines a new data structure, ExternalASTMerger::OriginMap, which clients are expected to maintain (default-constructing if the origin does not have an ExternalASTMerger servicing it)
  • As DeclContexts are imported, if they cannot be looked up by name they are placed in the OriginMap. This allows ExternalASTMerger to complete them later if necessary.
  • As DeclContexts are imported from an origin that already has its own OriginMap, the origins are forwarded – but only for those DeclContexts that are actually used. This keeps the amount of stored data minimal.

Diff Detail

Event Timeline

spyffe created this revision.Aug 10 2017, 12:51 PM
spyffe updated this revision to Diff 110660.Aug 10 2017, 5:45 PM

Updated the patch to eliminate the need to pass in an OriginMap for the target. The OriginMap can be extracted afterward, and merged by other algorithms if necessary.

spyffe updated this revision to Diff 111053.Aug 14 2017, 12:53 PM

Updated the patch to make it possible for external clients to record the origins of types manually, a requirement for LLDB.

spyffe updated this revision to Diff 111369.Aug 16 2017, 9:39 AM

Added more tests, and added an option to clang-import-test to actually use the origin-forwarding code. Also fixed some errors in CompleteType that were caught by these new tests.

spyffe updated this revision to Diff 112438.Aug 23 2017, 1:59 PM

Removed unnecessary code and improved code coverage. Also made a few operational changes:

  • In initial lookup, accept forward declarations. They can be completed later.
  • When doing lookup in a DeclContext, only look at source DeclContexts of the same type.
  • Assume that only Decls with hasExternalLexicalStorage are actually completed, saving runtime and simplifying the code. Fixed one case in Sema where this rule was violated.
spyffe updated this revision to Diff 115716.Sep 18 2017, 2:05 PM
spyffe added a reviewer: bruno.

Added DEBUG_PRINTF code to enable introspection into what's happening.

bruno edited edge metadata.Sep 20 2017, 4:50 PM

Hi Sean,

Thanks for making this functionality available in clang. Comments below.

Can you please add cfe-commits to the review for the next round?

include/clang/AST/ExternalASTMerger.h
32–33

Please add a comment explaining what Origins means and what it's used for.

44

Cool, I like these abstraction more than ImporterEndpoint. Please add comments explaining what ImporterTarget and ImporterSource represent in this context.

lib/AST/ExternalASTMerger.cpp
21

This seems brittle. Maybe hide this logic behind a variable that can be setup through clang-importer-test invocation by passing down a flag? Another solution is to hide users on dump functions, along the line of AST dumpers, etc.

35–36

Please add a comment explaining what do you mean by canonicalization here.

69

Why is it best to rely on the Origins map?

82–121

Add some comments on why you need a LazyASTImporter, so users know what to expect. Is LazyASTImporter expected to have other users outside this in the future? I wonder if it's worth to have this in ExternalASTMerger.h or any other place.

96

Please add a comment explaining what this method does.

98

This doesn't seem to be clang-formatted.

103

Same here!

106

Same here! And all the other places.

123

Odd identation here.

138

merger -> Merger

167

auto SourceDC -> auto *SourceDC

228

auto T1 -> auto *T1

229

auto T2 -> auto *T2, please also fix other places with * or & where appropriate.

249

What's the deal with this commented out snippet?

260

This is an example of where having a variable for conditionally printing extra information becomes handy.

283

No need for curly braces here.

spyffe updated this revision to Diff 116405.Sep 22 2017, 2:30 PM
spyffe marked 18 inline comments as done.
spyffe added a subscriber: cfe-commits.

Updated to reflect Bruno's suggestions.

  • Commented ExternalASTMerger.h extensively.
  • Refactored the log into a pluggable raw_ostream that defaults to llvm::nulls().
  • Added comments per Bruno's requests
  • Fixed some indentation
  • Fixed some variable names and auto [*&] s
  • Removed some commented-out code
  • Cleaned up some curly braces
bruno added a comment.Sep 22 2017, 3:07 PM

Thanks for the additional docs! More comments below.

lib/AST/ExternalASTMerger.cpp
169

Can you conditionalize the logging?

179

Same here.

187

Same here.

258

Same here.

287

Ditto

293

Ditto

tools/clang-import-test/clang-import-test.cpp
232

Can you also add a brief documentation for this one?

242

No need for the else here.

256

No need for curly braces here.

326

No need for the else here.

333

No need for curly braces here.

343

No need for the else here.

354–361

No need for braces in this entire block

365

No need for the else here.

spyffe abandoned this revision.Sep 22 2017, 4:46 PM
spyffe marked 13 inline comments as done.

At @bruno 's request, abandoning this revision in favor of the updated https://reviews.llvm.org/D38208.
That revision has all the changes requested.