This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Fix for ld.lld does not accept "AT" syntax for declaring LMA region
ClosedPublic

Authored by grimar on Dec 19 2017, 8:22 AM.

Diff Detail

Event Timeline

grimar created this revision.Dec 19 2017, 8:22 AM
grimar updated this revision to Diff 127854.EditedDec 21 2017, 3:16 AM
  • Fixed initial logic (all tests pass now).
ruiu added inline comments.Dec 26 2017, 12:45 AM
ELF/LinkerScript.cpp
810

This function call is not desirable because we know that Sec is not used by the function. Please improve, split or whatever you can do to findMemoryRegion so that its semantics becomes more clear.

ELF/ScriptParser.cpp
711

This seems unnecessarily complicated. Why don't you just not allow a space between "AT" and ">"?

714

Use consume

718

This order isn't easy to understand because it is not obvious what drop_front drops (which is ">", but you need to carefully read the code to find it out.) Please write this way

if (consume(">"))
else if (peek().startswith(">"))
else
grimar added inline comments.Dec 26 2017, 12:53 AM
ELF/ScriptParser.cpp
711

I checked that gnu linkers support all 4 forms:

AT>REGION
AT> REGION
AT >REGION
AT > REGION

PR mentions 2 ways: "AT> syntax" and "AT > syntax".
Spec mentions third: "AT>lma_region" (https://www.eecs.umich.edu/courses/eecs373/readings/Linker.pdf, p.48).

I guess we just want to support all of them then.

ruiu added inline comments.Dec 26 2017, 1:59 AM
ELF/ScriptParser.cpp
711

It's a little bit pathetic. If you want to do that, please try changing the lexer so that ">" is always parsed as a single-character token instead of a regular token character

grimar added inline comments.Dec 26 2017, 2:05 AM
ELF/ScriptParser.cpp
711

Changing lexer feels a bit overkill for this feature for me, though looks it probably the most correct way.
I'll try it, thanks.

ruiu added a comment.Dec 26 2017, 2:09 AM

I don't think it's overkill. Please take a look at https://reviews.llvm.org/D41577

grimar updated this revision to Diff 128176.Dec 26 2017, 7:32 AM
grimar marked 3 inline comments as done.
  • Rebased, addressed review comments.
ruiu added inline comments.Dec 26 2017, 6:32 PM
ELF/LinkerScript.h
223–225

Well, I think you already knew what I would be saying, but these names are not good. I think you just chose "pick" because "find" is already in use. That's not a good way of naming a function.

ELF/ScriptParser.cpp
711

This part of code looks much nicer now.

grimar added inline comments.Dec 28 2017, 10:44 PM
ELF/LinkerScript.h
223–225

I had to search different wording because find is already in use, that is obvious.
At first I tried findNamedMemoryRegion and findMemoryRegion, but did not like it. I chose pick because in compare with
findMemoryRegion which simply searches region by name, pickMemoryRegion uses more complex rules,
including heuristic to select the appropriate memory region for section.

After your comment I experimented with this code again, but found that I like what these methods do, so
I think problem is with naming and not with their logic/how they were splitted. Do you agree here ?

If so, what do you think about following naming ?

MemoryRegion *findMemoryRegion(llvm::StringRef Name);
MemoryRegion *matchMemoryRegion(OutputSection *Sec);

or

MemoryRegion *findMemoryRegion(llvm::StringRef Name);
MemoryRegion *selectMemoryRegion(OutputSection *Sec);
ruiu added inline comments.Jan 8 2018, 2:21 PM
ELF/LinkerScript.cpp
676

Looks like you don't even need this LMARegion member in the first place, as you can just check if LMARegionName is empty here.

grimar updated this revision to Diff 129061.Jan 9 2018, 4:14 AM
  • Addressed review comment.
ELF/LinkerScript.cpp
676

Removed LMARegion member.

ruiu added inline comments.Jan 9 2018, 3:22 PM
ELF/LinkerScript.cpp
676

Inline findMemoryRegion and delete that function.

807–810

Rename this findMemoryRegion.

grimar updated this revision to Diff 129230.Jan 10 2018, 3:23 AM
  • Addressed review comments.
grimar updated this revision to Diff 129429.Jan 11 2018, 4:17 AM
  • Addressed review comments (improved testcase).
ruiu accepted this revision.Jan 11 2018, 12:51 PM

LGTM

This revision is now accepted and ready to land.Jan 11 2018, 12:51 PM
This revision was automatically updated to reflect the committed changes.