This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Allow applying SHF_MERGE optimization for relocatable output.
ClosedPublic

Authored by grimar on Nov 14 2017, 6:28 AM.

Diff Detail

Event Timeline

grimar created this revision.Nov 14 2017, 6:28 AM
ruiu added inline comments.Nov 15 2017, 12:01 AM
ELF/InputSection.cpp
414

Do you need this temporary variable?

ELF/LinkerScript.cpp
470–472

... already been merged by sh_entsize. If -r was given, we want to create an output section for each mergeable input section because we don't want to mix two sections with different sh_entsize.

ELF/Writer.cpp
494

I wonder if you really need to handle this as a special case.

grimar added inline comments.Nov 15 2017, 12:22 AM
ELF/InputSection.cpp
414

No, but then code will be 2 lines anyways because of clang-format, IMO looks better with it.

ELF/Writer.cpp
494

Do you mean we can create symbols for all synthetic sections instead ? I think that is at least confusing to see in output and excessive.

ruiu added inline comments.Nov 15 2017, 12:30 AM
ELF/Writer.cpp
494

Well, I mean I actually do not understand what this code is for in the first place.

grimar added inline comments.Nov 15 2017, 12:35 AM
ELF/Writer.cpp
494

It is for this place:
https://github.com/llvm-mirror/lld/blob/master/ELF/SyntheticSections.cpp#L1529

When we rewrite relocations we updating info field and want to set proper section index.
Before this patch we had regular SHF_MERGE sections in output, so symbols were created for them.
But now them are synthetic and there is no symbol, so we emit wrong zero index and relocation is broken.

BTW, I think that affects regular output + --emit-relocs as well. I am going to check soon and will split that change to different patch if so.

ruiu added inline comments.Nov 15 2017, 1:28 AM
ELF/InputSection.cpp
414

I don't think so. Can you inline?

ELF/Writer.cpp
494

This is probably okay, but let me think about it, as it still feels somewhat ad-hoc. There might be some simpler way.

grimar added inline comments.Nov 15 2017, 1:29 AM
ELF/Writer.cpp
494

Sure. I can confirm that --emit-reloc is affected, I am going to post a patch for that soon, so we can continue discussion of this place there.

grimar updated this revision to Diff 123158.Nov 16 2017, 4:44 AM
  • Rebased. Most of changes were gone away.
ruiu accepted this revision.Nov 17 2017, 12:56 AM

LGTM

This revision is now accepted and ready to land.Nov 17 2017, 12:56 AM
grimar edited the summary of this revision. (Show Details)Nov 17 2017, 3:27 AM
This revision was automatically updated to reflect the committed changes.