[ELF] Linkerscript: allow adding start/end symbols to arbitrary section

[evgeny777]

Summary

Current revision does not allow adding symbols to special output sections, like .eh_frame, .eh_frame_hdr and others.
This patch implements this feature (which is required by our dynamic linker)

[ruiu]

This patch seems a bit tricky because it handles symbols at start or end of sections command specially. For example, it works as expected if a command is

.eh_frame_hdr { _foo = .; *(.eh_frame_hdr); _bar = .; }

but doesn't if

.eh_frame_hdr { _foo = .; *(.eh_frame_hdr); _bar1 = .; bar2 = .; }

I can see why you needed to do this (because special output sections are not a list of output sections), but I'm wondering if there's a better way than the way you implemented in this patch.

[evgeny777]

I agree, but unfortunately I don't see a lot of other options. What comes to my mind is:

• Implement more sophisticated logic, so it would handle your case.
• Check for symbol names (section name + "_end" or "_start"), and if there are then add start and/or end symbols.

Any other suggestions are appreciated, of course.

[evgeny777]

An attempt to make it slightly more clever.

[ruiu]

ELF/LinkerScript.cpp

313 ↗

(On Diff #68831)

I agree we need this function, but this is tricky, so this needs comments.

316 ↗

(On Diff #68831)

Add is used only once, so please inline.

[evgeny777]

Added support for expressions (see test case)

[evgeny777]

Addressed review comments

[ruiu]

ELF/LinkerScript.cpp

314 ↗

(On Diff #68838)

I'd also mention that special sections are not a list of regular output sections and therefore the regular mechanism to define symbols doesn't work. It's also good to mention that this approach is not perfect -- it only handles start and end symbols.

321 ↗

(On Diff #68838)

Use isa instead of dyn_cast.

321–324 ↗

(On Diff #68838)

Let's reduce the indentation level by using early continues.

if (isa<InputSectionDescription>(...)) {
  Start = false;
  continue;
}
auto *AssignCmd = dyn_cast...;
if (!AssignCmd)
  continue;

[evgeny777]

Addressed review comments

[ruiu]

ELF/LinkerScript.cpp

316 ↗

(On Diff #68841)

Format.

333–335 ↗

(On Diff #68841)

If an output section is defined as

.eh_frame { start = .; foo.o(.eh_frame); bar = .; bar.o(.eh_frame); end = . }

then bar is silently set so that it points to the end of .eh_frame section? I think we want to print an error message for that instead.

[evgeny777]

Added error checking + test case for error.

[evgeny777]

Rebased.

[evgeny777]

Fixed bug in addStartEndSymbols() which caused the following script to fail

.eh_frame_hdr : {
   *(.eh_frame_hdr)
   *(.eh_frame_hdr)
}

[ruiu]

ELF/LinkerScript.cpp

321 ↗

(On Diff #69490)

It seems unnecessarily complicated to me. Why don't you do something like this?

// Add start symbols.
for (int I = 0, E = Cmd->Commands.size(); I < E; ++I) {
  if (!isa<SymbolAssignment>(Cmd->Commands[I]))
    break;
  ....
}

// Add end symbols.
for (int I = Cmd->Commands.size() - 1; I >= 0; --I) {
  if (!isa<SymbolAssignment>(Cmd->Commands[I]))
    break;
  ....
}

[evgeny777]

ELF/LinkerScript.cpp

321 ↗

(On Diff #69490)

That's fine, but what about error diagnostics, you suggested to add last time?

[ruiu]

ELF/LinkerScript.cpp

321 ↗

(On Diff #69490)

Well, maybe something like this?

 // Add start symbols.
 int I = 0;
 for (int E = Cmd->Commands.size(); I < E; ++I) {
   if (!isa<SymbolAssignment>(Cmd->Commands[I]))
     break;
   ....
 }

 // Add end symbols.
 int J = Cmd->Commands.size() - 1;
 for (; J >= 0; --J) {
   if (!isa<SymbolAssignment>(Cmd->Commands[J]))
     break;
   ....
 }

// Warn on stray assignment expressions.
for (; I <= J; ++I)
  if (auto *Foo = dyn_cast<SymbolAssignment>(Cmd->Commands[I])
    ....

[evgeny777]

ELF/LinkerScript.cpp

321 ↗

(On Diff #69490)

The problem in this implementation is that it add 'end' symbols in reverse order. The following one will not work:

.eh_frame_hdr : {
    foo = .;
    *(.eh_frame_hdr)
    bar1 = .;
    bar2 = bar1 + 1;
}

[evgeny777]

Diff updated.
I've simplified the 'addStartEndSymbols' function and hope it's easier to understand now.

[ruiu]

LGTM

lld/trunk/ELF/LinkerScript.cpp

for (InputSectionBase<ELFT> *S : F->getSections()) {
       std::tie(OutSec, IsNew) = Factory.create(S, getOutputSectionName(S));
       if (IsNew)
         OutputSections->push_back(OutSec);
       OutSec->addSection(S);
     }
   }
 }
 
-template <class ELFT> void assignOffsets(OutputSectionBase<ELFT> *Sec) {
+// Linker script may define start and end symbols for special section types,
+// like .got, .eh_frame_hdr, .eh_frame and others. Those sections are not a list
+// of regular input input sections, therefore our way of defining symbols for
+// regular sections will not work. The approach we use for special section types
+// is not perfect - it handles only start and end symbols.
+template <class ELFT>
+void addStartEndSymbols(OutputSectionCommand *Cmd,
+                        OutputSectionBase<ELFT> *Sec) {
+  bool Start = true;
+  BaseCommand *PrevCmd = nullptr;
+
+  for (std::unique_ptr<BaseCommand> &Base : Cmd->Commands) {
+    if (auto *AssignCmd = dyn_cast<SymbolAssignment>(Base.get())) {
+      if (auto *Sym = cast_or_null<DefinedSynthetic<ELFT>>(AssignCmd->Sym)) {
+        Sym->Section = Sec;
+        Sym->Value =
+            AssignCmd->Expression(Sec->getVA() + (Start ? 0 : Sec->getSize())) -
+            Sec->getVA();
+      }
+    } else {
+      if (!Start && isa<SymbolAssignment>(PrevCmd))
+        error("section '" + Sec->getName() +
+              "' supports only start and end symbols");
+      Start = false;
+    }
+    PrevCmd = Base.get();
+  }
+}
+
+template <class ELFT>
+void assignOffsets(OutputSectionCommand *Cmd, OutputSectionBase<ELFT> *Sec) {
   auto *OutSec = dyn_cast<OutputSection<ELFT>>(Sec);
   if (!OutSec) {
     Sec->assignOffsets();
+    // This section is not regular output section. However linker script may
+    // have defined start/end symbols for it. This case is handled below.
+    addStartEndSymbols(Cmd, Sec);
     return;
   }
 
   typedef typename ELFT::uint uintX_t;
   uintX_t Off = 0;
 
   for (InputSection<ELFT> *I : OutSec->Sections) {
     if (auto *L = dyn_cast<LayoutInputSection<ELFT>>(I)) {
for (const std::unique_ptr<BaseCommand> &Base : Opt.Commands) {
 
     if (Cmd->AlignExpr)
       Sec->updateAlignment(Cmd->AlignExpr(Dot));
 
     if ((Sec->getFlags() & SHF_TLS) && Sec->getType() == SHT_NOBITS) {
       uintX_t TVA = Dot + ThreadBssOffset;
       TVA = alignTo(TVA, Sec->getAlignment());
       Sec->setVA(TVA);
-      assignOffsets(Sec);
+      assignOffsets(Cmd, Sec);
       ThreadBssOffset = TVA - Dot + Sec->getSize();
       continue;
     }
 
     if (!(Sec->getFlags() & SHF_ALLOC)) {
-      Sec->assignOffsets();
+      assignOffsets(Cmd, Sec);
       continue;
     }
 
     Dot = alignTo(Dot, Sec->getAlignment());
     Sec->setVA(Dot);
-    assignOffsets(Sec);
+    assignOffsets(Cmd, Sec);
     MinVA = std::min(MinVA, Dot);
     Dot += Sec->getSize();
   }
 
   // ELF and Program headers need to be right before the first section in
   // memory. Set their addresses accordingly.
   MinVA = alignDown(MinVA - Out<ELFT>::ElfHeader->getSize() -
                         Out<ELFT>::ProgramHeaders->getSize(),

lld/trunk/test/ELF/linkerscript/linkerscript-symbols-synthetic.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
-
 # Simple symbol assignment within input section list. The '.' symbol
 # is not location counter but offset from the beginning of output
 # section .foo
 # RUN: echo "SECTIONS { \
 # RUN:          .foo : { \
 # RUN:              begin_foo = .; \
-# RUN:              PROVIDE(_begin_foo = .); \
+# RUN:              PROVIDE(_begin_sec = .); \
 # RUN:              *(.foo) \
 # RUN:              end_foo = .; \
-# RUN:              PROVIDE_HIDDEN(_end_foo = .); \
+# RUN:              PROVIDE_HIDDEN(_end_sec = .); \
 # RUN:              size_foo_1 = SIZEOF(.foo); \
 # RUN:              . = ALIGN(0x1000); \
 # RUN:              begin_bar = .; \
 # RUN:              *(.bar) \
 # RUN:              end_bar = .; \
-# RUN:              size_foo_2 = SIZEOF(.foo); \ 
-# RUN:            } \
-# RUN:            size_foo_3 = SIZEOF(.foo); }" > %t.script
-# RUN: ld.lld -o %t1 --script %t.script %t
+# RUN:              size_foo_2 = SIZEOF(.foo); } \
+# RUN:          size_foo_3 = SIZEOF(.foo); \
+# RUN:          .eh_frame_hdr : { \
+# RUN:             __eh_frame_hdr_start = .; \
+# RUN:             __eh_frame_hdr_start2 = ALIGN(0x10); \
+# RUN:             *(.eh_frame_hdr) \
+# RUN:             __eh_frame_hdr_end = .; \
+# RUN:             __eh_frame_hdr_end2 = ALIGN(0x10); } \
+# RUN:       }" > %t.script
+# RUN: ld.lld -o %t1 --eh-frame-hdr --script %t.script %t
 # RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=SIMPLE %s
 
-# SIMPLE:      0000000000000128         .foo    00000000 .hidden _end_foo
-# SIMPLE:      0000000000000120         .foo    00000000 _begin_foo
-# SIMPLE-NEXT: 0000000000000120         .foo    00000000 begin_foo
-# SIMPLE-NEXT: 0000000000000128         .foo    00000000 end_foo
+# The script below contains symbols in the middle of .eh_frame_hdr section.
+# We don't support this.
+# RUN: echo "SECTIONS { \
+# RUN:          .eh_frame_hdr : { \
+# RUN:             PROVIDE_HIDDEN(_begin_sec = .); \
+# RUN:             __eh_frame_hdr_start = .; \
+# RUN:             *(.eh_frame_hdr) \
+# RUN:             __eh_frame_hdr_end = .; \
+# RUN:             *(.eh_frame_hdr) } \
+# RUN:             PROVIDE_HIDDEN(_end_sec = .); \
+# RUN:         }" > %t.script
+# RUN: not ld.lld -o %t1 --eh-frame-hdr --script %t.script %t 2>&1 | FileCheck --check-prefix=ERROR %s
+
+# Check that the following script is processed without errors
+# RUN: echo "SECTIONS { \
+# RUN:          .eh_frame_hdr : { \
+# RUN:             PROVIDE_HIDDEN(_begin_sec = .); \
+# RUN:             *(.eh_frame_hdr) \
+# RUN:             *(.eh_frame_hdr) \
+# RUN:             PROVIDE_HIDDEN(_end_sec = .); } \
+# RUN:         }" > %t.script
+# RUN: ld.lld -o %t1 --eh-frame-hdr --script %t.script %t
+
+# SIMPLE:      0000000000000160         .foo    00000000 .hidden _end_sec
+# SIMPLE:      0000000000000158         .foo    00000000 _begin_sec
+# SIMPLE-NEXT: 0000000000000158         .foo    00000000 begin_foo
+# SIMPLE-NEXT: 0000000000000160         .foo    00000000 end_foo
 # SIMPLE-NEXT: 0000000000000008         .foo    00000000 size_foo_1
 # SIMPLE-NEXT: 0000000000001000         .foo    00000000 begin_bar
 # SIMPLE-NEXT: 0000000000001004         .foo    00000000 end_bar
-# SIMPLE-NEXT: 0000000000000ee4         .foo    00000000 size_foo_2
-# SIMPLE-NEXT: 0000000000000ee4         *ABS*   00000000 size_foo_3
+# SIMPLE-NEXT: 0000000000000eac         .foo    00000000 size_foo_2
+# SIMPLE-NEXT: 0000000000000eac         *ABS*   00000000 size_foo_3
+# SIMPLE-NEXT: 0000000000001004         .eh_frame_hdr     00000000 __eh_frame_hdr_start
+# SIMPLE-NEXT: 0000000000001010         .eh_frame_hdr     00000000 __eh_frame_hdr_start2
+# SIMPLE-NEXT: 0000000000001018         .eh_frame_hdr     00000000 __eh_frame_hdr_end
+# SIMPLE-NEXT: 0000000000001020         .eh_frame_hdr     00000000 __eh_frame_hdr_end2
+# ERROR: section '.eh_frame_hdr' supports only start and end symbols
 
 .global _start
 _start:
  nop
 
 .section .foo,"a"
  .quad 0
 
 .section .bar,"a"
  .long 0
 
-.global _begin_foo, _end_foo
+.section .dah,"ax",@progbits
+ .cfi_startproc
+ nop
+ .cfi_endproc
+
+.global _begin_sec, _end_sec