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

> For instance why is the binary input format handled here but the elf input format is handled inside of Object, that seems kind of off to me. I remember you mentioned an idea a while back about having read and writer types that map in and out of a common representation. Maybe we should refactor the Object code to expose enough of an interface that code outside of Object can reconstruct the ELF Object the way this code does so for the binary input case.

I think this is exactly the issue with the big "if" statement, and also the naming of the functions. At the moment, we have a path which goes something like: HandleBinaryELFT -> HandleBinary -> HandleArgs (binary object), so we're converting from a binary input into an ELF object, and then emitting it as a binary object. The whole conversion to ELF seems wrong here. The advantage of having separate classes for readers and writers, with a common intermediate representation, is that the process looks a lot clearer.

Your code could look something like:

  InputReader Reader(InputFile, InputFormat);
  Object *Obj = Reader.createObject(); // Creates an intermediate object based on the input type.
                                       // There would be 5 different paths (one for each ELF format, and one for binary).
  Obj->handleArgs(Args);               // Removes/add sections/symbols etc.
  Obj->write(OutputFormat);            // Performs section and segment layout and writes out the file, according to the output format.

Note how the "handleArgs" call is hoisted up out of the depths of the nested call, and how the writing is handled separately. It would allow easy adding of new input and output formats, by simply extending the corresponding createObject/write implementation.

Does this sound plausible to you?



================
Comment at: test/tools/llvm-objcopy/binary-input-test.bin:1
+0000
----------------
I don't think you meant to add this file?


================
Comment at: test/tools/llvm-objcopy/binary-input.test:1
+# RUN: printf 0000 > %p/binary-input-test.bin
+# RUN: llvm-objcopy -I binary -B i386:x86-64 %p/binary-input-test.bin %t2
----------------
I think you probably want %T, not %p, since that's the test output directory, not the test source directory.


================
Comment at: test/tools/llvm-objcopy/binary-input.test:112
+#CHECK-NEXT:   Symbol {
+#CHECK-NEXT:     Name: _binary_binary_input_test_start
+#CHECK-NEXT:     Value: 0x0
----------------
I just did some experimenting, and these symbols should include the file extension (so in the current case, this one should be "_binary_binary_input_test_bin_start"), according to GNU objcopy. I think you want to be using filename() instead of stem().


Repository:
  rL LLVM

https://reviews.llvm.org/D41687