Handle full path and archive library names in section placement rules

[dmikulin]

Summary

Currently lld uses base names of files to match against file patterns in SECTION rules. This patch extends it to use a fully specified file name as it appears in --trace output to match agains, i.e, "<path>/<objname>.o" or "<path>/<libname>.a(<objname>.o)".

I had to leave matching against a base name in place not to break existing functionality, specifically PR29115, which added matching of non-wildcard file names.

[ruiu]

lld/ELF/LinkerScript.cpp

186 ↗

(On Diff #112231)

Does this "or"-behavior match with GNU linker?

[dmikulin]

Replaced the use of toString with InputFile::getQualifiedName.
Removed base name matching to more closely align with GNU ld way of processing file patterns.
Changed tests cases to reflect source changes.

[ruiu]

lld/ELF/InputFiles.cpp

43 ↗

(On Diff #112475)

I wonder if this is a premature optimization. Can you avoid caching at least for now? You can move this code to filename.

[dmikulin]

lld/ELF/InputFiles.cpp

43 ↗

(On Diff #112475)

It's done this way in toString(), I was thinking of using the same cache, but wasn't sure how separate these two interfaces need to be.

[ruiu]

lld/ELF/InputFiles.cpp

43 ↗

(On Diff #112475)

Caching is not part of an interface. It's hidden from users of this function. So, I'm not worried about "separation" of two interfaces.

[dmikulin]

lld/ELF/InputFiles.cpp

43 ↗

(On Diff #112475)

Are you saying that caching is fine and that I can use the same cache for both toString() and getQualifiedName() ?
Re-creating the same string on every call seems a bit wasteful... I have a modified patch with no caching if that's what you feel is a better alternative.

[ruiu]

lld/ELF/InputFiles.cpp

43 ↗

(On Diff #112475)

Doing the same thing but without caching is what I want this moment, because I don't want to optimize the code unless it is proved to be needed. Also, you can remove this function entirely and instead add code to filename function in LinkerScript.cpp.

[dmikulin]

Removed string caching.

[ruiu]

lld/ELF/LinkerScript.cpp

178 ↗

(On Diff #112592)

I'd flip it and return early to reduce indentation.

if (!S->File)
  return "";

181 ↗

(On Diff #112592)

No else since the last if ends with return.

lld/test/ELF/linkerscript/filename-spec.s

65–67 ↗

(On Diff #112592)

I applied this patch to verify that this behavior matches ld.bfd. What I found is that this doesn't seem to match with ld.bfd, as it produces FIRSTSECOND instead of SECONDFIRST if I replace ld.lld with ld.bfd.

[dmikulin]

lld/ELF/LinkerScript.cpp

181 ↗

(On Diff #112592)

I'll update the patch

lld/test/ELF/linkerscript/filename-spec.s

65–67 ↗

(On Diff #112592)

As far as I can tell, bfd linker does not match against archive library names.
gold produces SECONDFIRST, which makes more sense.

[ruiu]

lld/test/ELF/linkerscript/filename-spec.s

65–67 ↗

(On Diff #112592)

OK, but I believe ld.bfd is still the gold standard and we want to make our linker compatible with ld.bfd instead of ld.gold.

[dmikulin]

Addressed review comments

[dmikulin]

lld/test/ELF/linkerscript/filename-spec.s

65–67 ↗

(On Diff #112592)

But what ld.bfd does is not very logically consistent: it matches against directory names when a .o is specified on the link line, but only uses base name if the same file appears in a .a. It's quite possibly unintentional and can be treated as a bug in ld.bfd.

[ruiu]

LGTM

lld/ELF/LinkerScript.cpp

180 ↗

(On Diff #112598)

Remove this blank line.

lld/test/ELF/linkerscript/filename-spec.s

65–67 ↗

(On Diff #112592)

If I replace ld.lld with ld.gold, all tests indeed pass. So this is not a good situation; GNU linkers are not compatible each other, and we have to choose one of them. In such situation like this, we usually copy bfd's behavior, but as you said, gold's behavior seems more logical.

This feature doesn't seem to have been tested well. When I fed "SECTIONS { .foo : { *lib2* *lib1* } }" to gold, it crashed. Wow.

Perhaps the fact that ld.gold and ld.bfd are not compatible means that this incompatibility is not important. So I think I'm fine with gold's behavior.

[MikeStump]

The GNU ld documentation says that patterns are archive:file in:

https://sourceware.org/binutils/docs/ld/Input-Section-Basics.html#Input-Section-Basics

and this patch doesn't use the : spelling, so archive:file doesn't work. If archive:file is used, then one could use archive:file in the patterns as documented.

lld/trunk/ELF/LinkerScript.cpp

 bool AssertCommand::classof(const BaseCommand *C) {
   return C->Kind == AssertKind;
 }
 
 bool BytesDataCommand::classof(const BaseCommand *C) {
   return C->Kind == BytesDataKind;
 }
 
-static StringRef basename(InputSectionBase *S) {
-  if (S->File)
-    return sys::path::filename(S->File->getName());
+static std::string filename(InputSectionBase *S) {
+  if (!S->File)
     return "";
+  if (S->File->ArchiveName.empty())
+    return S->File->getName();
+  return (S->File->ArchiveName + "(" + S->File->getName() + ")").str();
 }
 
 bool LinkerScript::shouldKeep(InputSectionBase *S) {
-  for (InputSectionDescription *ID : Opt.KeptSections)
-    if (ID->FilePat.match(basename(S)))
+  for (InputSectionDescription *ID : Opt.KeptSections) {
+    std::string Filename = filename(S);
+    if (ID->FilePat.match(Filename))
       for (SectionPattern &P : ID->SectionPatterns)
         if (P.SectionPat.match(S->Name))
           return true;
+  }
   return false;
 }
 
 // A helper function for the SORT() command.
 static std::function<bool(InputSectionBase *, InputSectionBase *)>
 getComparator(SortSectionPolicy K) {
   switch (K) {
   case SortSectionPolicy::Alignment:
for (InputSectionBase *Sec : InputSections) {
       }
 
       // For -emit-relocs we have to ignore entries like
       //   .rela.dyn : { *(.rela.data) }
       // which are common because they are in the default bfd script.
       if (Sec->Type == SHT_REL || Sec->Type == SHT_RELA)
         continue;
 
-      StringRef Filename = basename(Sec);
+      std::string Filename = filename(Sec);
       if (!Cmd->FilePat.match(Filename) ||
           Pat.ExcludedFilePat.match(Filename) ||
           !Pat.SectionPat.match(Sec->Name))
         continue;
 
       Ret.push_back(cast<InputSection>(Sec));
       Sec->Assigned = true;
     }

lld/trunk/test/ELF/linkerscript/filename-spec.s

 # RUN: llvm-objdump -s %t3 | FileCheck --check-prefix=FIRSTSECOND %s
 
 # RUN: echo "SECTIONS { .foo : { \
 # RUN:   *second.o(.foo) \
 # RUN:   *first.o(.foo) } }" > %t4.script
 # RUN: ld.lld -o %t4 --script %t4.script %tfirst.o %tsecond.o
 # RUN: llvm-objdump -s %t4 | FileCheck --check-prefix=SECONDFIRST %s
 
-# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %T/filename-spec1.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o filename-spec1.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux \
-# RUN:   %p/Inputs/filename-spec.s -o %T/filename-spec2.o
+# RUN:   %p/Inputs/filename-spec.s -o filename-spec2.o
 
 # RUN: echo "SECTIONS { .foo : { \
 # RUN:   filename-spec2.o(.foo) \
 # RUN:   filename-spec1.o(.foo) } }" > %t5.script
 # RUN: ld.lld -o %t5 --script %t5.script \
-# RUN:   %T/filename-spec1.o %T/filename-spec2.o
+# RUN:   filename-spec1.o filename-spec2.o
 # RUN: llvm-objdump -s %t5 | FileCheck --check-prefix=SECONDFIRST %s
 
 # RUN: echo "SECTIONS { .foo : { \
 # RUN:   filename-spec1.o(.foo) \
 # RUN:   filename-spec2.o(.foo) } }" > %t6.script
 # RUN: ld.lld -o %t6 --script %t6.script \
-# RUN:   %T/filename-spec1.o %T/filename-spec2.o
+# RUN:   filename-spec1.o filename-spec2.o
 # RUN: llvm-objdump -s %t6 | FileCheck --check-prefix=FIRSTSECOND %s
 
+# RUN: mkdir -p %t.testdir1 %t.testdir2
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.testdir1/filename-spec1.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux \
+# RUN:   %p/Inputs/filename-spec.s -o %t.testdir2/filename-spec2.o
+# RUN: llvm-ar rsc %t.testdir1/lib1.a %t.testdir1/filename-spec1.o
+# RUN: llvm-ar rsc %t.testdir2/lib2.a %t.testdir2/filename-spec2.o
+
+# Verify matching of archive library names.
+# RUN: echo "SECTIONS { .foo : { \
+# RUN:   *lib2*(.foo) \
+# RUN:   *lib1*(.foo) } }" > %t7.script
+# RUN: ld.lld -o %t7 --script %t7.script --whole-archive \
+# RUN:   %t.testdir1/lib1.a %t.testdir2/lib2.a
+# RUN: llvm-objdump -s %t7 | FileCheck --check-prefix=SECONDFIRST %s
+
+# Verify matching directories.
+# RUN: echo "SECTIONS { .foo : { \
+# RUN:   *testdir2*(.foo) \
+# RUN:   *testdir1*(.foo) } }" > %t8.script
+# RUN: ld.lld -o %t8 --script %t8.script --whole-archive \
+# RUN:   %t.testdir1/lib1.a %t.testdir2/lib2.a
+# RUN: llvm-objdump -s %t8 | FileCheck --check-prefix=SECONDFIRST %s
+
+# Verify matching of archive library names in KEEP.
+# RUN: echo "SECTIONS { .foo : { \
+# RUN:   KEEP(*lib2*(.foo)) \
+# RUN:   KEEP(*lib1*(.foo)) } }" > %t9.script
+# RUN: ld.lld -o %t9 --script %t9.script --whole-archive \
+# RUN:   %t.testdir1/lib1.a %t.testdir2/lib2.a
+# RUN: llvm-objdump -s %t9 | FileCheck --check-prefix=SECONDFIRST %s
+
+# Verify matching directories in KEEP.
+# RUN: echo "SECTIONS { .foo : { \
+# RUN:   KEEP(*testdir2*(.foo)) \
+# RUN:   KEEP(*testdir1*(.foo)) } }" > %t10.script
+# RUN: ld.lld -o %t10 --script %t10.script --whole-archive \
+# RUN:   %t.testdir1/lib1.a %t.testdir2/lib2.a
+# RUN: llvm-objdump -s %t10 | FileCheck --check-prefix=SECONDFIRST %s
+
 .global _start
 _start:
  nop
 
 .section .foo,"a"
  .quad 1