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

I'm not sure I'm a big fan of the big stack of functions that gradually pick off an argument, and then choose a specific function to call next. It's effectively a giant nested if statement, whereas what we had before was much more linear. I haven't got a good alternative solution as of yet, but will have a think and try to come up with an alternative.

Otherwise, most of this looks fine. There should be tests for each of the different possible -B values though.



================
Comment at: tools/llvm-objcopy/Object.cpp:585
+
+template <class ELFT> Object<ELFT>::Object(uint16_t EMachine) {
+  auto StrTab = llvm::make_unique<StringTableSection>();
----------------
The contents of this function feel a bit out-of-order. It starts with setting stuff to do with sections, then adds a symbol, then sets some properties, based on the sections, then goes off and initialises the elf header, before coming back and updating some more properties to do with the sections. Could you maybe reorder things as follows:
1) Initialise the ELF header, probably in a separate function called by this function.
2) Create the string table section, and add it to the Sections array.
3) Create the symbol table, set the string table property, add the symbol, and add it to the array.

This should make the function a little easier to follow.


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


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:410
+
+void HandleELFT(StringRef Input, StringRef OutFmt) {
+  Expected<OwningBinary<Binary>> BinaryOrErr = createBinary(Input);
----------------
This function should be static (along with the rest of the new functions).




================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:427
+
+void HandleInputType(StringRef Input, StringRef OutFmt, StringRef InFmt,
+                     StringRef Arch) {
----------------
I'm not sure this function is well named, given that this function is effectively the driver of the rest of objcopy - it's not really handling the input. It's doing all the work. I'd even go so far as to say that it should be inlined into main.

If you do want to keep the function, I'd rename Arch to InArch, and change the argument order to Input, InFmt, InArch, OutFmt (i.e. keeping all input-related arguments together).


Repository:
  rL LLVM

https://reviews.llvm.org/D41687