[ELF] - Drop SHF_LINK_ORDER flag from the output.
AbandonedPublic

Authored by grimar on Mar 12 2018, 4:48 AM.

Details

Summary

It should be useless to leave SHF_LINK_ORDER flag in the
non-relocatable output because this flag is used only by the linker.
This patch removes it to make output cleaner.

I had to do an exception for SHT_ARM_EXIDX sections though
because otherwise, GNU strip tool reports a
"sh_link not set for section `.ARM.exidx'" warning.
This warning itself seems wrong/useless to me.

Diff Detail

grimar created this revision.Mar 12 2018, 4:48 AM

Looks ok to me. I agree that the GNU strip warning is not helpful for executables or shared libraries. When I initially put in support for SHT_ARM_EXIDX I left the flag in as I remembered seeing one of the gnu tools give such a warning but I couldn't remember which one, looks like it was strip.

Not sure this is worth it.

Even having a section table on output files is technically optional. In
practice it is necessary for static linkers, but anything other than the
symbol table is there for the benefit of a human looking at the output.

As such, SHF_LINK_ORDER has some meaning: The input sections had this
flag set. In my opinion it is OK to drop it or keep it, so we should do
whatever is simpler.

Cheers,
Rafael

George Rimar via Phabricator via llvm-commits
<llvm-commits@lists.llvm.org> writes:

grimar created this revision.
grimar added reviewers: ruiu, psmith, espindola.
Herald added subscribers: kristof.beyls, arichardson, emaste.

It should be useless to leave SHF_LINK_ORDER flag in the
non-relocatable output because this flag is used only by the linker.
This patch removes it to make output cleaner.

I had to do an exception for SHT_ARM_EXIDX sections though
because otherwise, GNU strip tool reports a
"sh_link not set for section `.ARM.exidx'" warning.
This warning itself seems wrong/useless to me.

https://reviews.llvm.org/D44377

Files:

ELF/Writer.cpp
test/ELF/link-order-flag.s

Index: test/ELF/link-order-flag.s
===================================================================

  • test/ELF/link-order-flag.s +++ test/ELF/link-order-flag.s @@ -0,0 +1,31 @@ +# REQUIRES: x86 +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o + + Check we keep SHF_LINK_ORDER flag for -r +# RUN: ld.lld -r %t.o -o %t +# RUN: llvm-readobj -s %t | FileCheck %s --check-prefix=NODROP +# NODROP: Section { +# NODROP: Index: +# NODROP: Name: .bar +# NODROP-NEXT: Type: SHT_PROGBITS +# NODROP-NEXT: Flags [ +# NODROP-NEXT: SHF_ALLOC +# NODROP-NEXT: SHF_LINK_ORDER +# NODROP-NEXT: ] + + Check we drop SHF_LINK_ORDER flag for regular output. +# RUN: ld.lld %t.o -o %t +# RUN: llvm-readobj -s %t | FileCheck %s --check-prefix=DROP +# DROP: Section { +# DROP: Index: +# DROP: Name: .bar +# DROP-NEXT: Type: SHT_PROGBITS +# DROP-NEXT: Flags [ +# DROP-NEXT: SHF_ALLOC +# DROP-NEXT: ] + +.section .foo,"a" +.quad 0 + +.section .bar,"ao",@progbits,.foo +.quad 0 Index: ELF/Writer.cpp ===================================================================
  • ELF/Writer.cpp +++ ELF/Writer.cpp @@ -1351,6 +1351,13 @@ for (BaseCommand *Base : Sec->SectionCommands) if (auto *ISD = dyn_cast<InputSectionDescription>(Base)) llvm::erase_if(ISD->Sections, [](InputSection *IS) { return !IS; }); + + In theory, there should not make sense to have this flag in the + non-relocatable output because the flag is used by linker only. + Binutils strip tool reports a warning when sh_link is not set for + SHT_ARM_EXIDX though. So we have to keep it to workaround warning. + if (!Config->Relocatable && Sec->Type != SHT_ARM_EXIDX) + Sec->Flags &= ~SHF_LINK_ORDER; } }

Index: test/ELF/link-order-flag.s
===================================================================

  • test/ELF/link-order-flag.s +++ test/ELF/link-order-flag.s @@ -0,0 +1,31 @@ +# REQUIRES: x86 +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o + + Check we keep SHF_LINK_ORDER flag for -r +# RUN: ld.lld -r %t.o -o %t +# RUN: llvm-readobj -s %t | FileCheck %s --check-prefix=NODROP +# NODROP: Section { +# NODROP: Index: +# NODROP: Name: .bar +# NODROP-NEXT: Type: SHT_PROGBITS +# NODROP-NEXT: Flags [ +# NODROP-NEXT: SHF_ALLOC +# NODROP-NEXT: SHF_LINK_ORDER +# NODROP-NEXT: ] + + Check we drop SHF_LINK_ORDER flag for regular output. +# RUN: ld.lld %t.o -o %t +# RUN: llvm-readobj -s %t | FileCheck %s --check-prefix=DROP +# DROP: Section { +# DROP: Index: +# DROP: Name: .bar +# DROP-NEXT: Type: SHT_PROGBITS +# DROP-NEXT: Flags [ +# DROP-NEXT: SHF_ALLOC +# DROP-NEXT: ] + +.section .foo,"a" +.quad 0 + +.section .bar,"ao",@progbits,.foo +.quad 0 Index: ELF/Writer.cpp ===================================================================
  • ELF/Writer.cpp +++ ELF/Writer.cpp @@ -1351,6 +1351,13 @@ for (BaseCommand *Base : Sec->SectionCommands) if (auto *ISD = dyn_cast<InputSectionDescription>(Base)) llvm::erase_if(ISD->Sections, [](InputSection *IS) { return !IS; }); + + In theory, there should not make sense to have this flag in the + non-relocatable output because the flag is used by linker only. + Binutils strip tool reports a warning when sh_link is not set for + SHT_ARM_EXIDX though. So we have to keep it to workaround warning. + if (!Config->Relocatable && Sec->Type != SHT_ARM_EXIDX) + Sec->Flags &= ~SHF_LINK_ORDER; } }

    _______________________________________________ llvm-commits mailing list llvm-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
grimar abandoned this revision.Mar 14 2018, 1:51 AM

Not sure this is worth it.

Even having a section table on output files is technically optional. In
practice it is necessary for static linkers, but anything other than the
symbol table is there for the benefit of a human looking at the output.

As such, SHF_LINK_ORDER has some meaning: The input sections had this
flag set. In my opinion it is OK to drop it or keep it, so we should do
whatever is simpler.

Cheers,
Rafael

Ok.