[PATCH] D33964: [LLVM][llvm-objcopy] Added basic plumbing to get things started

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich marked 3 inline comments as done.
jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:193
+      // image is a copy of the file image, we preserve the file image as well.
+      Offset = FirstInSeg->Offset + Section->OriginalOffset -
+               FirstInSeg->OriginalOffset;
----------------
jhenderson wrote:
> jakehehrlich wrote:
> > jhenderson wrote:
> > > I've just thought about yet another problem with this:
> > > 
> > > In some cases, the ELF header and/or Program Header table can appear in a segment before the first section. Consequently, we need to preserve the relative offset to the start of the section, rather than the first segment. At the moment, any space before the first section within the segment is ignored and lost.
> > > 
> > > This applies to Segment::finalize() too.
> > Yea PT_PHDR is a problem I have run into in extensions of this diff. I think it needs some special handling. I think I can support it when I officially add support for copying data of interstitial gaps and nested segments. It will still require special care however. Would you be ok with me not attempting to handle this right now? At the very least this requires handling nested segments because the PT_PHDR segment must be nested in a PT_LOAD segment.
> > 
> > The needed information is preserved now because the original offset of the segment is stored until the segment offset needs to be computed and the original offset of the section is now stored and kept. So that gap can be recovered when performing layout of the segment. This seems to be enough to confirm that there isn't a fundamental design issue that prevents the PT_PHDR case from being handled.
> I'm not sure it's specific to PT_PHDR - it's possible to put these headers in any arbitrary segment, if you want (though I'm not sure why you would in most cases), e.g. directly in the first PT_LOAD segment, with the appropriate linker script. Also, I don't think it's specific to the headers either - the file image is going to get messed up if there was empty space of any kind not covered by a section before the first section. Rather than preserve the relative offsets of sections to the first section, this code should probably preserve the offset relative to the original segment start.
> 
> Example:
> ```
> | Segment                   |
> | gap | Section1 | Section2 |
> A     B
> ```
> The offset of Section1 will be placed at A, if I've followed the code correctly, not at B, where it should be, thus messing up the file and therefore the memory image. Section2 will still be placed immediately after Section1 correctly.
> 
> As a side point, this will also have an impact on the alignment of the first section, but assuming the input program is valid, I don't think that this will be an issue.
The difference between this case and the PHDR case is that it's easy to allocate space for t his gap during layout but it is not easy to retroactively figure out that this segment should be covering the program headers which is space that is not even up for allocation from the perspective of the current algorithm. I'm going to upload a new diff that handles this case but does not handle the PHDR case.


================
Comment at: tools/llvm-objcopy/Object.cpp:79-80
+
+// Returns true IFF a section covers a segment
+static bool sectionCoversSegment(const SectionBase &Section,
+                                 const Segment &Segment) {
----------------
jhenderson wrote:
> It's not clear on the surface what is meant by "Covers" (I assume it means within or similar). Also, I think you've got it backwards in the name - it returns true, if the section is in the segment, not the other way around.
> 
> Also, I'm not sure what the behaviour of empty sections should be here. My instinct is to say that empty sections are "covered" by a segment, if they appear at the start, or in the middle of a segment, but not if it is only at the end. I could imagine a case with two segments with the first having the same end offset as the start of the next one. If there is an empty section at the meeting point, I don't think it makes sense for it to be in both. From a run-time point of view, I don't think it matters, but it could cause weird looking anomalies in static dumping tools, and also leads to the case of, "if the first segment moves to a lower offset, does the section move with it or stay with the other segment?"
I agree, "covers" is not the right word here. I'll say "within" or "inside of" I suppose. I thought I was using "cover" in the colloquial sense it is used when referring to these things but apparently I did not use it correctly. I thought it was an odd way to refer to things. I'll just refrain from using that term.

Oh weather an empty section should belong to a segment I agree and came to the same conclusion by the same reasoning. I plan on never modifying segment size or contents however so I don't think this will cause an issue. 


Repository:
  rL LLVM

https://reviews.llvm.org/D33964