[PATCH] D62364: llvm-objcopy: Implement --extract-partition and --extract-main-partition.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:516
+        return true;
+      return (Sec.Flags & SHF_ALLOC) != 0 && !Sec.ParentSegment;
+    };
----------------
jhenderson wrote:
> pcc wrote:
> > jhenderson wrote:
> > > It's quite tricky looking at the test to see if this part of the code is actually tested. Could you confirm that it is, i.e. that for both switches there is a non-alloc section not in a segment that is stripped and that there is a non-alloc section in a segment that is not stripped, please?
> > Non-alloc sections not in segments are not stripped. In the test, `.comment` is such a section and the test shows that it is kept in all output files.
> > 
> > The linker will not normally place non-alloc sections in segments (I think this is only possible with the `SECTIONS` directive, and lld forbids `SECTIONS` together with multiple partitions), so it would be tricky to construct such a test case, and I think we can consider a file with such a section to be invalid (as a multi-partition file, not in general) anyway. This condition returns false (i.e. keep) once it's determined that the section is non-alloc, so we wouldn't be increasing code coverage by having a non-alloc section in a segment.
> > 
> > The important case that is being tested is that alloc sections not in segments (i.e. not in segments for the current partition) are being stripped. The test shows that e.g. the main partition's `.text0` is being stripped from the loadable partitions, likewise `.bss1` in `part1` is being stripped from the main partition and `part2`, etc.
> > 
> > I wonder whether it would be better (in a separate change) to flip the predicate here and have it return true if we want to keep the section. That seems like it would make at least this clause a little less confusing, but I'm not sure about the others.
> Thanks.
> 
> I don't think we should flip the predicate - the function is effectively "shouldRemove()" and so true is the correct return for sections that should be removed (and false otherwise).
> 
> I think I misread `(Sec.Flags & SHF_ALLOC) != 0` as being "true if section is not-alloc", when it is the inverse. I'm not sure flipping the predicate would make this any less likely to happen.
Ack


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:900
   void setParentSegment(Segment &Child);
-  void readProgramHeaders();
+  void readProgramHeaders(const ELFFile<ELFT> &HdrElfFile);
   void initGroupSection(GroupSection *GroupSec);
----------------
jhenderson wrote:
> pcc wrote:
> > jhenderson wrote:
> > > I think this needs a comment explaining what `HdrElfFile` is. Either that or the name needs changing, because it's not immediately clear to me what it's supposed to be.
> > Is the comment on lines 1434-1436 not enough? I think that if I were trying to understand why we're passing `HdrElfFile` here, I'd look at what the caller of `readProgramHeaders` was doing, which would lead me to that comment. Maybe there's a better place for it though?
> I tend to go with a different approach (I guess everybody is different): if I'm in readProgramHeaders, and I want to know what my input argument is, I go to the declaration, because there's usually documentation comments there, if not at the definition, so maybe the comment needs moving here.
> 
> Alternatively, maybe it would be slightly less confusing renaming it slightly to `HeadersFile`?
Renaming to `HeadersFile` works for me; done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62364