[ELF] - Fix of 22906, referencing __start or __stop should keep the section from GC.

[grimar]

Summary

This fixes the https://llvm.org/bugs/show_bug.cgi?id=22906 bug.

In GNU Binutils, a reference to start or stop is sufficient to prevent the section from being garbage collected.

This patch is a bit different in behavior: we just do not reclaim sections if their section names are valid C identifiers.
So it does not matter of start/stop symbols are referenced.

[emaste]

ELF/MarkLive.cpp

83 ↗

(On Diff #48667)

Is the input->output section name mapping necessary? Those section names are not valid C identifiers anyhow.

[ed]

Unfortunately, this patch doesn't apply cleanly against LLD 3.8.0-rc2. If you generate a patch against that version of LLD, I'll perform a package build to see if it was fixing the issue I was running in to specifically. Thanks!

[ruiu]

Although I understand the point of this patch, is this significant? Do you need this to link some specific program?

[ed]

In D17502#359151, @ruiu wrote:

Although I understand the point of this patch, is this significant? Do you need this to link some specific program?

Yes. The unit testing binary for CloudABI's C library: https://github.com/NuxiNL/cloudlibc.

Each of the individual unit tests place a structure in a separate section, so they are registered automatically. The unit testing binary's entry point then iterates over all of the structures placed in this section and invokes the callbacks. Relevant sources:

https://github.com/NuxiNL/cloudlibc/blob/master/src/include/testing.h#L74-L101
https://github.com/NuxiNL/cloudlibc/blob/master/src/libc/testing/testing_execute.c#L28-L30

CloudABI recently switched over to LLD on x86-64. The only regression we run in to compared to GNU ld is this specific issue.

[grimar]

In D17502#358468, @ed wrote:

Unfortunately, this patch doesn't apply cleanly against LLD 3.8.0-rc2. If you generate a patch against that version of LLD, I'll perform a package build to see if it was fixing the issue I was running in to specifically. Thanks!

Yes, I tried to port it for LLD 3.8.0-rc2, but found that it is not that easy as seems. There are changes in logic of getOutputSectionName(), for example. In newer code it references linker script methods, in older - internal fields of Writer. So it does not looks possible to easy move this method from Writer class like was done in this patch.
So I am not sure it is worth to implement it for old code as there are too many code changes already, sorry.

ELF/MarkLive.cpp

83 ↗

(On Diff #48667)

I think it is. Thats mirrors logic of

void Writer<ELFT>::addStartStopSymbols(OutputSectionBase<ELFT> *Sec) {
  StringRef S = Sec->getName();
  if (!isValidCIdentifier(S))
    return;
...

which uses output section name to create start/stop identificator name.
Also since we can use linker script to place sections to any output section, it seems to be correct for me.

[emaste]

ELF/MarkLive.cpp

83 ↗

(On Diff #48667)

Ah yes, sorry - I glossed over the arbitrary mapping that might come from the linker script and only looked at the built-in set of .text. etc.

[ruiu]

ELF/MarkLive.cpp

83 ↗

(On Diff #48667)

This still seems to be overkill and would probably be slow. I don't feel that we really have to worry about a combination of start/stop and linker script. If you are using linker scripts, you can easily keep all sections using KEEP command, and you don't really have to depend on start/stop symbols because you can freely define any symbols using linker scripts.

I'd prefer a simpler implementation. You can check only input section names instead of output section names by adding this code to isReserved in MarkLive.cpp.

default:
  StringRef S = Sec->getSectionName();

  // We do not want to reclaim sections if they can be referred
  // by __start_* and __stop_* symbols.
  if (isValidCIdentifier(S))
    return true;

  return S.startswith(".ctors") || S.startswith(".dtors") ||
         S.startswith(".init") || S.startswith(".fini") ||
         S.startswith(".jcr");
}

[grimar]

• Applyed Rui's suggestion.

[grimar]

ELF/MarkLive.cpp

83 ↗

(On Diff #48667)

But don't you want at least to check that there are start_/stop symbols referenced to keep sections alive ?
And also we can check if we are using LS then:

  if (!usingScript() && isValidCIdentifier(S)) {
  if (SymbolBody *B = Symtab->find(("__start_" + S).str()))
    if (B->isUndefined())
      return true;
  if (SymbolBody *B = Symtab->find(("__start_" + S).str()))
    if (B->isUndefined())
      return true;
    return false;
}

Or do you mean Symtab->find() would be slow ? But as far I understand there are not too much sections that are isValidCIdentifiers.

[grimar]

In D17502#358468, @ed wrote:

Unfortunately, this patch doesn't apply cleanly against LLD 3.8.0-rc2. If you generate a patch against that version of LLD, I'll perform a package build to see if it was fixing the issue I was running in to specifically. Thanks!

Hello Ed ! Updated version patch fits LLD 3.8.0-rc2 fine. It is in attachment.

patch.patch123 KBDownload

[ed]

Hi George,

In D17502#360482, @grimar wrote:

Hello Ed ! Updated version patch fits LLD 3.8.0-rc2 fine. It is in attachment.

That is awesome. I've just done some experiments with your patch for 3.8.0-rc2 and it looks like I can get our unit testing binary to link. The unit testing binary also seems to work all right. Thanks a lot!

I've just included this patch into our package building system and switched it back to using LLD on x86-64 instead of using GNU ld:

https://github.com/NuxiNL/cloudabi-ports/commit/2629d0907a2324d1c257e7412485d02f247e2ae6

[ruiu]

LGTM with a nit.

ELF/MarkLive.cpp

90 ↗

(On Diff #48898)

Yes, I wanted to avoid hash table lookup here, and also the code duplication (although it is just 8 lines of code) was my concern. This is a small corner case so I think we want to avoid overdesigning stuff but just make it work with a minimal amount of work.

test/ELF/startstop-gccollect.s

1 ↗

(On Diff #48898)

You want to remove start and stop symbols from the test and just verify that we do not reclaim sections if their section names are valid C identifiers.

[grimar]

test/ELF/startstop-gccollect.s

1 ↗

(On Diff #48898)

That was already done in latest diff, no ? :)
start_/stop_ are just mentioned in comments to explain the behavior logic.

lld/trunk/ELF/MarkLive.cpp

template <class ELFT> static bool isReserved(InputSectionBase<ELFT> *Sec) {
   switch (Sec->getSectionHdr()->sh_type) {
   case SHT_FINI_ARRAY:
   case SHT_INIT_ARRAY:
   case SHT_NOTE:
   case SHT_PREINIT_ARRAY:
     return true;
   default:
     StringRef S = Sec->getSectionName();
+
+    // We do not want to reclaim sections if they can be referred
+    // by __start_* and __stop_* symbols.
+    if (isValidCIdentifier(S))
+      return true;
+
     return S.startswith(".ctors") || S.startswith(".dtors") ||
            S.startswith(".init") || S.startswith(".fini") ||
            S.startswith(".jcr");
   }
 }
 
 // This is the main function of the garbage collector.
 // Starting from GC-root sections, this function visits all reachable

lld/trunk/ELF/OutputSections.h

 template <class ELFT, bool IsRela>
 typename llvm::object::ELFFile<ELFT>::uintX_t
 getLocalRelTarget(const ObjectFile<ELFT> &File,
                   const llvm::object::Elf_Rel_Impl<ELFT, IsRela> &Rel,
                   typename llvm::object::ELFFile<ELFT>::uintX_t Addend);
 
 bool canBePreempted(const SymbolBody *Body, bool NeedsGot);
 
+bool isValidCIdentifier(StringRef S);
+
 // This represents a section in an output file.
 // Different sub classes represent different types of sections. Some contain
 // input sections, others are created by the linker.
 // The writer creates multiple OutputSections and assign them unique,
 // non-overlapping file offsets and VAs.
 template <class ELFT> class OutputSectionBase {
 public:
   typedef typename llvm::object::ELFFile<ELFT>::uintX_t uintX_t;

lld/trunk/ELF/OutputSections.cpp

 using namespace llvm::dwarf;
 using namespace llvm::object;
 using namespace llvm::support::endian;
 using namespace llvm::ELF;
 
 using namespace lld;
 using namespace lld::elf2;
 
+static bool isAlpha(char C) {
+  return ('a' <= C && C <= 'z') || ('A' <= C && C <= 'Z') || C == '_';
+}
+
+static bool isAlnum(char C) { return isAlpha(C) || ('0' <= C && C <= '9'); }
+
+// Returns true if S is valid as a C language identifier.
+bool elf2::isValidCIdentifier(StringRef S) {
+  if (S.empty() || !isAlpha(S[0]))
+    return false;
+  return std::all_of(S.begin() + 1, S.end(), isAlnum);
+}
+
 template <class ELFT>
 OutputSectionBase<ELFT>::OutputSectionBase(StringRef Name, uint32_t Type,
                                            uintX_t Flags)
     : Name(Name) {
   memset(&Header, 0, sizeof(Elf_Shdr));
   Header.sh_type = Type;
   Header.sh_flags = Flags;
 }

lld/trunk/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::addStartEndSymbols() {
   Define("__preinit_array_start", "__preinit_array_end",
          Out<ELFT>::Dynamic->PreInitArraySec);
   Define("__init_array_start", "__init_array_end",
          Out<ELFT>::Dynamic->InitArraySec);
   Define("__fini_array_start", "__fini_array_end",
          Out<ELFT>::Dynamic->FiniArraySec);
 }
 
-static bool isAlpha(char C) {
-  return ('a' <= C && C <= 'z') || ('A' <= C && C <= 'Z') || C == '_';
-}
-
-static bool isAlnum(char C) { return isAlpha(C) || ('0' <= C && C <= '9'); }
-
-// Returns true if S is valid as a C language identifier.
-static bool isValidCIdentifier(StringRef S) {
-  if (S.empty() || !isAlpha(S[0]))
-    return false;
-  return std::all_of(S.begin() + 1, S.end(), isAlnum);
-}
-
 // If a section name is valid as a C identifier (which is rare because of
 // the leading '.'), linkers are expected to define __start_<secname> and
 // __stop_<secname> symbols. They are at beginning and end of the section,
 // respectively. This is not requested by the ELF standard, but GNU ld and
 // gold provide the feature, and used by many programs.
 template <class ELFT>
 void Writer<ELFT>::addStartStopSymbols(OutputSectionBase<ELFT> *Sec) {
   StringRef S = Sec->getName();

lld/trunk/test/ELF/gc-sections-local-sym.s

 // RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
 // RUN: ld.lld %t -o %t2 -shared --gc-sections
 // RUN: llvm-readobj -t -s -section-data %t2 | FileCheck %s
 // REQUIRES: x86
 
 .global foo
 foo:
 
-.section bar,"a"
+.section .bar,"a"
 zed:
 
 // CHECK:      Name: .strtab
 // CHECK-NEXT: Type: SHT_STRTAB
 // CHECK-NEXT: Flags [
 // CHECK-NEXT: ]
 // CHECK-NEXT: Address:
 // CHECK-NEXT: Offset:

lld/trunk/test/ELF/startstop-gccollect.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+
+## Default run: sections foo and bar exist in output
+# RUN: ld.lld %t -o %tout
+# RUN: llvm-objdump -d %tout | FileCheck -check-prefix=DISASM %s
+
+## Check that foo and bar sections are not garbage collected,
+## we do not want to reclaim sections if they can be referred
+## by __start_* and __stop_* symbols.
+# RUN: ld.lld %t --gc-sections -o %tout
+# RUN: llvm-objdump -d %tout | FileCheck -check-prefix=DISASM %s
+
+# DISASM:      _start:
+# DISASM-NEXT:    11000:       90      nop
+# DISASM-NEXT: Disassembly of section foo:
+# DISASM-NEXT: foo:
+# DISASM-NEXT:    11001:       90      nop
+# DISASM-NEXT: Disassembly of section bar:
+# DISASM-NEXT: bar:
+# DISASM-NEXT:    11002:       90      nop
+
+.global _start
+.text
+_start:
+ nop
+
+.section foo,"ax"
+ nop
+
+.section bar,"ax"
+ nop