[PATCH] D41687: [llvm-objcopy] Add support for input types and the -I and -B flags

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Context missing.

In https://reviews.llvm.org/D41687#966842, @jakehehrlich wrote:

> In https://reviews.llvm.org/D41687#966597, @jhenderson wrote:
>
> > I just did some experimenting with GNU objcopy with this, and I'm getting a binary copy of the input, when using -I binary, even with -B i386:x86-64. I can workaround this using -O elf64-x86-64, i.e. "objcopy text.txt text.o -B i386:x86-64 -I binary -O elf64-x86-64". If I drop the -B, I get an EM_NONE machine type, and if I drop the -O, I get a text file. Not sure how you want to handle this!
>
>
> Well for command line compatibility we're going to have to accept "-O elf64-x86-64" which we don't do right now. In fact for the specific use case that I'm trying to solve here I'll need that to work (whoops). I'm fine copying the behavior of GNU objcopy here but I might add it in two more changes. One change will add extra output formats (like elf64-x86-64) and the other will make the default output type the corresponding input type. Those two changes together with this change should produce command line compatibility. Sound good?


Yes, sounds good.



================
Comment at: test/tools/llvm-objcopy/binary-input-archs.test:1
+# RUN: printf 0000 > %t
+
----------------
You don't need to change this test in this commit, but it will need updating once you make the -O format changes.


================
Comment at: test/tools/llvm-objcopy/binary-input.test:2
+# RUN: printf 0000 > %t
+# RUN: llvm-objcopy -I binary -B i386:x86-64 %t %t2
+# RUN: llvm-readobj -file-headers -sections -symbols %t2 | FileCheck %s
----------------
Could you modify this test slightly, please, to exercise the non-alnum replacement code. The easiest thing to do would be to add a file extension to the input file (e.g. make it %t.bin rather than %t).


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:343-345
+  // We need to construct new symbol names but Symbols don't own their names.
+  // We allocate the new symbol names using a string saver that stays around
+  // for the duration of the program.
----------------
jhenderson wrote:
> I wonder if this is an indication that Symbols should own their own names. It would mean a bit of copying in the ELF input case, but could prevent easy-to-make errors if we want to create or rename symbols.
> 
> If you prefer keeping it as is, I'd make a separate function called "MakeBinarySymbolName(StringRef BaseName, StringRef Suffix)", so that the warts of the name ownership can be kept separate from the adding of symbols, and it can be reused in other places too.
> 
> Did you consider making InputBinaryFormat a subclass of Object? That would allow you to have a slightly nicer name ownership resolution, apart from anything else.
The comment needs updating now that symbols do own their own names, and I think the StringSaver header probably can be deleted too.


Repository:
  rL LLVM

https://reviews.llvm.org/D41687