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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

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.

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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91583