[PATCH] D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals

Tue Aug 11 14:55:44 PDT 2020

bd1976llvm added a comment.

In D85782#2211636 <https://reviews.llvm.org/D85782#2211636>, @MaskRay wrote:

> The additional test file places too much burden on people maintaining the tests in the future. I think an additional `@hidden_data = hidden global` in `CodeGen/X86/linux-preemption.ll` should be sufficient.

Ok. Great! Unfortunately, I do think that the expansion of the elf-code-model testing is necessary at some point. The current test is only for PIE which constrains the codegen.

>> GNU now recommends -fvisiblity=hidden (https://gcc.gnu.org/wiki/Visibility) so my understanding is that there will be very few STV_DEFAULT symbols.
>
> I am not very sure this is now recommended but is indeed a non-rare configuration we should care about.

Agreed. Recommended is probably too strong.

>> Improves the assembly (as there are fewer .Lfoo$local labels).
>
> There are still plenty of cases with the `.Lfoo$local`. I'd not state this advantage explicitly in this case.

No problem - this is a minor benefit anyway.

>> Restores the effectiveness of the --wrap linker option and makes the behavior of --wrap consistent between LTO and normal builds for references within a translation-unit.
>
> I think we should explicitly call out that this is still brittle. It is just a side benefit. (I know for you it is a major thing but I hope we don't give us false impression that this is reliably supported.) The main thing is the .symtab saving.
>
> (My disagreement about advertising --wrap as an advantage does not mean that I'd unnecessarily break this - I think this will be sufficient robust - despite the brittle --wrap setup; and I'd pay attention for this case when improving/refactoring --wrap in LLD to not cause you additional trouble)
>
> Also state that the --wrap behavior is specific to LLD.

Agreed and thanks for considering us :)

Do you want me to close this review and put up another one with the improvements to the description that you have recommended (maybe not even mentioning the --wrap behaviour at all), or should I update this review?

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

https://reviews.llvm.org/D85782