This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Linkerscript: support complex section pattern grammar.
ClosedPublic

Authored by grimar on Sep 20 2016, 5:46 AM.

Details

Summary

Previously we were failed to parce complex expressions like:

foo : { *(SORT_BY_NAME(bar) zed) }

This is PR30442

Patch allows to handle them, for example the next grammar is parsed fine now:

.abc : { *(SORT(.foo.* EXCLUDE_FILE (*file1.o) .bar.*) .bar.*) }

Main idea of patch that globs and excludes can be wrapped in a SORT.
There is a difference in semanics of ld/gold:
ld likes:

*(SORT(EXCLUDE_FILE (*file1.o) .foo.1))

gold likes:

*(EXCLUDE_FILE (*file1.o) SORT(.foo.1))

Patch implements ld grammar.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 71917.Sep 20 2016, 5:46 AM
grimar retitled this revision from to [ELF] - Linkerscript: support complex section pattern grammar..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, evgeny777 and 2 others.
ruiu added inline comments.Sep 20 2016, 9:20 PM
ELF/LinkerScript.cpp
159–160 ↗(On Diff #71917)

Can you pass two pointers to InputSectionData instead of iterators?

S and E? Can you rename them Begin and End if they mean begin and end?

197–198 ↗(On Diff #71917)
InputSectionData *Begin = I->Sections.data() + SizeBefore;
InputSectionData *End = I->Sections.data() + I->Sections.size();
764–765 ↗(On Diff #71917)

Instead of mutating an argument, create a new vector and return it. It is the common practice in this file.

1098–1099 ↗(On Diff #71917)

Returns a new vector instead of mutating the argument.

ELF/LinkerScript.h
104–105 ↗(On Diff #71917)

So1 -> Sort1

RE is a common acronym for Regular Expression, but SO is not.

119–120 ↗(On Diff #71917)

Remove = ... Default. You seem to always assign them.

grimar updated this revision to Diff 72023.Sep 21 2016, 4:38 AM
  • Addressed review comments.
grimar updated this object.Sep 21 2016, 4:39 AM
grimar added inline comments.
ELF/LinkerScript.cpp
159–160 ↗(On Diff #71917)

It was Start/End. And it is actually pointers to pointers here, I am not sure it looks better than iterators, but done.

197–198 ↗(On Diff #71917)

Done.

764–765 ↗(On Diff #71917)

Done.

1098–1099 ↗(On Diff #71917)

Done.

ELF/LinkerScript.h
104–105 ↗(On Diff #71917)

Done.

119–120 ↗(On Diff #71917)

Done.

ruiu added inline comments.Sep 21 2016, 8:06 AM
ELF/LinkerScript.cpp
774–775 ↗(On Diff #72023)

Out and In look like they are short for Output and Input, so these names are confusing. Since we know the type here, Inner and Outer should suffice.

1133–1137 ↗(On Diff #72023)

This code seem too complicated compared to what it is doing. I think you could do like this.

SortSectionPolicy Inner = readSortKind();
SortSectionPolicy Outer = Default;
if (Inner != SortSectionPolicy::Default) {
  expect("(");
  Outer = readSortKind();
  if (Outer != SortSectionPolicy::Default) {
    expect("(");
    V = readInputSectionList();
    expect(")");
  } else {
    V = readInputSectionList();
  }
  expect(")");
}

for (SectionPattern &Pat : V) {
  Pat.SortInner = Inner;
  Pat.SortOuter = Outer;
}
grimar updated this revision to Diff 72052.Sep 21 2016, 8:35 AM
  • Addressed review comments.
ELF/LinkerScript.cpp
774–775 ↗(On Diff #72023)

Done.

1133–1137 ↗(On Diff #72023)

:) Something like this was my first idea when I strarted implementation. But then I thought you will not like the additional loop at the end..

ruiu accepted this revision.Sep 21 2016, 8:55 AM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 21 2016, 8:55 AM
This revision was automatically updated to reflect the committed changes.