Page MenuHomePhabricator

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

Authored by bd1976llvm on Aug 11 2020, 2:10 PM.

Details

Summary

This patch restricts the behaviour of referencing via .Lfoo$local local aliases, introduced in https://reviews.llvm.org/D73230, to STV_DEFAULT globals only.

Hidden symbols via -fvisiblity=hidden (https://gcc.gnu.org/wiki/Visibility) is an important scenario.

Benefits:

  • Improves the size of object files by using fewer STT_SECTION symbols.
  • The code reads a bit better (it was not obvious to me without going back to the code reviews why the canBenefitFromLocalAlias function currently doesn't consider visibility).

There is also a side benefit in restoring the effectiveness of the --wrap linker option and making the behavior of --wrap consistent between LTO and normal builds for references within a translation-unit. Note: this --wrap behaviour (which is specific to LLD) should not be considered reliable. See comments on https://reviews.llvm.org/D73230 for more.

Diff Detail

Event Timeline

bd1976llvm created this revision.Aug 11 2020, 2:10 PM
bd1976llvm requested review of this revision.Aug 11 2020, 2:10 PM
MaskRay added a comment.EditedAug 11 2020, 2:35 PM

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.

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.

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.

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.

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?

Updating the summary should be fine.

The two new tests code-model-elf-pic-sip.ll and code-model-elf-pic-nosip.ll are a bit too long. It seems to me we just need an additional variable in test/CodeGen/X86/linux-preemption.ll and we will be good. There may be value improving test coverage but the excessive testing in code-model-elf-pic-sip.ll and code-model-elf-pic-nosip.ll should probably go to a separate change. (I do wonder whether they duplicate existing testing)

bd1976llvm edited the summary of this revision. (Show Details)Aug 11 2020, 6:00 PM

Moved testing into linux-preemption.ll

bd1976llvm edited the summary of this revision. (Show Details)Aug 12 2020, 1:38 AM

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.

I was sure that I had read this somewhere. From the following GCC page: https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fvisibility

-fvisibility=[default|internal|hidden|protected]
...It is strongly recommended that you use this in any shared objects you distribute.

MaskRay accepted this revision.Aug 13 2020, 12:36 PM

Thanks!

This revision is now accepted and ready to land.Aug 13 2020, 12:36 PM
skan added a subscriber: skan.
skan added a comment.Aug 25 2020, 2:06 AM

I think this introduces an old issue when lowering the instruction.

Ref: https://sourceware.org/bugzilla/show_bug.cgi?id=13600

Reproducer:

__attribute__((visibility("protected"))) void * foo (void) { return (void *)foo; }
clang -c  -O0  -fpic test.c 
clang -O0  -fpic test.o  -shared

Error message:
bfd/bin/ld: test.o: relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object

Analysis:
Consider visibility in canBenefitFromLocalAlias function limits the MC's ability to use local reference when selecting operand.

Before this patch, the assemble looks like

        .text
        .file   "test.c"
        .protected      foo                     # -- Begin function foo
        .globl  foo
        .p2align        4, 0x90
        .type   foo,@function
foo:                                    # @foo
.Lfoo$local:
        .cfi_startproc
# %bb.0:                                # %entry
        pushq   %rbp
        .cfi_def_cfa_offset 16
        .cfi_offset %rbp, -16
        movq    %rsp, %rbp
        .cfi_def_cfa_register %rbp
        leaq    .Lfoo$local(%rip), %rax
        popq    %rbp
        .cfi_def_cfa %rsp, 8
        retq
.Lfunc_end0:

.Lfoo$local(%rip) can be calculated directly and is not a relocation when emiting object file, so we no need to worry about the visibility of foo.

After this patch

leaq    .Lfoo$local(%rip), %rax    ->  leaq    foo(%rip), %rax

The visibility of foo conflicts with the relocation type..

I think this introduces an old issue when lowering the instruction.

Thanks for reporting this. I will need to refresh my understanding on this. Use of protected visibility is rare. The gcc bug is unclear... reading though it they haven't come up with a decision as to what behaviour they want yet. As things stand we now seem to match the current gcc behaviour. We could go back to the original behaviour or we could emit a R_X86_64_PLT32 rather than R_X86_64_PC32 (which is probably the more "correct" ELF thing to do). @MaskRay?

MaskRay added a subscriber: xiangzhangllvm.EditedAug 25 2020, 9:12 AM

I think this introduces an old issue when lowering the instruction.

Ref: https://sourceware.org/bugzilla/show_bug.cgi?id=13600

Reproducer:

__attribute__((visibility("protected"))) void * foo (void) { return (void *)foo; }
clang -c  -O0  -fpic test.c 
clang -O0  -fpic test.o  -shared

Error message:
bfd/bin/ld: test.o: relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object

Analysis:
Consider visibility in canBenefitFromLocalAlias function limits the MC's ability to use local reference when selecting operand.

Before this patch, the assemble looks like

        .text
        .file   "test.c"
        .protected      foo                     # -- Begin function foo
        .globl  foo
        .p2align        4, 0x90
        .type   foo,@function
foo:                                    # @foo
.Lfoo$local:
        .cfi_startproc
# %bb.0:                                # %entry
        pushq   %rbp
        .cfi_def_cfa_offset 16
        .cfi_offset %rbp, -16
        movq    %rsp, %rbp
        .cfi_def_cfa_register %rbp
        leaq    .Lfoo$local(%rip), %rax
        popq    %rbp
        .cfi_def_cfa %rsp, 8
        retq
.Lfunc_end0:

.Lfoo$local(%rip) can be calculated directly and is not a relocation when emiting object file, so we no need to worry about the visibility of foo.

After this patch

leaq    .Lfoo$local(%rip), %rax    ->  leaq    foo(%rip), %rax

The visibility of foo conflicts with the relocation type..

tl;dr This is a longstanding GNU ld bug introduced in binutils 2.26

% cat a.c
__attribute__((visibility("protected"))) void * foo (void) { return (void *)foo; }
% gcc -fpic a.c -shared -fuse-ld=bfd # relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object

clang does not behave worse than GCC+GNU ld, so there is no regression on our side.


Longer answer:

binutils 2.26 introduced a regression: R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

The original issue is "Copy relocation against protected symbol doesn't work".
I agree with Rich Felker (https://gcc.gnu.org/ml/gcc/2016-04/msg00168.html) and
Cary Coutant (https://sourceware.org/ml/binutils/2016-03/msg00407.html https://gcc.gnu.org/ml/gcc/2016-04/msg00158.html https://gcc.gnu.org/ml/gcc/2016-04/msg00169.html) that we should
keep using direct access against protected symbols and disallow copy relocations against protected symbols.

I appreciate that Cary Coutant and Rafael Ávila de Espíndola added diagnostics to gold and lld, respectively:

@skan Perhaps you are in a good position to change the resolutions to the following issues:)

GCC 5 x86-64 introduced a regression (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248) I suggested a fix in May, 2019: https://sourceware.org/pipermail/gcc/2019-May/229309.html
i386 was flagged as a reproduce (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55012)

__attribute__((visibility("protected"))) int a;
int foo() { return a; } // GCC>=5 uses R_X86_64_GOTPCREL/R_X86_64_REX_GOTPCRELX instead of R_X86_64_PC32

binutils 2.26 introduced a regression R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

tl;dr This is a longstanding GNU ld bug introduced in binutils 2.26

% cat a.c
__attribute__((visibility("protected"))) void * foo (void) { return (void *)foo; }
% gcc -fpic a.c -shared -fuse-ld=bfd # relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object

clang does not behave worse than GCC+GNU ld, so there is no regression on our side.

Sorry I was in a rush and made a mistake in my reply when I mentioned PLT relocations. That was just a mistake.. I meant to say that we could access via the GOT.

Thanks @MaskRay - awesome reply.

skan added a comment.EditedAug 26 2020, 1:35 AM

Longer answer:

binutils 2.26 introduced a regression: R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

The original issue is "Copy relocation against protected symbol doesn't work".
I agree with Rich Felker (https://gcc.gnu.org/ml/gcc/2016-04/msg00168.html) and
Cary Coutant (https://sourceware.org/ml/binutils/2016-03/msg00407.html https://gcc.gnu.org/ml/gcc/2016-04/msg00158.html https://gcc.gnu.org/ml/gcc/2016-04/msg00169.html) that we should
keep using direct access against protected symbols and disallow copy relocations against protected symbols.

I appreciate that Cary Coutant and Rafael Ávila de Espíndola added diagnostics to gold and lld, respectively:

@skan Perhaps you are in a good position to change the resolutions to the following issues:)

GCC 5 x86-64 introduced a regression (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248) I suggested a fix in May, 2019: https://sourceware.org/pipermail/gcc/2019-May/229309.html
i386 was flagged as a reproduce (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55012)

__attribute__((visibility("protected"))) int a;
int foo() { return a; } // GCC>=5 uses R_X86_64_GOTPCREL/R_X86_64_REX_GOTPCRELX instead of R_X86_64_PC32

binutils 2.26 introduced a regression R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

Thanks, this information is helpful to me. @MaskRay What's resolution do you want me to change?
"thus I suggest we make HAVE_LD_PIE_COPYRELOC non-default" this one?

@hans, I think that we should put this change onto the release branch for llvm11.

hans added a comment.Sep 15 2020, 6:09 AM

@hans, I think that we should put this change onto the release branch for llvm11.

I'm not familiar with the details of this patch, can you explain why we should merge it to llvm11?

We're very late in the release process, so unless it's fixing some major problem, I would be hesitant to merge.

@hans, I think that we should put this change onto the release branch for llvm11.

I'm not familiar with the details of this patch, can you explain why we should merge it to llvm11?

We're very late in the release process, so unless it's fixing some major problem, I would be hesitant to merge.

Apologies - I failed to consider where LLVM is in the release process!

This isn't needed to fix a major problem. However, it would be great to get this patch into a released LLVM ASAP to prevent users coming to rely on the behaviour prior to this patch. See report by @skan above. The patch should be very safe as it essentially restores the referencing behaviour to what it was prior to https://reviews.llvm.org/D73230 for symbol definitions that do not have default ELF visibility.

If you have to do another RC it would be worth considering merging this.

@hans, I think that we should put this change onto the release branch for llvm11.

I'm not familiar with the details of this patch, can you explain why we should merge it to llvm11?

We're very late in the release process, so unless it's fixing some major problem, I would be hesitant to merge.

Apologies - I failed to consider where LLVM is in the release process!

This isn't needed to fix a major problem. However, it would be great to get this patch into a released LLVM ASAP to prevent users coming to rely on the behaviour prior to this patch. See report by @skan above. The patch should be very safe as it essentially restores the referencing behaviour to what it was prior to https://reviews.llvm.org/D73230 for symbol definitions that do not have default ELF visibility.

I testify that this patch is safe. With this patch the behavior is closer to GCC.

(skan's report is a GNU ld x86 bug and has been always the case in GCC x86 + GNU ld circa 2015)

If you have to do another RC it would be worth considering merging this.