This is an archive of the discontinued LLVM Phabricator instance.

Differential D44377

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

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

Details

Reviewers
ruiu
psmith
• espindola
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

Event Timeline

grimar created this revision.Mar 12 2018, 4:48 AM
Herald added subscribers: kristof.beyls, arichardson, emaste. · View Herald TranscriptMar 12 2018, 4:48 AM
peter.smith added a subscriber: peter.smith.Mar 12 2018, 5:52 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.

• rafael added a subscriber: • rafael.Mar 13 2018, 11:42 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

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
In D44377#1036266, @rafael wrote:

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.

Revision Contents

PathSize
ELF/
7 lines
test/
ELF/
31 lines

Diff 137986

ELF/Writer.cpp

Show First 20 Lines • Show All 1,345 Lines • ▼ Show 20 Lines for (OutputSection *Sec : OutputSections) {
for (int I = 0, N = Sections.size(); I < N; ++I) for (int I = 0, N = Sections.size(); I < N; ++I)
*ScriptSections[I] = Sections[I]; *ScriptSections[I] = Sections[I];
// Remove the Sections we marked as duplicate earlier. // Remove the Sections we marked as duplicate earlier.
for (BaseCommand *Base : Sec->SectionCommands) for (BaseCommand *Base : Sec->SectionCommands)
if (auto *ISD = dyn_cast<InputSectionDescription>(Base)) if (auto *ISD = dyn_cast<InputSectionDescription>(Base))
llvm::erase_if(ISD->Sections, [](InputSection *IS) { return !IS; }); 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;
} }
} }
static void applySynthetic(const std::vector<SyntheticSection *> &Sections, static void applySynthetic(const std::vector<SyntheticSection *> &Sections,
std::function<void(SyntheticSection *)> Fn) { std::function<void(SyntheticSection *)> Fn) {
for (SyntheticSection *SS : Sections) for (SyntheticSection *SS : Sections)
if (SS && SS->getParent() && !SS->empty()) if (SS && SS->getParent() && !SS->empty())
Fn(SS); Fn(SS);
▲ Show 20 Lines • Show All 881 Lines • Show Last 20 Lines

test/ELF/link-order-flag.s

# 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