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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with a couple of minor comments.



================
Comment at: test/tools/llvm-objcopy/ELF/binary-input-with-arch.test:16
+
+# CHECK:   Class: 64-bit
+# CHECK:   DataEncoding: LittleEndian
----------------
Super-nit: remove the extra spaces between CHECK and the pattern here :)


================
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
----------------
rupprecht wrote:
> jhenderson wrote:
> > Let's put a space between -O and its argument to make it a little more readable and consistent with other tests.
> Done here, but I do want to make sure that it works this way to ease migration, so I've left in a single test case (that also checks for no spaces in -Ibinary and -Barch) w/o the spaces.
Okay, that makes a lot of sense.


================
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
+
----------------
rupprecht wrote:
> jhenderson wrote:
> > Nit: -file-headers uses a single dash, but --check-prefixes uses two. Please be consistent (I'd prefer double, but that's just me).
> Done + I'll fix the other test cases in this dir in a separate NFC commit
Sounds good.


================
Comment at: test/tools/llvm-objcopy/ELF/cross-arch-sections-symbols.test:26
+      Section:  .text
+      Value:    0x1234
+    - Name:     bar
----------------
rupprecht wrote:
> jhenderson wrote:
> > These values should probably be inside the sections, once they have non-zero size.
> Done; is this what you meant?
Almost. The symbol value of foo points to somewhere that isn't inside .text (i.e. it has value 0x1234, but the section size is only 32, so it's a long way past the end). It's not technically invalid ELF format, but it is a bit weird. If you don't want to update again, that's fine, but changing the value to, say, 16, would be my preference.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53667