This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Refactor target errors
ClosedPublic

Authored by evgeny777 on Nov 24 2016, 5:21 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

evgeny777 updated this revision to Diff 79221.Nov 24 2016, 5:21 AM
evgeny777 retitled this revision from to [ELF] Refactor target errors.
evgeny777 updated this object.
evgeny777 added reviewers: ruiu, rafael.
evgeny777 set the repository for this revision to rL LLVM.
evgeny777 added a project: lld.
evgeny777 added subscribers: grimar, ikudrin, llvm-commits.
ruiu added inline comments.Nov 24 2016, 9:24 AM
ELF/Writer.cpp
1559 ↗(On Diff #79221)

I think we should make OutputSections global, instead of doing a plumbing work to propagate information from Target to Writer.

evgeny777 added inline comments.Nov 24 2016, 9:30 AM
ELF/Writer.cpp
1559 ↗(On Diff #79221)

How about Buffer pointer? It isn't stored in OutputSection<ELFT>.
What do you think about saving buffer position in OutputSection<ELFT> and iterating over Symtab<ELFT>::Sections, instead of OutputSections ?

ruiu added inline comments.Nov 24 2016, 9:48 AM
ELF/Writer.cpp
1559 ↗(On Diff #79221)

Interesting idea. Not sure if that's actually good at the moment, but it's worth a try.

evgeny777 updated this revision to Diff 79284.Nov 25 2016, 1:54 AM
evgeny777 removed rL LLVM as the repository for this revision.

Addressed review comments

ruiu added inline comments.Nov 25 2016, 8:38 AM
ELF/Target.cpp
61 ↗(On Diff #79284)

I don't think we need a new error-ish function here. I'd define a function that returns the output location info as a string, then I could write like this.

error(getErrorLoc(Loc) + ": relocation " + toString(Type) + " out of range");
813 ↗(On Diff #79284)

Then this can be

fatal(getErrorLoc(Loc) + ": unrecognized reloc " + Twine(Type));
ELF/Writer.cpp
1560–1561 ↗(On Diff #79284)

Just return the return value of getLocation().

evgeny777 updated this revision to Diff 79381.Nov 28 2016, 2:24 AM

Addressed review comments

ruiu accepted this revision.Nov 28 2016, 6:40 AM
ruiu edited edge metadata.

LGTM. Nice!

ELF/Writer.cpp
1571 ↗(On Diff #79381)

You can return IS->getLocation(Loc - ISLoc) + ": " here.

1575 ↗(On Diff #79381)

and return "" here.

ELF/Writer.h
14 ↗(On Diff #79381)

Do you need this?

This revision is now accepted and ready to land.Nov 28 2016, 6:40 AM
This revision was automatically updated to reflect the committed changes.