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

Fri Aug 23 04:38:46 PDT 2019

jhenderson added a comment.

Could you write some comments in the test code that show what the cases are that you are testing, similar to how you've written it in your inline comments. This will make it easier to understand what is going on.

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:556-557
+    const Elf_Phdr &Phdr = Segments.getOriginal(I);
+    if (segmentWithinSegment(Child, Phdr))
+      return I;
+  }
----------------
abrachet wrote:
> 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!
I'm thinking that if the segment layout looks something like the following:

```
| Section1 | Section2 | Section3 | Section4 |
| Segment1            | Segment2 |
| Segment3                                  |
```

Everything should have Segment3 as their parent (except Segment3 itself of course).

If you don't flatten the tree, then sections 1, 2, and 3 will have parents of Segment 1 or 2 which will make it harder to perform layout, as you effectively have to chase the parents pointer to find the top-level element to determine where to move them.

> 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!

Yes, I think you misunderstood. I was just referring to the fact that the parent of a section should be the top-most parent in the tree.

================
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 &&
----------------
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.

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:563
+
+// This is identical to the same function in llvm-objcopy except uses ELFT
+// types not the llvm-objcopy ones.
----------------
except it uses

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:564
+// This is identical to the same function in llvm-objcopy except uses ELFT
+// types not the llvm-objcopy ones.
+template <typename ELFT>
----------------
and not

================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:589
+bool MutableELFObject<ELFT>::segmentWithinSegment(
+    const typename ELFT::Phdr &Child, const typename ELFT::Phdr &Parent) {
+  if (Child.p_offset == Parent.p_offset && Child.p_filesz == Parent.p_filesz)
----------------
I'd rename Parent to PotentialParent and Child to Segment, because the Parent isn't necessarily a Parent yet (and therefore the Child isn't necessarily a Child of that segment).

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

https://reviews.llvm.org/D66070