[PATCH] D88827: [llvm-objcopy][NFC] Move core implementation of llvm-objcopy into separate library.

Wed Oct 7 03:53:48 PDT 2020

jhenderson added a comment.

Looks pretty good from my point of view. Some general comments:

1. The functions in the library header need doxygen style comments to explain the interface. Possibly these could be added in a follow-up patch.
2. I think if we are moving files around, now is a good time to run a blanket clang-format on the files. Whether that should be done as part of the patch or a separate follow-on one, I don't know.
3. Probably not this patch, but we should consider our testing strategy for the libray and llvm-objcopy going forward. Some examples for this dicussion: should we move the existing lit tests? Should we port/duplicate some of them as gtest unit tests? How should new library features be tested (gtest or lit)? How should new llvm-strip/objcopy/... options be tested (bearing in mind in theory there should be library testing etc etc)? I think this sort of discussion probably belongs on the mailing list.

I think keeping the surface area of the library down to just the executeObjcopyOn* functions plus the config struct is a pretty good example of a facade style design pattern, and I think makes sense in this context for at least the first version. This assumes that the use-case is a simple in-process read-in/transform/write-out (possibly to a memory buffer for further operations by other tools). We might want to expose other bits of the process and provide other ways of driving the objcopy process in the future (e.g. making it more interactive somehow), but I think that is an extension for later. We do need to nail down the initial use-case(s) though to make sure we don't produce something that isn't useful.

================
Comment at: llvm/include/llvm/ObjCopy/Buffer.h:23-24
 // abstract interface and doesn't depend on a particular implementation.
 // TODO: refactor the buffer classes in LLVM to enable us to use them here
 // directly.
 class Buffer {
----------------
Maybe as part of a separate patch, it would be worth taking a look at this TODO. It would be great if the Buffer could be removed from the library API and generic LLVM buffers used instead (for example an in-memory buffer or a file buffer, depending on what people want to do).

================
Comment at: llvm/include/llvm/ObjCopy/CopyConfig.h:126-127

 // Matcher that checks symbol or section names against the command line flags
 // provided for that option.
 class NameMatcher {
----------------
This comment probably needs updating to better match the new usage - but see out-of-line comment.

================
Comment at: llvm/include/llvm/ObjCopy/ObjCopy.h:19-22
+/// The function applies transformation described by Config to
+/// the specified binary and writes result into the Out. It does
+/// the dispatch based on the format of the input binary
+/// (ELF, MachO or COFF).
----------------
I think details of the output format shouldn't be described in the comment - theoretically objcopy could even mutate from one object format to another (see e.g. the IHEX stuff).

================
Comment at: llvm/include/llvm/ObjCopy/ObjCopy.h:26-28
+/// The function applies transformation described by Config to
+/// the specified archive. It does the dispatch based on the format
+/// of the input binary (ELF, MachO or COFF).
----------------

================
Comment at: llvm/lib/ObjCopy/COFF/Object.cpp:1
 //===- Object.cpp ---------------------------------------------------------===//
 //
----------------
Whilst you're moving this and the equivalent files for other formats around, could you please rename them to be obvious from the filename which format they are for (same goes for their headers), please? For example, COFF/Object.cpp -> COFF/COFFObject.cpp. The reason for this is that when using the Visual Studio IDE, all the "Object.cpp" files end up listed next to each other in the file browser, and the only way of figuring out which is which is by opening them and seeing.

================
Comment at: llvm/lib/ObjCopy/wasm/Reader.h:9

-#ifndef LLVM_TOOLS_LLVM_OBJCOPY_WASM_READER_H
-#define LLVM_TOOLS_LLVM_OBJCOPY_WASM_READER_H
+#ifndef LLVM_LIB_LLVM_OBJCOPY_WASM_READER_H
+#define LLVM_LIB_LLVM_OBJCOPY_WASM_READER_H
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88827