[PATCH] D91583: [lld] Allow --export-dynamic to override --lto-whole-program-visibility

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D91583#2401149 <https://reviews.llvm.org/D91583#2401149>, @MaskRay wrote:

> Thanks for the clarification.
>
> In D91583#2401116 <https://reviews.llvm.org/D91583#2401116>, @tejohnson wrote:
>
>> In D91583#2401100 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91583/new/

https://reviews.llvm.org/D91583