This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Check visibility/linkage of functions and variables
ClosedPublic

Authored by martong on Jan 25 2019, 4:49 AM.

Details

Summary

During import of a global variable with external visibility the lookup
will find variables (with the same name) but with static visibility.
Clearly, we cannot put them into the same redecl chain. The same is
true in case of functions. In this fix we filter the lookup results and
consider only those which have the same visibility as the decl we
currently import.

We consider two decls in two anonymous namsepaces to have the same
visibility only if they are imported from the very same translation
unit.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Jan 25 2019, 4:49 AM
martong updated this revision to Diff 183523.Jan 25 2019, 4:58 AM
  • Remove old style import in case of FoundArray
balazske added inline comments.
unittests/AST/ASTImporterTest.cpp
2523 ↗(On Diff #183523)

Is this dump needed? (The test should not write unnecessary text output. But debug statements can be leaved in the test, possibly in comment.)

shafik added inline comments.Jan 25 2019, 3:09 PM
lib/AST/ASTImporter.cpp
2954 ↗(On Diff #183523)

We don't really need an else here and it would be more like an early exit which is what llvm style guide suggests.

In the same vein I would do:

if (Importer.GetFromTU(Found) != From->getTranslationUnitDecl())
   return false;

so a second early exit and remove a level of nesting at the same time.

unittests/AST/ASTImporterTest.cpp
65 ↗(On Diff #183523)

Are these changes directly related to the visibility change? There is a lot of noise that is not obviously related to the description in the PR.

If not maybe it should be a separate PR?

martong updated this revision to Diff 183826.Jan 28 2019, 4:38 AM
martong marked 5 inline comments as done.
  • Remove dumpDeclContext() call
  • Remove superfluous else
martong marked an inline comment as done.Jan 28 2019, 4:41 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
2954 ↗(On Diff #183523)

It's a good catch, thanks.

unittests/AST/ASTImporterTest.cpp
65 ↗(On Diff #183523)

Actually, it will be more precise to create first a patch which contains the tests related refactor, I agree. I'll do that and this patch will be dependent upon that.

2523 ↗(On Diff #183523)

Thanks for catching this, good point.

martong updated this revision to Diff 183834.Jan 28 2019, 5:30 AM

I have created a separate patch for the test related refactor, this patch now
depends on that patch and contains merely the visibility related change and
their tests.

martong updated this revision to Diff 184697.Feb 1 2019, 2:10 AM
  • Move hunks into this patch from parent patch
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 2:10 AM
shafik accepted this revision.Feb 12 2019, 10:02 AM

I ran check-lldb locally and it looks good.

This revision is now accepted and ready to land.Feb 12 2019, 10:02 AM
martong updated this revision to Diff 186802.Feb 14 2019, 1:26 AM

Rebase to master(trunk)

@shafik Thanks for the review!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2019, 5:08 AM