This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - add support for relocations against local symbols when producing relocatable output.
ClosedPublic

Authored by grimar on Mar 2 2016, 8:31 AM.

Details

Reviewers
ruiu
rafael
Summary

There was a known limitation for -r option:
relocations against local symbols were not supported.
For example rel[a].eh_frame sections contained relocations against sections
and that was not supported for -r before. Patch fixes that.

Diff Detail

Event Timeline

grimar updated this revision to Diff 49631.Mar 2 2016, 8:31 AM
grimar retitled this revision from to [ELF] - add support for relocations against local symbols when producing relocatable output..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
emaste added a subscriber: emaste.Mar 2 2016, 8:35 AM
ruiu added inline comments.Mar 2 2016, 9:03 AM
ELF/InputSection.cpp
139

The value should always be available, right? So, you want to do

uint32_t Idx = Out<ELFT>::SymTab->Locals[Sym];

instead of using find.

ELF/OutputSections.h
246

Please use DenseMap. Also, I'd use uint32_t instead of unsigined.

grimar updated this revision to Diff 49642.Mar 2 2016, 9:48 AM
  • Addressed review comments.
ELF/InputSection.cpp
139

I dont like []. Problem is that [] operator creates and returns default value if there is no map entry. That can hide error, while accessing end iterator works like some kind of assert.
But here I think it is save to assume that value is really always available, so ok.

ELF/OutputSections.h
246

Fixed, btw I just used the same type as assigned to entries:
unsigned NumLocals = 0;
So I am not sure it is better, at least that was consistent.

ruiu accepted this revision.Mar 2 2016, 10:01 AM
ruiu edited edge metadata.

LGTM

ELF/InputSection.cpp
139

If it's a concern, you can add an assert here to check that the key always exists, but I think it is probably a bit too overly cautious.

ELF/OutputSections.h
246

Why I wrote that comment is because we have this piece of code in copyRelocations() and want to be consistent.

uint32_t SymIndex = Rel.getSymbol(Config->Mips64EL);
ELF/Writer.cpp
581

Please guard this line with "if (Config->Relocatable)"

This revision is now accepted and ready to land.Mar 2 2016, 10:01 AM
grimar marked 2 inline comments as done.Mar 2 2016, 11:54 PM
grimar added inline comments.
ELF/Writer.cpp
581

Done. I thought about something like that, because it is not reasonable to fill and use it for non-relocatable output.
At first I thought Locals can be wrapped in a unique ptr, but that probably a bit more code, with no big profit.

I guarded it with 'if' and updated a comment for Locals about that:

// Local symbol -> ID, filled only when producing relocatable output.
grimar closed this revision.Mar 2 2016, 11:57 PM

r262590