Page MenuHomePhabricator

[ASTImporter] Fix LLDB lookup in transparent ctx and with ext src
ClosedPublic

Authored by martong on Apr 30 2019, 12:19 PM.

Details

Summary

With LLDB we use localUncachedLookup(), however, that fails to find
Decls when a transparent context is involved and the given DC has
external lexical storage. The solution is to use noload_lookup, which
works well with transparent contexts. But, we cannot use only the
noload_lookup since the slow case of localUncachedLookup is still needed
in some other cases.

These other cases are handled in ASTImporterLookupTable, but we cannot
use that with LLDB since that traverses through the AST which initiates
the load of external decls again via DC::decls().

We must avoid loading external decls during the import becuase
ExternalASTSource is implemented with ASTImporter, so external loads
during import results in uncontrolled and faulty import.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Apr 30 2019, 12:19 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
martong marked 2 inline comments as done.Apr 30 2019, 12:29 PM
martong added inline comments.
lldb/packages/Python/lldbsuite/test/lang/c/modules/main.c
8 ↗(On Diff #197409)

In TestCmodules.py we have import Darwin and then expr *fopen("/dev/zero", "w"). This imports the name FILE from the Darwin module. Then when we want expr *myFile, the two different definition of the FILE structs collides.
This happens because now the lookup finds the existing definition for the FILE in the TU associated with the expression evaluator, and that comes from the Darwin module.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
640 ↗(On Diff #197409)

We must remove the below addDeclInternal call, because that may be called from another
addDeclInternal:

6  clang::CXXConstructorDecl::isDefaultConstructor() const + 87
7  clang::CXXRecordDecl::addedMember(clang::Decl*) + 803
8  clang::DeclContext::addHiddenDecl(clang::Decl*) + 341
9  clang::DeclContext::addDeclInternal(clang::Decl*) + 29
10 lldb_private::ClangASTSource::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&) + 3917
11 lldb_private::ClangASTSource::ClangASTSourceProxy::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&) + 102
12 clang::ExternalASTSource::FindExternalLexicalDecls(clang::DeclContext const*, llvm::SmallVectorImpl<clang::Decl*>&) + 101
13 clang::DeclContext::LoadLexicalDeclsFromExternalStorage() const + 269
14 clang::DeclContext::buildLookup() + 336
15 clang::DeclContext::makeDeclVisibleInContextWithFlags(clang::NamedDecl*, bool, bool) + 403
16 clang::DeclContext::addDeclInternal(clang::Decl*) + 92
17 clang::ASTNodeImporter::VisitFieldDecl(clang::FieldDecl*) + 3851

... and at the second call of addDeclInternal we may add an incomplete Decl (CXXConstructorDecl above).
We must always avoid redundant work in lldb which is already handled in Clang::ASTImporter.

a_sidorin accepted this revision.May 4 2019, 2:31 AM

Hello Gabor!
This looks good to me, but let's wait for LLDB guys to take a look at the patch. Thanks!

This revision is now accepted and ready to land.May 4 2019, 2:31 AM
ormris removed a subscriber: ormris.May 6 2019, 9:11 AM

I ran check-lldb and I hit one regression in TestFormatters.py, part of what I am seeing is as follows:

