[PATCH] D53667: [llvm-objcopy] Handle -O <format> flag.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/binary-input-with-arch.test:2
+# RUN: echo -n abcd > %t.x-txt
+# Preserve input to verify it is not modified
+# RUN: cp %t.x-txt %t-copy.txt
----------------
Nit: full stop, here and elsewhere.


================
Comment at: test/tools/llvm-objcopy/ELF/binary-input-with-arch.test:8
+
+# CHECK:      Format: ELF64-x86-64
+# CHECK-NEXT: Arch: x86_64
----------------
I don't think you need to check most of the lines that you check in this test. Keep it to the minimum that is actually interesting. I think that's the Format, the Arch, the AddressSize, the Class, possibly the DataEncoding the Machine and the SectionHeaderEntrySize. It might even be more minimal than that.


================
Comment at: test/tools/llvm-objcopy/ELF/cross-arch-bad-format.test:1
+# RUN: yaml2obj %s > %t.o
+
----------------
I'm not sure what the "cross-arch" bit refers to in this test name. Perhaps it should be called "bad-output-format.test"?


================
Comment at: test/tools/llvm-objcopy/ELF/cross-arch-bad-format.test:13
+
+# BAD-OUTPUT-FORMAT: Invalid format: 'xyz'.
----------------
I think this message needs to specify that its the Output format that is bad (i.e. the argument to -O).


================
Comment at: test/tools/llvm-objcopy/ELF/cross-arch-headers.test:3
+
+# RUN: llvm-objcopy %t.o -Oelf32-i386 %t.elf32_i386.o
+# RUN: llvm-readobj -file-headers %t.elf32_i386.o | FileCheck %s --check-prefixes=CHECK,I386,32
----------------
Let's put a space between -O and its argument to make it a little more readable and consistent with other tests.


================
Comment at: test/tools/llvm-objcopy/ELF/cross-arch-headers.test:4
+# RUN: llvm-objcopy %t.o -Oelf32-i386 %t.elf32_i386.o
+# RUN: llvm-readobj -file-headers %t.elf32_i386.o | FileCheck %s --check-prefixes=CHECK,I386,32
+
----------------
Nit: -file-headers uses a single dash, but --check-prefixes uses two. Please be consistent (I'd prefer double, but that's just me).


================
Comment at: test/tools/llvm-objcopy/ELF/cross-arch-headers.test:58
+
+# CHECK:        ElfHeader {
+# CHECK-NEXT:     Ident {
----------------
Similar comments apply here to the above test. Remove the things that aren't relevant to this test.


================
Comment at: test/tools/llvm-objcopy/ELF/cross-arch-sections-symbols.test:14
+  Machine:         EM_386
+Sections:
+  - Name:            .text
----------------
Maybe worth having a non-zero size for the sections?


================
Comment at: test/tools/llvm-objcopy/ELF/cross-arch-sections-symbols.test:21
+    Flags:           [ SHF_ALLOC ]
+Symbols:
+  Global:
----------------
Maybe worth having a non-zero size for the symbols?


================
Comment at: test/tools/llvm-objcopy/ELF/cross-arch-sections-symbols.test:26
+      Section:  .text
+      Value:    0x1234
+    - Name:     bar
----------------
These values should probably be inside the sections, once they have non-zero size.


================
Comment at: tools/llvm-objcopy/CopyConfig.h:51
   MachineInfo BinaryArch;
+  // Only applicable when --output-format != Binary (e.g. elf64-x86-64).
+  Optional<MachineInfo> OutputArch;
----------------
Binary -> binary, I believe? The comment above needs updating to be consistent with this one too.


================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:550
   std::unique_ptr<Object> Obj = Reader.create();
-  const ElfType OutputElfType = getOutputElfType(In);
+  // Prefer OutputArch (-O<format>) if set, otherwise infer if from the input.
+  const ElfType OutputElfType =
----------------
infer if -> infer it


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53667