[LTO] Prevent devirtualization for symbols dynamically exported

[tejohnson]

Summary

Identify dynamically exported symbols (--export-dynamic[-symbol=],
--dynamic-list=, or definitions needed to preempt shared objects) and
prevent their LTO visibility from being upgraded.
This helps avoid use of whole program devirtualization when there may
be overrides in dynamic libraries.

[MaskRay]

I'll need to study what whole program visibility does (e.g. I need to read http://lists.llvm.org/pipermail/llvm-dev/2019-December/137543.html "RFC: Safe Whole Program Devirtualization Enablement").
My gut feeling is that: --export-dynamic is not sufficient to capture whether symbols are exported.
When linking an executable,

• Symbols matched by a --dynamic-list pattern are exported to the dynamic symbol table
• Symbols matched by a --export-dynamic-symbol pattern exported to the dynamic symbol table
• Symbols defined in the executable which are referenced by a shared object are exported.

If the usage of --export-dynamic means whether the mode should apply to synthesized new symbols, I think having a mode referring to --export-dynamic makes sense.

[tejohnson]

In D91583#2398638, @MaskRay wrote:

I'll need to study what whole program visibility does (e.g. I need to read http://lists.llvm.org/pipermail/llvm-dev/2019-December/137543.html "RFC: Safe Whole Program Devirtualization Enablement").
My gut feeling is that: --export-dynamic is not sufficient to capture whether symbols are exported.
When linking an executable,

• Symbols matched by a --dynamic-list pattern are exported to the dynamic symbol table
• Symbols matched by a --export-dynamic-symbol pattern exported to the dynamic symbol table
• Symbols defined in the executable which are referenced by a shared object are exported.

If the usage of --export-dynamic means whether the mode should apply to synthesized new symbols, I think having a mode referring to --export-dynamic makes sense.

Not just synthesized new symbols. It determines whether we can convert the LTO visibility of vtables to internal visibility. The intention was that it could be applied when you are doing static linking of a binary (without shared libraries on the link line). The problem is that --export-dynamic suggests that the binary might be intending to dlopen some shared libraries that weren't necessarily linked against. It is true that --export-dynamic-symbol and --dynamic-list can have this effect, but in a much more limited fashion. The goal is to be able to apply --lto-whole-program-visibility widely based on the linking mode, but then automatically disable it in situations where it is likely problematic.

[MaskRay]

Hi, I am still learning the feature and I've just played a bit with the test. I've a couple of questions:

• The --export-dynamic usage in the test seems a bit confusing. Are the VisibleToRegularObj bits of these _ZTV* symbols the important matter and --export-dynamic is just an approach to make them true?
• If yes, I guess all these --export-dynamic can be replaced with -u _ZTV1B -u _ZTV1C -u _ZTV1D in all the RUN lines. --export-dynamic is preferred just due to its brevity. If that is the case, I think this deserves a comment considering its subtle interaction with noexportdynamic.
• When is devirtualization invalid? For example, if _ZTV1D is exported to the dynamic symbol table and a shared object inherits from class D and overrides the method?

If my last point is correct, I'd agree that we probably need something like a tri-state option. (I hope we can remove --lto-whole-program-visibility if possible)
on/noexportdynamic the value names do not capture the actual meaning. The on actually means: devirtualization is safe as long as the used _ZTV* symbols are not exported.

[tejohnson]

In D91583#2401100, @MaskRay wrote:

Hi, I am still learning the feature and I've just played a bit with the test. I've a couple of questions:

• The --export-dynamic usage in the test seems a bit confusing. Are the VisibleToRegularObj bits of these _ZTV* symbols the important matter and --export-dynamic is just an approach to make them true?

Correct. It was just a shorthand way of preventing these symbols from being eliminated.

• If yes, I guess all these --export-dynamic can be replaced with -u _ZTV1B -u _ZTV1C -u _ZTV1D in all the RUN lines. --export-dynamic is preferred just due to its brevity. If that is the case, I think this deserves a comment considering its subtle interaction with noexportdynamic.

Right, that's why for one of the tests where I'm using the new noexportdynamic value I switched to the -u sequence instead. I can add a comment.

• When is devirtualization invalid? For example, if _ZTV1D is exported to the dynamic symbol table and a shared object inherits from class D and overrides the method?

Correct. If it is both exported and then overridden.

If my last point is correct, I'd agree that we need something like a tri-state option. (I hope we can remove --lto-whole-program-visibility if possible)
on/noexportdynamic the value names do not capture the actual meaning. The on actually means: devirtualization is safe as long as the used _ZTV* symbols are not exported.

Suggestion on the name? It's basically an assertion, i.e. that there is LTO whole program visibility (no, when --export-dynamic not specified, and yes). 'on' means the user is asserting that the _ZTV* are not going to be exported and overridden.

[MaskRay]

Thanks for the clarification.

In D91583#2401116, @tejohnson wrote:

In D91583#2401100, @MaskRay wrote:

Hi, I am still learning the feature and I've just played a bit with the test. I've a couple of questions:

• The --export-dynamic usage in the test seems a bit confusing. Are the VisibleToRegularObj bits of these _ZTV* symbols the important matter and --export-dynamic is just an approach to make them true?

Correct. It was just a shorthand way of preventing these symbols from being eliminated.

