Page MenuHomePhabricator

Remove kindInGroup reference
ClosedPublic

Authored by ruiu on Jan 26 2015, 4:06 PM.

Details

Summary

That kind of reference was used only in ELFFile, and the use of
that reference there didn't seem to make sense. All test still
pass (after adjusting symbol names) without that code. LLD is
still be able to link LLD and Clang. Looks like we just don't
need this.

Diff Detail

Event Timeline

ruiu updated this revision to Diff 18789.Jan 26 2015, 4:06 PM
ruiu retitled this revision from to Remove kindInGroup reference.
ruiu updated this object.
ruiu edited the test plan for this revision. (Show Details)
ruiu added a project: lld.
ruiu added a subscriber: Unknown Object (MLST).
kledzik edited edge metadata.Jan 26 2015, 4:26 PM

From what I recall, ELF has the semantics that you cannot just remove an atom in the middle of a section, because there is not enough relocations in the section to fix up the remaining (e.g CALLs to static functions in the same section don't have relocations on them, so if the distance changed because some atom between the two was removed, the CALL now be to the new location). The group reference was a way to keep all atoms in a section together.

ruiu added a comment.Jan 26 2015, 4:35 PM

All atoms in the same section already have kindAfter references, so that
all of them are added and removed as a group. So we don't need another
references to make them a group.

This is mainly relevant when you turn on garbage collection as Nick pointed out. You can refer to an atom in a section but to keep the section to be consistent w.r.t ELF you have to keep all the remaining atoms together.

ruiu added a comment.Jan 26 2015, 8:10 PM

I think it's not for garbage collection. Because all atoms in the same
section are already connected with LayoutAfter references, they are treated
as a group by the garbage collector.
2015/01/26 19:25 "Shankar Easwaran" <shankar.kalpathi.easwaran@gmail.com>:

This is mainly relevant when you turn on garbage collection as Nick
pointed out. You can refer to an atom in a section but to keep the section
to be consistent w.r.t ELF you have to keep all the remaining atoms
together.

http://reviews.llvm.org/D7189

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

Section groups(grouping multiple sections as part of a group) cannot be handled without ingroup references for garbage collection and correctness.

ruiu added a comment.Jan 27 2015, 10:36 AM

That's not correct. As you may know (because you are the person who implemented section group support to LLD), kindInGroup is not used to implement section group. Please take a look at the code, this piece of code doesn't seem to do anything meaningful including section groups.

shankarke edited edge metadata.Jan 27 2015, 11:37 AM

Section groups is only implemented in lld/Core(does not test garbage collection with section groups though). Support is not in the ELF Reader as yet too.

Sorry to club this in the current review thread, I think we are overloading the Layout references for garbage collection.

If you are creating a reference (kindLayoutAfter) from A to B, that may not mean that you cannot garbage collect B for the end user.

My thought on Layout references was that it only guarantees that atoms appear in Layout reference order.

Why are we overloading this for Garbage collection (aside from saving space/code) ?

We should create kindLive (or) some better name IMO for Garbage collection. With this the complex LayoutPass can be optional and would be meant only for users that need the LayoutReferences to specify layout of the image.

We could come up with a simpler pass for Layout(to sort atoms by file ordinal).

What do you think ?

ruiu added a comment.Jan 27 2015, 1:37 PM

Sorry, Shankar, I don't get what you mean by "Section groups is only implemented in lld/Core". Can you please take a look at the diff? kindInGroup was used only in ELFFile.h, and that use didn't make sense. I'm just trying to delete dead code by this change. This change didn't change functionality, LLD is still able to link itself/Clang, nothing more than that.

shankarke accepted this revision.Jan 27 2015, 1:49 PM
shankarke edited edge metadata.
This revision is now accepted and ready to land.Jan 27 2015, 1:49 PM
ruiu added a comment.Jan 27 2015, 2:39 PM

I'm going to submit this because Shankar approved on Phab.

Shankar, if you still want to bring up the point you mentioned in the last email, please start a new thread. Thanks!

atanasyan resigned from this revision.Feb 3 2016, 12:30 AM
atanasyan removed a reviewer: atanasyan.
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in r227259.