This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO][test] Add visibility related tests
ClosedPublic

Authored by MaskRay on Dec 8 2020, 5:35 PM.

Diff Detail

Unit TestsFailed

Event Timeline

MaskRay created this revision.Dec 8 2020, 5:35 PM
MaskRay requested review of this revision.Dec 8 2020, 5:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2020, 5:35 PM
MaskRay updated this revision to Diff 310411.Dec 8 2020, 8:06 PM

Improve tests

MaskRay updated this revision to Diff 310687.Dec 9 2020, 3:33 PM

Add a Mach-O test

MaskRay updated this revision to Diff 310705.Dec 9 2020, 4:07 PM

Check %tb.bc as well

Is this good?

Couple minor suggestions copied from review for D92900. Also, I think the macho test will fail right now since it references some symbols that don't exist there?

llvm/test/ThinLTO/X86/visibility-elf.ll
36 ↗(On Diff #310705)

Suggest making this comment a bit more explicit, e.g.:

;; Currently the visibility is not propagated onto an unimported function, because we don't have summaries for declarations.

40 ↗(On Diff #310705)

Suggest making this comment something like:

;; This can be hidden, but we cannot communicate the declaration's visibility to other modules because
;; declarations don't have summaries, and the IRLinker overrides it when importing the protected def.

llvm/test/ThinLTO/X86/visibility-macho.ll
36 ↗(On Diff #310705)

The protected_* symbols are not defined in this test.

MaskRay updated this revision to Diff 314155.Dec 30 2020, 4:30 PM
MaskRay marked 3 inline comments as done.

Address comments.

llvm/test/ThinLTO/X86/visibility-elf.ll
36 ↗(On Diff #310705)

Thanks for the suggestions!

llvm/test/ThinLTO/X86/visibility-macho.ll
36 ↗(On Diff #310705)

My bad. protected is ELF specific and does not make sense on Mach-O. I'll delete protected_*.
Ideally the backend should error on protected.

tejohnson accepted this revision.Dec 30 2020, 4:48 PM

lgtm thanks!

This revision is now accepted and ready to land.Dec 30 2020, 4:48 PM
This revision was landed with ongoing or failed builds.Dec 30 2020, 4:58 PM
This revision was automatically updated to reflect the committed changes.