This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Linkerscript KEEP command.
ClosedPublic

Authored by grimar on Feb 13 2016, 2:43 AM.

Details

Summary

When link-time garbage collection is in use (-gc-sections), it is often useful to mark sections that should not be eliminated. This is accomplished by surrounding an input section's wildcard entry with KEEP().
Patch implements that command.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 47908.Feb 13 2016, 2:43 AM
grimar retitled this revision from to [ELF] - Linkerscript KEEP command..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
ruiu edited edge metadata.Feb 16 2016, 11:56 AM

What's the performance impact of this change? This change made the linker compare all discarded section names against all rules in linker scripts, which can be expensive. Of course that depends on input, but O(n*m) where n is the number of discarded sections and m is the number of section rules is a bit scary.

ELF/LinkerScript.cpp
46–52 ↗(On Diff #47908)

You want to define

template <class ELFT>
SectionRule *LinkerScript::find(InputSectionBase<ELFT> *S) {
  for (SectionRule &R : Sections)
    if (R.match(S))
      return &R;
  return nullptr;
}

and use it both from shouldKeep and from getOutputSection.

384 ↗(On Diff #47908)

Why did you change this?

389–396 ↗(On Diff #47908)

This should be a regular member function rather than a closure.

413 ↗(On Diff #47908)

Please use setError() in the linker script as other code does.

grimar updated this revision to Diff 48164.Feb 17 2016, 2:33 AM
grimar edited edge metadata.
grimar marked 3 inline comments as done.

Addressed Rui's comments.

In D17242#353765, @ruiu wrote:

What's the performance impact of this change? This change made the linker compare all discarded section names against all rules in linker scripts, which can be expensive. Of course that depends on input, but O(n*m) where n is the number of discarded sections and m is the number of section rules is a bit scary.

That really depends I guess. Should be tested on real app that uses the features of LS. Currently I think it is not in the state when valid tests can be performed.
There are lot of possible ways to optimize I believe, but for now I suggest to leave this one implementation.

ELF/LinkerScript.cpp
384 ↗(On Diff #47908)

I changed that when modified readOutputSectionDescription() as well. So this change is to match the style of readOutputSectionDescription() which is called from readSections(). Not sure it is fine for this patch, probably separate patch for both is better.
That`s why it was changed in separate refactoring patch: http://reviews.llvm.org/D17256.
Point was that there is no need to check Error flag here as skip() does that at first line.

I returned that back as mentioned above refactor patch has it.

ruiu added inline comments.Feb 17 2016, 10:25 AM
ELF/LinkerScript.cpp
55 ↗(On Diff #48164)

return R && R->Keep;

395 ↗(On Diff #48164)

Did you test this code with an incomplete input? I think this will fall into an infinite loop if you give

SECTIONS { .foo : { KEEP(
396 ↗(On Diff #48164)

Why don't you add Keep as a parameter to the constructor?

ELF/LinkerScript.h
58 ↗(On Diff #48164)

Remove LinkerScript::.

grimar updated this revision to Diff 48289.Feb 18 2016, 1:55 AM
grimar marked 4 inline comments as done.
  • Addressed Rui's comments.
ELF/LinkerScript.cpp
395 ↗(On Diff #48164)

Hmm you're right, my mistake.

ruiu added inline comments.Feb 18 2016, 12:01 PM
ELF/LinkerScript.cpp
404 ↗(On Diff #48289)

s/Cmd/Tok/.

(When we read a token, we don't know if it is a KEEP command or just a regular token.)

ELF/LinkerScript.h
27 ↗(On Diff #48289)

Please do not over use friend. No one but LinkerScript should use SectionRule, so you want to make the member public.

grimar updated this revision to Diff 48490.Feb 19 2016, 7:01 AM
  • Addressed review comments.
  • Updated testcase: added case when section name matches two entries in the SECTIONS directive, the first one doesn't have KEEP, the second one does.
rafael added inline comments.Feb 19 2016, 12:16 PM
test/ELF/linkerscript-sections-keep.s
52 ↗(On Diff #48490)

Yes, that is the test I had in mind, thanks.

ruiu accepted this revision.Feb 22 2016, 2:19 PM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Feb 22 2016, 2:19 PM
This revision was automatically updated to reflect the committed changes.