Page MenuHomePhabricator

[lld] Allow --export-dynamic to override --lto-whole-program-visibility
Needs ReviewPublic

Authored by tejohnson on Mon, Nov 16, 6:35 PM.

Details

Reviewers
MaskRay
espindola
Summary

Adds a version of --lto-whole-program-visibility that takes a parameter,
including a new 'noexportdynamic', in which case --lto-whole-program-visibility
is not enabled when --export-dynamic is specified. Use of --export-dynamic
suggests that there may not in fact be whole program visibility, because
symbols become available externally, enabling e.g. virtual function
overrides not seen by the LTO unit. Adding this support simplifies use
of --lto-whole-program-visibility in contexts where static linking is
typically the case, but where some binaries will use --export-dynamic to
enable dlopen.

Diff Detail

Event Timeline

tejohnson created this revision.Mon, Nov 16, 6:35 PM
Herald added a project: Restricted Project. · View Herald Transcript
tejohnson requested review of this revision.Mon, Nov 16, 6:35 PM
MaskRay added a comment.EditedMon, Nov 16, 7:57 PM

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.

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 added a comment.EditedTue, Nov 17, 3:30 PM

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.

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.

Thanks for the clarification.

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.

Thanks for the clarification.

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.

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 added a subscriber: pcc.Wed, Nov 18, 7:45 PM

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.

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 added a comment.Thu, Nov 19, 10:08 AM
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.

In D91583#2406005, @pcc 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.

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.