Page MenuHomePhabricator

[ELF] Emit DT_TEXTREL dynamic table flag
ClosedPublic

Authored by atanasyan on May 11 2014, 2:30 AM.

Details

Summary

If one or more dynamic relocation might modify a read-only section, dynamic table should contain DT_TEXTREL tag.

The patch introduces new RelocationTable::canModifyReadonlySection() method. This method checks through the relocations to see if any modifies a read-only section. The DynamicTable class calls this method and emits the DT_TEXTREL tag if necessary.

Diff Detail

Event Timeline

atanasyan updated this revision to Diff 9289.May 11 2014, 2:30 AM
atanasyan retitled this revision from to [ELF] Emit DT_TEXTREL dynamic table flag.
atanasyan updated this object.
atanasyan edited the test plan for this revision. (Show Details)
atanasyan added reviewers: Bigcheese, ruiu, shankarke.
atanasyan added a subscriber: Unknown Object (MLST).

This patch LGTM but do you think it's better to have a pass ?

This revision is now accepted and ready to land.May 11 2014, 5:43 AM

Good point. I will send new version of the patch soon.

atanasyan updated this revision to Diff 9294.May 11 2014, 6:59 AM
atanasyan updated this object.
atanasyan removed a reviewer: shankarke.

In this version of thу patch the RelocationTable::canModifyReadonlySection() method iterates over relocations to see if any one modifies a read-only section each time when we need this information.

ruiu edited edge metadata.May 12 2014, 1:01 PM

LGTM

lib/ReaderWriter/ELF/SectionChunks.h
959

can you expand "auto"?

960

nit: fit in one line

atanasyan added inline comments.May 12 2014, 1:38 PM
lib/ReaderWriter/ELF/SectionChunks.h
959

The type name of rel is rather long - std::pair<const DefinedAtom *, const Reference *>. It is possible to make it shorter using typedefs but I cannot invent a good short name.

960

Yeah, the expression does not look good. But how can we fit it into one line?

ruiu accepted this revision.May 12 2014, 1:41 PM
ruiu edited edge metadata.
ruiu added inline comments.
lib/ReaderWriter/ELF/SectionChunks.h
959

I'd probably write like this

for (const auto &rel : _relocs) {

const DefinedAtom *atom = rel.first
...

But I feel it might be a bit verbose. It's up to you.

960

It looked me that it would fit in 80 chars. It actually isn't. Sorry.

joerg added a subscriber: joerg.May 12 2014, 3:46 PM

Can you also add the corresponding --warn-shared-textrel option?
It would also be nice to indicate which section at what offset by which relocation created the text relocation.
The lack of any hint towards what triggered the text relocation is one of the most annoying "features" of GNU ld.

Sure, I plan to add the --warn-shared-textrel option in a separate patch.

atanasyan closed this revision.May 13 2014, 12:49 AM

Thanks for review.

Closed by commit rL208670.