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

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 7 inline comments as done.
abrachet added inline comments.


================
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;
----------------
jhenderson wrote:
> 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.
Thats much better :)


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:556-557
+    const Elf_Phdr &Phdr = Segments.getOriginal(I);
+    if (segmentWithinSegment(Child, Phdr))
+      return I;
+  }
----------------
jhenderson wrote:
> 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.
Something like `if (segmentWithinSegment(Child, Phdr) && findParentSegment(Phdr) != None)`?

> Same comment applies to sections.
I think I am misunderstanding something, do sections have anything to do whether a segment can have a parent? There is no overlapping sections I thought so I don't think this relates to that. Sorry about the confusion!


================
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 &&
----------------
jhenderson wrote:
> 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.
> 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.
Indeed, thanks fixed that.

> 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.
I have added this it happens on line 941 of the unit test, is that what you mean?

> Also, consider the following bug in your sectionWithinSegment code: https://bugs.llvm.org/show_bug.cgi?id=41126.
I //think// I have this:
```
|-Segment-|
    |-Section-|
```
tested and it properly says that the section is in the segment. There was no change needed for that.
To get yaml2obj to output that I just made the sections size larger than the segment and put the section in the segment. I had trouble with the other two examples in that bug report because `Offset:` was not recognized for sections?  So I don't know how to test these two cases:
```
    |-Segment-|
|-Section-|

  |-Segment-|
|---Section---|
```
 but I know for sure they will not be reported as being in the segment.


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

https://reviews.llvm.org/D66070