This is an archive of the discontinued LLVM Phabricator instance.

Handle full path and archive library names in section placement rules

Authored by dmikulin on Aug 22 2017, 3:11 PM.



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.

Diff Detail


Event Timeline

dmikulin created this revision.Aug 22 2017, 3:11 PM
ruiu added inline comments.Aug 22 2017, 3:28 PM
186 ↗(On Diff #112231)

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

dmikulin updated this revision to Diff 112475.Aug 23 2017, 4:17 PM

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 added inline comments.Aug 23 2017, 4:49 PM
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 added inline comments.Aug 24 2017, 8:49 AM
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 added inline comments.Aug 24 2017, 11:28 AM
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 added inline comments.Aug 24 2017, 11:39 AM
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 added inline comments.Aug 24 2017, 11:55 AM
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 updated this revision to Diff 112592.Aug 24 2017, 12:31 PM

Removed string caching.

ruiu added inline comments.Aug 24 2017, 12:40 PM
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.

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 marked 2 inline comments as done.Aug 24 2017, 1:01 PM
dmikulin added inline comments.
181 ↗(On Diff #112592)

I'll update the patch

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 added inline comments.Aug 24 2017, 1:10 PM
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

dmikulin updated this revision to Diff 112598.Aug 24 2017, 1:14 PM
dmikulin marked an inline comment as done.

Addressed review comments

dmikulin added inline comments.Aug 24 2017, 1:28 PM
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 accepted this revision.Aug 24 2017, 1:42 PM


180 ↗(On Diff #112598)

Remove this blank line.

65–67 ↗(On Diff #112592)

If I replace ld.lld with, 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 and ld.bfd are not compatible means that this incompatibility is not important. So I think I'm fine with gold's behavior.

This revision is now accepted and ready to land.Aug 24 2017, 1:42 PM
dmikulin marked an inline comment as done.Aug 24 2017, 2:59 PM
This revision was automatically updated to reflect the committed changes.

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

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.