• If yes, I guess all these --export-dynamic can be replaced with -u _ZTV1B -u _ZTV1C -u _ZTV1D in all the RUN lines. --export-dynamic is preferred just due to its brevity. If that is the case, I think this deserves a comment considering its subtle interaction with noexportdynamic.

Right, that's why for one of the tests where I'm using the new noexportdynamic value I switched to the -u sequence instead. I can add a comment.

• When is devirtualization invalid? For example, if _ZTV1D is exported to the dynamic symbol table and a shared object inherits from class D and overrides the method?

Correct. If it is both exported and then overridden.

If my last point is correct, I'd agree that we need something like a tri-state option. (I hope we can remove --lto-whole-program-visibility if possible)
on/noexportdynamic the value names do not capture the actual meaning. The on actually means: devirtualization is safe as long as the used _ZTV* symbols are not exported.

Suggestion on the name? It's basically an assertion, i.e. that there is LTO whole program visibility (no, when --export-dynamic not specified, and yes). 'on' means the user is asserting that the _ZTV* are not going to be exported and overridden.

I have a further question: is it realistic to add another bit along with VisibleToRegularObj to convey the information whether a symbol is includeInDynsym()?
This way virtual functions related to individual _ZTV* can be safely devirtualized, no matter how users specify --export-dynamic-symbol,--export-dynamic,--dynamic-list or add link-time shared objects to alter the includeInDynsym() state of _ZTV* symbols.