AssertionError: False is not True : FileCheck'ing result of `expression --show-types -- *(new foo(47))`
Error output:
error: no matching constructor for initialization of 'foo'
candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const foo' for 1st argument
candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'foo' for 1st argument
clang/lib/AST/ASTImporter.cpp
1818 ↗(On Diff #197409)

se -> so?

lldb/source/Symbol/ClangASTImporter.cpp
922 ↗(On Diff #197409)

I am going to say that the logging change is an excellent additions and stands alone from this change. Although I realize the test depends on this new feature. It makes sense to add the logging in a separate PR.

I also say this b/c I found a regression and it would be nice to get the logging in while we resolve the regression.

martong updated this revision to Diff 200031.May 17 2019, 6:25 AM
  • Rebase to master
  • Rebase to D62061
martong marked an inline comment as done.May 17 2019, 6:27 AM
martong added inline comments.
lldb/source/Symbol/ClangASTImporter.cpp
922 ↗(On Diff #197409)

Ok, I have created a new patch for logging (https://reviews.llvm.org/D62061).
I made this patch to be the child of that and rebased to that.

martong updated this revision to Diff 200033.May 17 2019, 6:29 AM
  • se -> so
martong marked 2 inline comments as done.May 17 2019, 6:31 AM
martong updated this revision to Diff 205093.Jun 17 2019, 8:56 AM
  • Fix regression of TestFormatters.py

About the regression of TestFormatters.py: I realized that the problem is about the wrong implementation of the ExternalASTSource interface.
In the implementation of FindExternalLexicalDecls of this interface, we simply ignored those cases when the given predicate (passed as a param) is false. When that happens, that means we still have some more external decls which should be dug up by the upcoming calls of DeclContext::lookup. The fix is about to indicate this.

@shafik @jingham This is a polite Ping.

@martong Sorry for the delay, feel free to ping me in the future about these patches. I'll review them ASAP now that I'm back in office, so these delay's hopefully won't happen again.

I tried applying this patch and it seems it needs to be rebased. I would do it myself, but I'm not entirely sure how to rebase the changes to ASTNodeImporter::ImportDefinition. It seems we got rid of To->completeDefinition();, so not sure if the code that this patch adds is still in the right place.

martong updated this revision to Diff 209832.Jul 15 2019, 6:04 AM
  • Rebase to master

@martong Sorry for the delay, feel free to ping me in the future about these patches. I'll review them ASAP now that I'm back in office, so these delay's hopefully won't happen again.

I tried applying this patch and it seems it needs to be rebased. I would do it myself, but I'm not entirely sure how to rebase the changes to ASTNodeImporter::ImportDefinition. It seems we got rid of To->completeDefinition();, so not sure if the code that this patch adds is still in the right place.

Raphael,

Thank you for looking into this. I've rebased to master and updated the patch.

This LGTM now but I will wait for @teemperor to take a look at it.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
620 ↗(On Diff #209832)

I think a comment on the predicate would be helpful as well.

teemperor requested changes to this revision.Jul 16 2019, 2:20 AM

On my system clang-format has some complaints, so I think you need to rerun clang-format. Probably caused by the rebasing.

I have some minor comments about the TestAST.py (see the inline comments), but otherwise this patch LGTM.

lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
1 ↗(On Diff #209832)

This comment (and the name of the test class) are copy-pasted from TestCModules and should be updated. If we can merge the two test cases (see the inline comment below), then this would be fixed as a side-effect.

57 ↗(On Diff #209832)

I think this test has a good chance to fail once someone renamed/removes/changes this internal struct (especially if it's currently abstracted with a typedef). Would it be possible to just make a minimal module with open and FILE/__sFILE instead? If it's too much trouble, then I'm also fine with merging this as-is (as this regression is easy to fix).

67 ↗(On Diff #209832)

It seems that this is the only different in the test compared to TestCModules.py. Would it be possible to just add this logging/checking to TestCModules.py as it's anyway testing something very similar?

This revision now requires changes to proceed.Jul 16 2019, 2:20 AM
martong updated this revision to Diff 210281.Jul 17 2019, 3:04 AM
martong marked 5 inline comments as done.
  • Applied clang-format on lldb parts (this changed two lines)
  • Added a comment for predicate
  • Merged the test into TestCModules.py
martong marked an inline comment as done.Jul 17 2019, 3:04 AM
martong added inline comments.
lldb/packages/Python/lldbsuite/test/lang/c/ast/TestAST.py
57 ↗(On Diff #209832)

I'd rather keep this as-is, because I don't have enough confidence and experience with c modules (and with macOS).

67 ↗(On Diff #209832)

Ok, I have removed the TestAST.py and added the extra logging and the check into TestCModules.py.

teemperor accepted this revision.Jul 17 2019, 4:46 AM

LGTM, thanks a lot for fixing this!

This revision is now accepted and ready to land.Jul 17 2019, 4:46 AM

Thank you guys for the review!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2019, 6:48 AM

Jenkins looks okay: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31157/ .The build is red but the the previous run has the very same failing test case:
expression_command/weak_symbols/TestWeakSymbols.py