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

[Alex Brachet via Phabricator via llvm-commits]

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


================
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:
> abrachet wrote:
> > 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.
> > Indeed, thanks fixed that.
> 
> I think you've missed an issue - what happens if two segments have the same offset, size and alignment? In that case, neither will be treated as a parent of the other, and the algorithm falls apart.
> 
> > 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:
> 
> 
> I //think// there's a ShOffset field available to set the claimed offset of a section (the actual data will still be written where it would be). The alternative to that is to effectively force the section's offset by setting the AddressAlign and Address fields, so that yaml2obj has to place it at a certain position. You'll see quite a few examples of this within tests. Once you have forced a section to a specific position, you can then just set the ProgramHeader fields accordingly.
> I think you've missed an issue - what happens if two segments have the same offset, size and alignment? In that case, neither will be treated as a parent of the other, and the algorithm falls apart.
Ok, now it is finally fixed. It isn't very pretty because I need to pass the indexes of each segment but I just choose whichever segment is first to be the parent.


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

https://reviews.llvm.org/D66070