If a per-symbol bit is not realistic, I think a tri-state --lto-whole-program-visibility makes sense. However, the meaning is still subtle and it deserves some more explanation in the documentation. (perhaps https://clang.llvm.org/docs/LTOVisibility.html plus a section in docs/ld.lld.1)

For off, a more conventional name is none (--icf=none, --build-id=none)
For on, I have a tentative suggestion: assume-no-exported-vtable. The name still does not capture the concept that "if this symbol is not devirtualized, whether it is exported" does not matter.
For noexportdynamic, I am actually wondering whether we really need to make it different from on.

[tejohnson]

In D91583#2401149, @MaskRay wrote:

Thanks for the clarification.

In D91583#2401116, @tejohnson wrote:

In D91583#2401100, @MaskRay wrote:

Hi, I am still learning the feature and I've just played a bit with the test. I've a couple of questions:

• The --export-dynamic usage in the test seems a bit confusing. Are the VisibleToRegularObj bits of these _ZTV* symbols the important matter and --export-dynamic is just an approach to make them true?

Correct. It was just a shorthand way of preventing these symbols from being eliminated.

• If yes, I guess all these --export-dynamic can be replaced with -u _ZTV1B -u _ZTV1C -u _ZTV1D in all the RUN lines. --export-dynamic is preferred just due to its brevity. If that is the case, I think this deserves a comment considering its subtle interaction with noexportdynamic.

Right, that's why for one of the tests where I'm using the new noexportdynamic value I switched to the -u sequence instead. I can add a comment.

• When is devirtualization invalid? For example, if _ZTV1D is exported to the dynamic symbol table and a shared object inherits from class D and overrides the method?

Correct. If it is both exported and then overridden.

If my last point is correct, I'd agree that we need something like a tri-state option. (I hope we can remove --lto-whole-program-visibility if possible)
on/noexportdynamic the value names do not capture the actual meaning. The on actually means: devirtualization is safe as long as the used _ZTV* symbols are not exported.

Suggestion on the name? It's basically an assertion, i.e. that there is LTO whole program visibility (no, when --export-dynamic not specified, and yes). 'on' means the user is asserting that the _ZTV* are not going to be exported and overridden.

I have a further question: is it realistic to add another bit along with VisibleToRegularObj to convey the information whether a symbol is includeInDynsym()?
This way virtual functions related to individual _ZTV* can be safely devirtualized, no matter how users specify --export-dynamic-symbol,--export-dynamic,--dynamic-list or add link-time shared objects to alter the includeInDynsym() state of _ZTV* symbols.

I looked into this some more and it turns out that vtables themselves don't actually need to be exported to be overridden, so this won't be more accurate. So I think a tri-state is the best option, with the new value tied to the --export-dynamic being a strong signal that vtables could be overridden.

If a per-symbol bit is not realistic, I think a tri-state --lto-whole-program-visibility makes sense. However, the meaning is still subtle and it deserves some more explanation in the documentation. (perhaps https://clang.llvm.org/docs/LTOVisibility.html plus a section in docs/ld.lld.1)

Ok let me come up with something and add it to this patch.

For off, a more conventional name is none (--icf=none, --build-id=none)

ok

For on, I have a tentative suggestion: assume-no-exported-vtable. The name still does not capture the concept that "if this symbol is not devirtualized, whether it is exported" does not matter.

I'd prefer not to tie this to vtables specifically, in the case that we want to apply the whole program visibility concept beyond vtables in the future. Maybe 'always'?

For noexportdynamic, I am actually wondering whether we really need to make it different from on.

I think we do want to keep these separate because it would be good to have a mode to force this on if any cases pop up where --export-dynamic is used but it doesn't actually violate any requirements for the whole program optimization. I anticipate that it's best to be conservative under that case, but for some binaries it may be ok if they are carefully vetted.

[MaskRay]

I have a further question: is it realistic to add another bit along with VisibleToRegularObj to convey the information whether a symbol is includeInDynsym()?
This way virtual functions related to individual _ZTV* can be safely devirtualized, no matter how users specify --export-dynamic-symbol,--export-dynamic,--dynamic-list or add link-time shared objects to alter the includeInDynsym() state of _ZTV* symbols.

Should a new bit be introduced? If the executable defines a virtual class A which is overridden by a link-time shared object, the vtable symbol will be exported. It is a misoptimization if LTO considers devirtualizes A's member functions. How does the --lto-whole-program-visibility design deal with this pitfall?

I looked into this some more and it turns out that vtables themselves don't actually need to be exported to be overridden, so this won't be more accurate. So I think a tri-state is the best option, with the new value tied to the --export-dynamic being a strong signal that vtables could be overridden.

[pcc]

I've said before that I think that --lto-whole-program-visibility should relax visibility of vtable symbols etc to hidden. That way, --export-dynamic wouldn't actually allow you to make this kind of mistake.

[tejohnson]

In D91583#2404519, @pcc wrote:

I've said before that I think that --lto-whole-program-visibility should relax visibility of vtable symbols etc to hidden. That way, --export-dynamic wouldn't actually allow you to make this kind of mistake.

That would presumably result in an error in some of the problematic cases, whereas here we want to simply suppress --lto-whole-program-visibility to avoid any issues automatically.

But isn't it the case that you don't even need for the vtable symbol itself to be exported in order to derive from the class and override its virtual methods?

[pcc]

In D91583#2404678, @tejohnson wrote:

In D91583#2404519, @pcc wrote:

I've said before that I think that --lto-whole-program-visibility should relax visibility of vtable symbols etc to hidden. That way, --export-dynamic wouldn't actually allow you to make this kind of mistake.

That would presumably result in an error in some of the problematic cases, whereas here we want to simply suppress --lto-whole-program-visibility to avoid any issues automatically.

But isn't it the case that you don't even need for the vtable symbol itself to be exported in order to derive from the class and override its virtual methods?

That's true, but it's the same situation that we have now with --lto-whole-program-visibility and not passing --export-dynamic.

I thought that --lto-whole-program-visibility was basically intended to do the same thing as specifying __attribute__((visibility("hidden"))) on classes, except at link time. That isn't fundamentally incompatible with --export-dynamic (or, for that matter, -shared) since you can always expose an interface that doesn't involve the classes.

[tejohnson]

In D91583#2406005, @pcc wrote:

In D91583#2404678, @tejohnson wrote:

In D91583#2404519, @pcc wrote:

I've said before that I think that --lto-whole-program-visibility should relax visibility of vtable symbols etc to hidden. That way, --export-dynamic wouldn't actually allow you to make this kind of mistake.

That would presumably result in an error in some of the problematic cases, whereas here we want to simply suppress --lto-whole-program-visibility to avoid any issues automatically.

But isn't it the case that you don't even need for the vtable symbol itself to be exported in order to derive from the class and override its virtual methods?

That's true, but it's the same situation that we have now with --lto-whole-program-visibility and not passing --export-dynamic.

Right, just confirming my understanding.

I thought that --lto-whole-program-visibility was basically intended to do the same thing as specifying __attribute__((visibility("hidden"))) on classes, except at link time. That isn't fundamentally incompatible with --export-dynamic (or, for that matter, -shared) since you can always expose an interface that doesn't involve the classes.

Correct. It's an assertion. The goal of --lto-whole-program-visibility was to apply it uniformly when the build system believes it is doing static linking, but in some occasional cases a binary may require --export-dynamic and it is simpler to automatically fall back to non-lto-whole-program-visibility in that case.

[tejohnson]

While I investigate alternative mechanisms to handle --export-dynamic, I've sent D92060 to add a --no- version of the option, to simplify working around issues when --lto-whole-program-visibility is specified broadly.

[tejohnson]

I implemented @MaskRay's suggestion and added a bit to convey whether a symbol is exported to the dynamic linker (via --export-dynamic[-symbol=] or --dynamic-list=), and use that to prevent the LTO visibility upgrade for WPD. I added support to both lld and gold plugin, and associated tests. Note that I couldn't use includeInDynsym in lld because that is not set for linkonce_odr symbols that were thus have canBeOmittedFromSymbolTable set (since any referencing module must have it's own copy) - we still want to block the LTO visibility upgrade for those symbols to avoid WPD. So I am using a slightly different interface that more directly checks whether export-dynamic is in effect.

[tejohnson]

Update per previous comment

[tejohnson]

In D91583#2475421, @tejohnson wrote:

I implemented @MaskRay's suggestion and added a bit to convey whether a symbol is exported to the dynamic linker (via --export-dynamic[-symbol=] or --dynamic-list=), and use that to prevent the LTO visibility upgrade for WPD. I added support to both lld and gold plugin, and associated tests. Note that I couldn't use includeInDynsym in lld because that is not set for linkonce_odr symbols that were thus have canBeOmittedFromSymbolTable set (since any referencing module must have it's own copy) - we still want to block the LTO visibility upgrade for those symbols to avoid WPD. So I am using a slightly different interface that more directly checks whether export-dynamic is in effect.

ping - @MaskRay can you take a look?

[MaskRay]

Sorry for the delay. I did not review much over Christmas. I'll try to get to this sometime in the next couple of days.

[MaskRay]

Generally looks good.

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

sym->exportDynamic || sym->inDynamicList

Then isExportDynamic does not need to be public.

lld/test/ELF/lto/devirt_vcall_vis_export_dynamic.ll

2 ↗

(On Diff #314154)

Nit: in LLD tests we use ;; to differentiate regular comments from CHECK RUN markers.

43 ↗

(On Diff #314154)

%s -> /dev/null

lld/test/ELF/lto/devirt_vcall_vis_public.ll

8

s/\t/ /

9

s/\t/ /

The two lines can be joined.

llvm/include/llvm/LTO/LTO.h

466 ↗

(On Diff #314154)

How about VisibleToOtherModules?

The name VisibleToDynamicLinker is too tied to the ELF binary format.

[tejohnson]

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

sym->exportDynamic is false for linkonce_odr vtables, that was what I was referencing in this comment (otherwise I could use includeInDynsym which checks that):

Note that I couldn't use includeInDynsym in lld because that is not set for linkonce_odr symbols that were thus have canBeOmittedFromSymbolTable set (since any referencing module must have it's own copy) - we still want to block the LTO visibility upgrade for those symbols to avoid WPD. So I am using a slightly different interface that more directly checks whether export-dynamic is in effect.

lld/test/ELF/lto/devirt_vcall_vis_export_dynamic.ll

2 ↗

(On Diff #314154)

Can I do this in a follow on NFC commit? Otherwise it will make the diffs really noisy in this test.

llvm/include/llvm/LTO/LTO.h

466 ↗

(On Diff #314154)

VisibleToOtherModules sounds to me like it means LLVM Modules that are being linked together statically. I wanted to note that these are symbols that may have dynamic references not seen by the static link. Is there anything like --export-dynamic for other binary formats? If not, then it is ELF specific anyway.

[tejohnson]

lld/test/ELF/lto/devirt_vcall_vis_export_dynamic.ll

43 ↗

(On Diff #314154)

Fixed here and elsewhere - but not sure why it matters?

lld/test/ELF/lto/devirt_vcall_vis_public.ll

8

Done here and elsewhere

9

ditto

[tejohnson]

Address comments

[MaskRay]

llvm/include/llvm/LTO/LTO.h

466 ↗

(On Diff #314154)

@rnk for thoughts on COFF.

[tejohnson]

Rebase and use ";;" instead of ";" for comments.

[tejohnson]

lld/test/ELF/lto/devirt_vcall_vis_export_dynamic.ll

2 ↗

(On Diff #314154)

I went ahead and fixed this in the existing tests in 5b42fd8dd4e7e29125a09a41a33af7c9cb57d144. I have updated this with a rebased version that fixes the comments in the new tests as well.

[MaskRay]

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

Sorry, I don't understand the difference. If I replace this with sym->exportDynamic, I don't get a test failure...

llvm/include/llvm/LTO/LTO.h

466 ↗

(On Diff #314154)

Perhaps another name is Exported.

For ELF, the does not seem to be restricted to shared objects seen as in the input file.

[tejohnson]

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

Ah, this is a test deficiency, looks like I need to make one or more of the vtables linkonce_odr to expose it. Will address that. The reason it is an issue for linkonce_odr can be seen in createBitcodeSymbol in lld/ELF/InputFiles.cpp, where it does:

if (canOmitFromDynSym)
  newSym.exportDynamic = false;

The canOmitFromDynSym gets propagated via the input file but is originally set in GlobalValue::canBeOmittedFromSymbolTable() for linkonce_odr with hasAtLeastLocalUnnamedAddr().

llvm/include/llvm/LTO/LTO.h

466 ↗

(On Diff #314154)

"Exported" is ambiguous - we use that throughout ThinLTO to mean exported from the current module (to other modules being LTO linked).

For ELF, the does not seem to be restricted to shared objects seen as in the input file.

Sorry I don't follow?

[tejohnson]

Use linkonce_odr vtables to illustrate issue with Symbol exportDynamic

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

I've improved the tests. Confirmed that the improved lld test fails if you make the change you proposed.

[tejohnson]

ping

[MaskRay]

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

I see that sym->isExportDynamic is used to prevent canOmitFromDynSym (unnamed_addr linkonce_odr or local_unnamed_addr linkonce_odr constant) logic.

There is one case where sym->isExportDynamic(sym->kind(), sym->visibility) may be false while sym->exportDynamic is true: a shared object with a STV_DEFAULT reference to the symbol can set exportDynamic (InputFiles.cpp:1557).

sym->isExportDynamic(...) || sym->exportDynamic should be safe.

llvm/include/llvm/LTO/LTO.h

466 ↗

(On Diff #314154)

Seems that the idea is just whether the symbol is exported and can be used by other linked images. A dynamic linker is the ELF concept but the idea can be used by other binary formats. In COFF it is called "export table". In Mach-O there are linker options -exported_*, and non-exported symbols are converted to private externs.

ThinLTO has already used exported to mean symbols exchanged among LLVM modules so export should not be used. Perhaps just exportDynamic? The name stills stems from ELF but users from other binary formats can still find similarity.

[MaskRay]

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

The comprehensive rule for when exportDynamic is set:

* non-local `STV_DEFAULT/STV_PROTECTED` (this means it can be hid by `--exclude-libs`)
* logical OR of the following:
  + undefined
  + (`--export-dynamic` || `-shared`) && ! (unnamed_addr linkonce_odr GlobalVariable || local_unnamed_addr linkonce_odr constant GlobalVariable)
  + matched by `--dynamic-list/--export-dynamic-symbol-list/--export-dynamic-symbol`
  + defined or referenced by a shared object as `STV_DEFAULT`
  + `STV_PROTECTED` definition in a shared object preempted by copy relocation/canonical PLT when `--ignore-{data,function}-address-equality}` is specified
  + `-z ifunc-noplt` && has at least one relocation

The last two are edge cases (but works if you use sym->exportDynamic).

About the common case "defined or referenced by a shared object as STV_DEFAULT":
for ld.lld %t.o %t1.so -o %t, if %t1.so defines (linkonce_odr) the vtable, it can be preempted by the executable definition. %t thus needs to export the vtable.

This may deserve a test (sym->isExportDynamic(...) || sym->exportDynamic and `sym->isExportDynamic(...) have different behaviors)

[tejohnson]

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

I see, so essentially sym->isExportDynamic(...) and sym->exportDynamic are non-overlapping and neither is a superset of the either. I will change the code to check them both. In terms of creating a test, can any of the cases where the latter is true but the former is not be triggered for a vtable? Since this is only being used for WPD right now, I'd need to be able to test this with a vtable def visible to a virtual call that would otherwise be devirtualized.

llvm/include/llvm/LTO/LTO.h

466 ↗

(On Diff #314154)

Sounds good, will change to simply ExportDynamic.

[MaskRay]

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

llc -filetype=obj -relocation-model=pic %t.ll -o %t.o
lld -shared %t.o -o %t.so

The output has a dynamic symbol _ZTV1B.

[tejohnson]

Rename VisibleToDynamicLinker to ExportDynamic

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

sym->isExportDynamic(...) already returns true if config->shared, so in the library link it should already be handled by the current patch, or am I misunderstanding?

[MaskRay]

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

llc -filetype=obj -relocation-model=pic %t.ll -o %t.o
ld.lld -shared %t.o -o %t.so
ld.lld %t.o %t.so -o %t

The ELF semantic is that %t.o preempts every default visibility definition in %t.so. So, even in the absence of --export-dynamic/--dynamic-list/--export-dynamic-symbol, the definitions in %t need to be exported (to .dynsym) to allow preemption at runtime.

This is a case where sym->isExportDynamic(...) is false while sym->exportDynamic is true.

(sym->isExportDynamic(...) is true while sym->exportDynamic is false should be impossible, so no need for a test.)

[tejohnson]

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

llc -filetype=obj -relocation-model=pic %t.ll -o %t.o
ld.lld -shared %t.o -o %t.so
ld.lld %t.o %t.so -o %t

The ELF semantic is that %t.o preempts every default visibility definition in %t.so. So, even in the absence of --export-dynamic/--dynamic-list/--export-dynamic-symbol, the definitions in %t need to be exported (to .dynsym) to allow preemption at runtime.

This is a case where sym->isExportDynamic(...) is false while sym->exportDynamic is true.

Ok let me create a test for this.

(sym->isExportDynamic(...) is true while sym->exportDynamic is false should be impossible, so no need for a test.)

It can happen which is why I added the check for sym->isExportDynamic(...) and not sym->exportDynamic in the first place. The current test case (with the linkonce_odr vtables) is exactly this case, because of canOmitFromDynSym (this was what I was describing upthread).

[MaskRay]

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

Ah yes, I forgot again that sym->exportDynamic can be set to false due to canOmitFromDynSym (ThinLTO auto hiding) logic.

[tejohnson]

Also check sym->exportDynamic and add test

[tejohnson]

I think I've addressed all the comments now.

lld/ELF/LTO.cpp

252 ↗

(On Diff #314154)

Added check of sym->exportDynamic and added test case (confirmed it fails because we get devirtualizations without this change).

[MaskRay]

LG. There are several minor required comment/description changes and test updates.

Under --export-dynamic[-symbol=] and --dynamic-list=, identify the exported symbols and prevent their LTO visibility from being upgraded.

The description needs a update now, something like: Identify the exported symbols (--export-dynamic[-symbol=], --dynamic-list=, or definitions needed to preempt shared objects)

This helps avoid use of whole program devirtualization when there may be overrides in dynamically loaded libraries.

Perhaps just dynamic libraries. "dynamically loaded libraries" gives me a feeling of dlopen, but the issue can arise with regular link-time shared objects as well: it is valid for a new version shared object to add more symbols (derived classes with vtables).

lld/test/ELF/lto/devirt_vcall_vis_export_dynamic.ll

2 ↗

(On Diff #319642)

This needs an update (due to a new case: shared object preemption): exported symbols prevent devirtualization.

101 ↗

(On Diff #319642)

107 ↗

(On Diff #319642)

[MaskRay]

[LTO] Prevent devirtualization for symbols exported to dynamic linker

And the subject ("dynamic linker") needs a change

[tejohnson]

Address comments

lld/test/ELF/lto/devirt_vcall_vis_export_dynamic.ll

101 ↗

(On Diff #319642)

Woops, good catch. I was testing the cases one at a time and missed fixing all the RUN lines again.

[lanza]

@tejohnson I'm not sure this change is working correctly -- either that or my builds are messed up.

BitcodeCompiler::add builds the resolutions for a bitcode module's symbols. For all symbols in the module r.ExportDynamic is set via isExportDynamic:

static bool isExportDynamic(Kind k, uint8_t visibility) {
  if (k == SharedKind)
    return visibility == llvm::ELF::STV_DEFAULT;
  return config->shared || config->exportDynamic;
}

The Kind for a BitcodeFile symbol is not SharedKind and thus config->shared causes this to always come back true. This is then used for GlobalResolutions.ExportDynamic which is used to build the DynamicExportSymbols list. Thus the DynamicExportSymbols list contains all the bitcode module's symbols and nothing gets vcall_visibility.

[tejohnson]

In D91583#2610509, @lanza wrote:

@tejohnson I'm not sure this change is working correctly -- either that or my builds are messed up.

BitcodeCompiler::add builds the resolutions for a bitcode module's symbols. For all symbols in the module r.ExportDynamic is set via isExportDynamic:

static bool isExportDynamic(Kind k, uint8_t visibility) {
  if (k == SharedKind)
    return visibility == llvm::ELF::STV_DEFAULT;
  return config->shared || config->exportDynamic;
}

The Kind for a BitcodeFile symbol is not SharedKind and thus config->shared causes this to always come back true. This is then used for GlobalResolutions.ExportDynamic which is used to build the DynamicExportSymbols list. Thus the DynamicExportSymbols list contains all the bitcode module's symbols and nothing gets vcall_visibility.

To me this is WAI. Why is "config->shared" true for your bitcode module? This should only affect when using the linker flags that assert you have whole program visibility during the link, which isn't true for a shared library and its symbols.

[lanza]

To me this is WAI. Why is "config->shared" true for your bitcode module? This should only affect when using the linker flags that assert you have whole program visibility during the link, which isn't true for a shared library and its symbols.

Got ya. For our Android apps we compute the actual import and export lists exactly and thus can compute the symbol visibility during linking and use --lto-whole-program-visibility accordingly. (Though this is not yet used in production for build system reasons). This change makes the list of symbols equivalent to the list of DynamicExportSymbols, so even though we can tell lld that _ZTVN3xyz is internal-only it won't get vcall_visibility.

[MaskRay]

In D91583#2612405, @lanza wrote:

To me this is WAI. Why is "config->shared" true for your bitcode module? This should only affect when using the linker flags that assert you have whole program visibility during the link, which isn't true for a shared library and its symbols.

Got ya. For our Android apps we compute the actual import and export lists exactly and thus can compute the symbol visibility during linking and use --lto-whole-program-visibility accordingly. (Though this is not yet used in production for build system reasons). This change makes the list of symbols equivalent to the list of DynamicExportSymbols, so even though we can tell lld that _ZTVN3xyz is internal-only it won't get vcall_visibility.

The sym->isExportDynamic(sym->kind(), sym->visibility) || sym->exportDynamic || sym->inDynamicList condition is a bit conservative.
I sent D98220 to allow WPD with hidden/internal symbols.

[tejohnson]

In D91583#2612405, @lanza wrote:

To me this is WAI. Why is "config->shared" true for your bitcode module? This should only affect when using the linker flags that assert you have whole program visibility during the link, which isn't true for a shared library and its symbols.

Got ya. For our Android apps we compute the actual import and export lists exactly and thus can compute the symbol visibility during linking and use --lto-whole-program-visibility accordingly. (Though this is not yet used in production for build system reasons). This change makes the list of symbols equivalent to the list of DynamicExportSymbols, so even though we can tell lld that _ZTVN3xyz is internal-only it won't get vcall_visibility.

I see - so just to confirm, when compiling it isn't clear that these symbols are have internal or hidden visibility, but only during linking? Because otherwise clang should already have applied an appropriate vcall_visibility that allows WPD.

[lanza]

I see - so just to confirm, when compiling it isn't clear that these symbols are have internal or hidden visibility, but only during linking? Because otherwise clang should already have applied an appropriate vcall_visibility that allows WPD.

Correct, when compiling we don't know what's exported and what's not, so nothing is applied. This is fundamentally similar to a problem you mentioned at a talk last year -- a source file shared between multiple apps might not be used the same everywhere, so you can only know after actually linking how it's used. So we apply stricter visibility via checking what was actually imported from the library. If _ZTV5Thing isn't used outside the current library we can mark it as such and then WPD operate with this extra info.

[MaskRay]

In D91583#2612525, @lanza wrote:

I see - so just to confirm, when compiling it isn't clear that these symbols are have internal or hidden visibility, but only during linking? Because otherwise clang should already have applied an appropriate vcall_visibility that allows WPD.

Correct, when compiling we don't know what's exported and what's not, so nothing is applied. This is fundamentally similar to a problem you mentioned at a talk last year -- a source file shared between multiple apps might not be used the same everywhere, so you can only know after actually linking how it's used. So we apply stricter visibility via checking what was actually imported from the library. If _ZTV5Thing isn't used outside the current library we can mark it as such and then WPD operate with this extra info.

Do you use a local: version node in a version script to make vtable symbols local in a -shared link? LTO does not know the effective binding has become local in that case and can lose devirtualization opportunities.

The other possibility is -flto={full,thin} compile in one translation unit and -flto={full,thin} -fvisibility=hidden in another translation unit. Due to ELF visibility rule the most constraining one wins which may be more constraining than !vcall_visibility.

[tejohnson]

In D91583#2612525, @lanza wrote:

I see - so just to confirm, when compiling it isn't clear that these symbols are have internal or hidden visibility, but only during linking? Because otherwise clang should already have applied an appropriate vcall_visibility that allows WPD.

Correct, when compiling we don't know what's exported and what's not, so nothing is applied. This is fundamentally similar to a problem you mentioned at a talk last year -- a source file shared between multiple apps might not be used the same everywhere, so you can only know after actually linking how it's used. So we apply stricter visibility via checking what was actually imported from the library. If _ZTV5Thing isn't used outside the current library we can mark it as such and then WPD operate with this extra info.

Ok thanks. Dumb question - how do you know when you are linking the shared library what will be used outside of it (because presumably you don't know this until it is linked into the consuming binaries later on in the build process)? And do you communicate this info via --export-dynamic-symbol or the like?

[lanza]

Do you use a local: version node in a version script to make vtable symbols local in a -shared link? LTO does not know the effective binding has become local in that case and can lose devirtualization opportunities.

Yup. This is exactly what I've started looking for over the past few days. LTO is clearly not taking full advantage of the fact that we can guarantee these symbols are hidden and local only. The ExportDynamic was the first obvious thing I ran into. I'm sure there's a good bit more.

Ok thanks. Dumb question - how do you know when you are linking the shared library what will be used outside of it (because presumably you don't know this until it is linked into the consuming binaries later on in the build process)? And do you communicate this info via --export-dynamic-symbol or the like?

Not a dumb question, it's a pretty reasonable one. We run the link twice. Once in the normal order -- leaf to root in the dependency graph and lld tells us the list of imported symbols. From those lists we generate the full list of symbols that need to be exported and pass a version script with local:\n*; included.

lld/ELF/Driver.cpp

if (s == "none")
     return false;
   if (s != "zlib")
     error("unknown --compress-debug-sections value: " + s);
   if (!zlib::isAvailable())
     error("--compress-debug-sections: zlib is not available");
   return true;
 }
 
+static bool getLtoWholeProgramVisibility(opt::InputArgList &args) {
+  auto *Arg = args.getLastArg(OPT_lto_whole_program_visibility,
+                              OPT_lto_whole_program_visibility_eq);
+  if (!Arg)
+    return false;
+  if (Arg->getOption().getID() == OPT_lto_whole_program_visibility)
+    return true;
+  StringRef s = Arg->getValue();
+  if (s == "off")
+    return false;
+  else if (s == "on")
+    return true;
+  else if (s == "noexportdynamic")
+    return !(args.hasFlag(OPT_export_dynamic, OPT_no_export_dynamic, false));
+  error("unknown --lto-whole-program-visibility: " + s);
+  return false;
+}
+
 static StringRef getAliasSpelling(opt::Arg *arg) {
   if (const opt::Arg *alias = arg->getAlias())
     return alias->getSpelling();
   return arg->getSpelling();
 }
 
 static std::pair<StringRef, StringRef> getOldNewOptions(opt::InputArgList &args,
                                                         unsigned id) {
static void readConfigs(opt::InputArgList &args) {
   config->init = args.getLastArgValue(OPT_init, "_init");
   config->ltoAAPipeline = args.getLastArgValue(OPT_lto_aa_pipeline);
   config->ltoCSProfileGenerate = args.hasArg(OPT_lto_cs_profile_generate);
   config->ltoCSProfileFile = args.getLastArgValue(OPT_lto_cs_profile_file);
   config->ltoDebugPassManager = args.hasArg(OPT_lto_debug_pass_manager);
   config->ltoEmitAsm = args.hasArg(OPT_lto_emit_asm);
   config->ltoNewPassManager = args.hasArg(OPT_lto_new_pass_manager);
   config->ltoNewPmPasses = args.getLastArgValue(OPT_lto_newpm_passes);
-  config->ltoWholeProgramVisibility =
-      args.hasArg(OPT_lto_whole_program_visibility);
+  config->ltoWholeProgramVisibility = getLtoWholeProgramVisibility(args);
   config->ltoo = args::getInteger(args, OPT_lto_O, 2);
   config->ltoObjPath = args.getLastArgValue(OPT_lto_obj_path_eq);
   config->ltoPartitions = args::getInteger(args, OPT_lto_partitions, 1);
   config->ltoSampleProfile = args.getLastArgValue(OPT_lto_sample_profile);
   config->ltoBasicBlockSections =
       args.getLastArgValue(OPT_lto_basic_block_sections);
   config->ltoUniqueBasicBlockSectionNames =
       args.hasFlag(OPT_lto_unique_basic_block_section_names,

lld/ELF/Options.td

def lto_partitions: JJ<"lto-partitions=">,
   HelpText<"Number of LTO codegen partitions">;
 def lto_cs_profile_generate: FF<"lto-cs-profile-generate">,
   HelpText<"Perform context sensitive PGO instrumentation">;
 def lto_cs_profile_file: JJ<"lto-cs-profile-file=">,
   HelpText<"Context sensitive profile file path">;
 def lto_obj_path_eq: JJ<"lto-obj-path=">;
 def lto_sample_profile: JJ<"lto-sample-profile=">,
   HelpText<"Sample profile file path">;
-def lto_whole_program_visibility: FF<"lto-whole-program-visibility">,
-  HelpText<"Asserts that the LTO link has whole program visibility">;
+def lto_whole_program_visibility: FF<"lto-whole-program-visibility">;
+def lto_whole_program_visibility_eq: JJ<"lto-whole-program-visibility=">,
+  HelpText<"Asserts that the LTO link has whole program visibility. "
+    "If specified without any argument, defaults to 'on'. "
+    "If 'noexportdynamic' then on unless --export-dynamic also specified.">,
+  MetaVarName<"[off,noexportdynamic,on]">;
 def disable_verify: F<"disable-verify">;
 defm mllvm: Eq<"mllvm", "Additional arguments to forward to LLVM's option processing">;
 def opt_remarks_filename: Separate<["--"], "opt-remarks-filename">,
   HelpText<"YAML output file for optimization remarks">;
 def opt_remarks_passes: Separate<["--"], "opt-remarks-passes">,
   HelpText<"Regex for the passes that need to be serialized to the output file">;
 def opt_remarks_with_hotness: FF<"opt-remarks-with-hotness">,
   HelpText<"Include hotness information in the optimization remarks file">;

lld/test/ELF/lto/devirt_vcall_vis_public.ll

 ; REQUIRES: x86
 ; Test that --lto-whole-program-visibility enables devirtualization.
 
 ; Index based WPD
 ; Generate unsplit module with summary for ThinLTO index-based WPD.
 ; RUN: opt --thinlto-bc -o %t2.o %s
 ; RUN: ld.lld %t2.o -o %t3 -save-temps --lto-whole-program-visibility \
 ; RUN:   -mllvm -pass-remarks=. --export-dynamic 2>&1 | FileCheck %s --check-prefix=REMARK
 ; RUN: llvm-dis %t2.o.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
+; Try again with explicit "on"
+; RUN: ld.lld %t2.o -o %t3 -save-temps --lto-whole-program-visibility=on \
+; RUN:   -mllvm -pass-remarks=. --export-dynamic 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: llvm-dis %t2.o.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
+; Try again with "noexportdynamic" and use -u instead of --export-dynamic
+; RUN: ld.lld %t2.o -o %t3 -save-temps --lto-whole-program-visibility=noexportdynamic \
+; RUN:   -mllvm -pass-remarks=. -u _start -u _ZTV1B -u _ZTV1C -u _ZTV1D \
+; RUN:    2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: llvm-dis %t2.o.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
 
 ; Hybrid WPD
 ; Generate split module with summary for hybrid Thin/Regular LTO WPD.
 ; RUN: opt --thinlto-bc --thinlto-split-lto-unit -o %t.o %s
 ; RUN: ld.lld %t.o -o %t3 -save-temps --lto-whole-program-visibility \
 ; RUN:   -mllvm -pass-remarks=. --export-dynamic 2>&1 | FileCheck %s --check-prefix=REMARK
 ; RUN: llvm-dis %t.o.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
 
 ; Regular LTO WPD
 ; RUN: opt -o %t4.o %s
 ; RUN: ld.lld %t4.o -o %t3 -save-temps --lto-whole-program-visibility \
 ; RUN:   -mllvm -pass-remarks=. --export-dynamic 2>&1 | FileCheck %s --check-prefix=REMARK
 ; RUN: llvm-dis %t3.0.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
 
 ; REMARK-DAG: single-impl: devirtualized a call to _ZN1A1nEi
 ; REMARK-DAG: single-impl: devirtualized a call to _ZN1D1mEi
 
 ; Try everything again but without -whole-program-visibility to confirm
 ; WPD fails
 
 ; Index based WPD
 ; RUN: ld.lld %t2.o -o %t3 -save-temps \
 ; RUN:   -mllvm -pass-remarks=. --export-dynamic 2>&1 | FileCheck %s --implicit-check-not single-impl --allow-empty
 ; RUN: llvm-dis %t2.o.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-NODEVIRT-IR
+; Try again with explicit "off"
+; RUN: ld.lld %t2.o -o %t3 -save-temps --lto-whole-program-visibility=off \
+; RUN:   -mllvm -pass-remarks=. --export-dynamic 2>&1 | FileCheck %s --implicit-check-not single-impl --allow-empty
+; RUN: llvm-dis %t2.o.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-NODEVIRT-IR
+; Try again with "noexportdynamic"
+; RUN: ld.lld %t2.o -o %t3 -save-temps --lto-whole-program-visibility=noexportdynamic \
+; RUN:   -mllvm -pass-remarks=. --export-dynamic 2>&1 | FileCheck %s --implicit-check-not single-impl --allow-empty
+; RUN: llvm-dis %t2.o.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-NODEVIRT-IR
 
 ; Hybrid WPD
 ; RUN: ld.lld %t.o -o %t3 -save-temps \
 ; RUN:   -mllvm -pass-remarks=. --export-dynamic 2>&1 | FileCheck %s --implicit-check-not single-impl --allow-empty
 ; RUN: llvm-dis %t.o.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-NODEVIRT-IR
 
 ; Regular LTO WPD
 ; RUN: ld.lld %t4.o -o %t3 -save-temps \