This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Store PhdrEntry values by pointers instead of storing by value.
ClosedPublic

Authored by grimar on Jul 25 2017, 5:32 AM.

Details

Summary

Patch changes vector to store PhdrEntry by pointers,
at least 2 patches already tried to rely on this change: D35680 and D34956.

I think that happens because it is generally more convinent way,
so even if we will never land these patches it is still usefull to do IMO.

But if we land it I'll be able to rebase those ones to exclude unrelative changes.

Diff Detail

Event Timeline

grimar created this revision.Jul 25 2017, 5:32 AM
ruiu edited edge metadata.Jul 25 2017, 11:32 AM

This doesn't really seem to improve the code, although it probably won't decrease the quality as well. I looked at your two other patches, but I cannot find why they need this change.

ELF/LinkerScript.cpp
801–806

FirstPTLoad is not a type that you can handle easily as it needs to dereference twice. You can do this.

auto It = llvm::find_if(...);
if (It == Phdrs.end())
  return;
PhdrEntry *FirstPTLoad = *It;
grimar updated this revision to Diff 108243.Jul 26 2017, 3:14 AM
  • Addressed review comment.
In D35832#820468, @ruiu wrote:

I looked at your two other patches, but I cannot find why they need this change.

For example in D35680 I am storing pointer to PT_LOAD segment instead of pointer to output section.
When we have vector of PhdrEntry (by value) it is hard to work with pointers on elements, because resize/remove operations
applied to vector may/will break the pointer value. If we have vector of pointers instead, it solves this problem easily.

ELF/LinkerScript.cpp
801–806

Yes, this looks better.

ruiu accepted this revision.Jul 26 2017, 1:54 PM

LGTM

This revision is now accepted and ready to land.Jul 26 2017, 1:54 PM
This revision was automatically updated to reflect the committed changes.