[PATCH] D66070: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 6]

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D66070#1640579 <https://reviews.llvm.org/D66070#1640579>, @abrachet wrote:

> > That should allow you to handle multiple levels of nesting/overlapping etc without too much difficulty.
>
> Hmm if a segment had a list of its //child// segments, then couldn't layout be done recursively on each child segment? Would this not be more straight forward?


What happens if a segment is inside another segment? You only want to lay it out if it hasn't been laid out already, so you'd have to find a way to track that. On the other hand, if you only lay out things that have no parent (and then lay out the things with parents based on their parent's layout), you don't have this issue. Keeping track of children is not necessarily bad, as it allows you to lay the children out at the same time as the parent, but it's not as important (you can always do two passes for example).

> I think what I currently have should move more in the direction you are suggesting. I have a concern with my current implementation, its if I should use the original program headers or the updated ones. It's possible to change a segments vaddr, although you have mentioned unlikely. This would be problematic to use `sectionInSegment` after changing the program headers because then it would be obviously wrong. So right now I use the original program headers.

Using the original program headers is probably the right thing to do, as that ensures the original relative layout is preserved.

> Also something to look out for, I use parent segments as indexes not pointers and those are `ssize_t` not `size_t` because the alternative of using one past the last segment like an end iterator was too ugly than using -1 to denote no parent. I could also use size_t's max I suppose.

See my inline suggestion about using `Optional`. `size_t`'s max would be my next suggestion.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:257-258
 
+  /// Returns -1 if the section has no parent segment, otherwise returns the
+  /// index of the parent.
+  ssize_t findSectionsParentSegment(SectionRef Sec) const;
----------------
How about using Optional<size_t> instead of -1? I think that would more clearly convey the meaning and would avoid the weirdness of using `ssize_t` everywhere.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:262
+public:
+  class SegmentRef;
+
----------------
Could you not just move `SegmentRef`'s definition earlier?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:265
+private:
+  ssize_t findSegmentsParentSegment(SegmentRef Segment) const;
+
----------------
I'd keep this and `findSectionsParentSegment()` next to each other, and I'd seriously consider just calling them both `findParentSegment()`.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:556-557
+    const Elf_Phdr &Phdr = Segments.getOriginal(I);
+    if (segmentWithinSegment(Child, Phdr))
+      return I;
+  }
----------------
Something you may wish to consider is effectively flattening the nesting, so that only top-level segments are parents, i.e. if a segment has a parent, then the parent cannot have a parent itself. This may make the layout algorithm simpler, but could probably be left until you're at that point.

Same comment applies to sections.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:563
+template <typename ELFT>
+bool MutableELFObject<ELFT>::sectionWithinSegment(
+    const typename ELFT::Shdr &Sec, const typename ELFT::Phdr &Seg) {
----------------
Is this identical to the llvm-objcopy version? If not, could you highlight how it's different, please.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:587
+    const typename ELFT::Phdr &Child, const typename ELFT::Phdr &Parent) {
+
+  return Parent.p_offset <= Child.p_offset &&
----------------
Delete the blank line here.

You need to be careful. It looks to me like a segment could end up as its own parent here, if I'm not misreading the code.

Also, you should consider taking alignment requirements into account, in the event that two segments have the same size and offset. See https://bugs.llvm.org/show_bug.cgi?id=42963.

Also, consider the following bug in your sectionWithinSegment code: https://bugs.llvm.org/show_bug.cgi?id=41126.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66070/new/

https://reviews.llvm.org/D66070