[LTO] Prevent devirtualization for symbols exported to dynamic linker

[tejohnson]

Summary

Under --export-dynamic[-symbol=] and --dynamic-list=, identify the
exported symbols and prevent their LTO visibility from being upgraded.
This helps avoid use of whole program devirtualization when there may
be overrides in dynamically loaded 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

sym->exportDynamic || sym->inDynamicList

Then isExportDynamic does not need to be public.

lld/test/ELF/lto/devirt_vcall_vis_export_dynamic.ll

2

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

43

%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

How about VisibleToOtherModules?

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

[tejohnson]

lld/ELF/LTO.cpp

252

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

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

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

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

@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

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

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

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

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

"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

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