[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:
> 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.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:885
+struct ReaderConfig {
+  Optional<StringRef> ExtractPartition;
+};
----------------
jhenderson wrote:
> Do you anticipate there being multiple members in this struct in the future? If not, it seems a bit superfluous.
Okay, I changed this to a ctor arg.


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62364