This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Don't emit broken relocations for SHF_MERGE sections when --emit-relocs is used.
ClosedPublic

Authored by grimar on Nov 15 2017, 3:05 AM.

Details

Summary

Previously our relocations we rewrote were broken for that case.
We emited incorrect addend and broken relocation info field
because did not produce section symbol for mergeable synthetic sections.

Also helps to simplify D40026.

Diff Detail

Event Timeline

grimar created this revision.Nov 15 2017, 3:05 AM
grimar edited subscribers, added: evgeny777; removed: eastig.Nov 15 2017, 3:05 AM
grimar edited the summary of this revision. (Show Details)Nov 15 2017, 3:16 AM
ruiu accepted this revision.Nov 16 2017, 12:33 AM

LGTM

ELF/Writer.cpp
495

Synthetic sections don't have any relocations except mergeable sections. Mergeable output sections may have relocations because mergeable input sections may have relocations.

test/ELF/emit-relocs-mergeable-i386.s
6

Check if

test/ELF/emit-relocs-mergeable.s
6

Check if

This revision is now accepted and ready to land.Nov 16 2017, 12:33 AM
grimar added inline comments.Nov 16 2017, 3:10 AM
ELF/Writer.cpp
495

Sorry, I think I do not understand your suggested comment meaning.

You're saying that "Synthetic sections don't have any relocations except mergeable sections".

My testcase have '.foo' section that have '.foo.rela' section. Latter has relocations that refer to section symbol for '.strings',
used for relocation calulation. '.strings' in my case is a synthetic section. And it does not have '.strings.rela' or something,
so it never have relocations in my understanding.

So synthetic mergeable section '.strings' does not have relocations. But your suggested comment saying different.
I am going to commit this comment as is with fix for "-r" -> "-emit-relocs". Lets discuss the better wording if you think it
is not good now and I'll be happy to change it in followup.

ruiu added a comment.Nov 16 2017, 3:20 AM

So, synthetic sections have no relocations, right? But mergeable synthetic section is an exception. I don't see any problem with my comment.

grimar marked 4 inline comments as done.Nov 16 2017, 3:29 AM
In D40070#927204, @ruiu wrote:

So, synthetic sections have no relocations, right?

Right.

But mergeable synthetic section is an exception.

No. Mergeable synthetic sections also have no relocations.

We have synthetic section .debug_str, for example. And .debug_info that uses it.
.debug_info has relocations (has '.rela.debug_info' section). But .debug_str not.
.debug_str is used for relocation calculation for different section, which is .debug_info,
but it does not have relocations by itself.

ruiu added a comment.Nov 16 2017, 3:32 AM

Then it contradicts with the other comment in the same function.

// Create one STT_SECTION symbol for each output section we might
// have a relocation with.
In D40070#927225, @ruiu wrote:

Then it contradicts with the other comment in the same function.

// Create one STT_SECTION symbol for each output section we might
// have a relocation with.

Sorry, but I see no issue in this comment, probably my english is not good enough to see mistake in wording here.
What I think it is saying is that we create section symbols so that relocations that uses section symbols can use it, at least that
is what actually we do I believe.

ruiu added a comment.Nov 16 2017, 3:57 AM

Aah, I'd think the comment is actually ambiguous. I was thinking of a section that a relocation is applied to, but it can also be read as it is talking about a section that a relocation's symbol belongs to, and the latter interpretation is correct. Let's update that comment too. I think something like "Create a section symbol for each output section so that we can represent relocations that point to the section. If we know that no relocation is referring to a section (that happens if the section is a synthetic one), we don't create a section symbol for that section" should work.

In D40070#927255, @ruiu wrote:

Aah, I'd think the comment is actually ambiguous. I was thinking of a section that a relocation is applied to, but it can also be read as it is talking about a section that a relocation's symbol belongs to, and the latter interpretation is correct. Let's update that comment too. I think something like "Create a section symbol for each output section so that we can represent relocations that point to the section. If we know that no relocation is referring to a section (that happens if the section is a synthetic one), we don't create a section symbol for that section" should work.

Sounds good for me, thanks !

So second comment will remain the same right ? (or do you prefer something different)

// We do not create symbol for synthetic sections because do not have
// relocations that might use it, but SHF_MERGE sections is an exception
// used for --emit-relocs. After applying merging optimisation we want to
// rewrite relocations and need section symbol for synthetic mergeable
// sections to refer to.
ruiu added a comment.Nov 16 2017, 4:13 AM

Yes, that comment is okay, but since we are updating the first comment, and the comment has already mentioned synthetic sections, we can simplify it. I'd write "Unlike other synthetic sections, mergeable output sections contain data copied from input sections, and there may be a relocation pointing to its contents if -r or -emit-reloc are given."

Given that, LGTM. Thanks!

This revision was automatically updated to reflect the committed changes.