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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've got a few minor comments, but structurally this looks reasonable to me. I could potentially see other benefits to this approach in the future too.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/partitions.test:1-3
+RUN: llvm-objcopy --extract-main-partition %p/Inputs/partitions.elf %t1
+RUN: llvm-objcopy --extract-partition=part1 %p/Inputs/partitions.elf %t2
+RUN: llvm-objcopy --extract-partition=part2 %p/Inputs/partitions.elf %t3
----------------
It might be helpful briefly explaining what "part1" and "part2" are for those not massively familiar with the intended switch behaviour. This is probably best done by explaining what partitions.elf contains.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/partitions.test:9-12
+RUN: not llvm-objcopy --extract-partition=part3 %p/Inputs/partitions.elf %t4 2>&1 | FileCheck --check-prefix=ERROR1 %s
+RUN: not llvm-objcopy --extract-main-partition --extract-partition=part3 %p/Inputs/partitions.elf %t4 2>&1 | FileCheck --check-prefix=ERROR2 %s
+ERROR1: error: could not find partition named 'part3'
+ERROR2: error: cannot specify --extract-partition together with --extract-main-partition
----------------
I'd prefer this part of the test to be after the `MAIN:` bits further down. I'd also recommend putting the first error message after the first check and the second after the second check. Keeping the first three RUNs at the top together makes sense though, so I'd do:
```
RUN: extract main
RUN: extract 1
RUN: extract 2

CHECKs for the first three

RUN: error 1
ERROR1:
RUN: error 2
ERROR2:
```


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/partitions.test:14
+
+MAIN: ELF Header:
+MAIN-NEXT:   Magic:   7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00
----------------
We usually add extra spacing to make all the content line up like it does in the real output, i.e:

```
MAIN:      ELF Header:
MAIN-NEXT:   Magic:
```


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:516
+        return true;
+      return (Sec.Flags & SHF_ALLOC) != 0 && !Sec.ParentSegment;
+    };
----------------
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?


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


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62364