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

Thu Nov 18 00:41:10 PST 2021

jhenderson added a comment.

In D88827#3118304 <https://reviews.llvm.org/D88827#3118304>, @avl wrote:

> ping,

Apologies for not getting to this - a number of other reviews have used up what time I've had to do reviews.

This patch is looking very close to being ready, but not quite there yet.

================
Comment at: llvm/include/llvm/Object/ObjCopy/COFF/COFFObjcopy.h:26-28
+/// Applies the transformations described by \p Config to
+/// \p In(FileFormat::Unspecified) and writes the result into \p Out.
+/// \returns any Error encountered whilst performing the operation.
----------------
I've no issue with adding comments like these, but could you spin them off into separate patches, please, as they are independently useful, and this will help reduce the size of this patch.

Same applies for other new doc comments.

================
Comment at: llvm/include/llvm/Object/ObjCopy/ConfigManager.h:24-25
+
+// ConfigManager keeps all configurations and prepare
+// format-specific options.
+struct ConfigManager : public MultiFormatConfig {
----------------
This should be a doc comment if it is needed. I don't think the comment is particularly useful in its current form. The functionality is fairly obvious from its interface.

================
Comment at: llvm/include/llvm/Object/ObjCopy/ConfigManager.h:33
+
+  Expected<const COFFConfig &> getCOFFConfig() const override {
+    if (!Common.SplitDWO.empty() || !Common.SymbolsPrefix.empty() ||
----------------
The functionality in this and similar functions for wasm and Mach-O should be moved to a .cpp file. It's not unlikely that it will change over time, so we don't want to force rebuilds due to it being unnecessarily in the header.

================
Comment at: llvm/include/llvm/Object/ObjCopy/ObjCopy.h:12-18
+#include "llvm/Object/Archive.h"
+#include "llvm/Object/ArchiveWriter.h"
+#include "llvm/Object/Binary.h"
+#include "llvm/Object/ObjCopy/CommonConfig.h"
+#include "llvm/Object/ObjCopy/MultiFormatConfig.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/raw_ostream.h"
----------------
I think you can forward declare most of these classes, and include the headerse in the .cpp rather than having this header include them all.

I think about the only ones you likely need are Error.h, and vector.

================
Comment at: llvm/lib/ObjCopy/COFF/Object.cpp:1
 //===- Object.cpp ---------------------------------------------------------===//
 //
----------------
alexander-shaposhnikov wrote:
> avl wrote:
> > avl wrote:
> > > alexander-shaposhnikov wrote:
> > > > avl wrote:
> > > > > jhenderson wrote:
> > > > > > 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.
> > > > > ok.
> > > > @jhenderson, I'm sorry to disagree, but renaming files this way doesn't seem to be a good idea and the provided justification doesn't appear to be sufficient.
> > > > Since this file contains the implementation of what's declared in Object.h I would strongly prefer to have it named Object.cpp given it is already located in the corresponding folder. Visual Studio IDE might have some peculiarities but having consistent naming is important, adding such prefixes doesn't seem to be a good approach. 
> > > Would it be OK, If both of the files would be renamed Object.h -> COFFObject.h and Object.cpp->COFFObject.cpp ?
> > > @jhenderson, I'm sorry to disagree, but renaming files this way doesn't seem to be a good idea and the provided justification doesn't appear to be sufficient.
> > > Since this file contains the implementation of what's declared in Object.h I would strongly prefer to have it named Object.cpp given it is already located in the corresponding folder. Visual Studio IDE might have some peculiarities but having consistent naming is important, adding such prefixes doesn't seem to be a good approach. 
> > 
> > @alexshap Could you explain this renaming thing, please? i.e. if both header file COFF/Object.h and src file COFF/Object.cpp would be renamed(COFF/COFFObject.h, COFF/COFFObject.cpp), would it be OK? 
> I'm very sorry, but i still think that the old names were good, adding these prefixes is unnecessary and makes things less intuitive (e.g. class Object is described in Object.h).
Actually, I think the way to solve this is to use some CMake functionality to stick specific files in IDE folders, so that they don't end up clashing: in the Visual Studio IDE, for example, I see 4 Object.h files next to each other in the Header Files group, but there's no indication which is which. Instead, we could create COFF, ELF etc sub-groups for the headers and source files. I believe https://cmake.org/cmake/help/latest/command/source_group.html is the relevant piece of CMake.

================
Comment at: llvm/lib/Object/CMakeLists.txt:32-47
+  ObjCopy/ObjCopy.cpp
+  ObjCopy/COFF/COFFObjcopy.cpp
+  ObjCopy/COFF/Object.cpp
+  ObjCopy/COFF/Reader.cpp
+  ObjCopy/COFF/Writer.cpp
+  ObjCopy/ELF/ELFObjcopy.cpp
+  ObjCopy/ELF/Object.cpp
----------------
I thought ObjCopy was going to be its own distinct library, not a part of the Object library? It would make more sense to me for it to be separate, as it's a fairly distinct piece of functionality.

================
Comment at: llvm/lib/Object/ObjCopy/ELF/ELFObjcopy.cpp:171
 static Error makeStringError(std::error_code EC, const Twine &Msg,
                              Ts &&... Args) {
   std::string FullMsg = (EC.message() + ": " + Msg).str();
----------------
As you're renaming files, it's probably worth doing a complete clang-format of the whole file at the same time. Applies to all files with clang-format issues in otherwise untouched code.

================
Comment at: llvm/unittests/Object/ObjCopyTest.cpp:9
+
+#include "llvm/Object/ObjCopy/ObjCopy.h"
+#include "llvm/ADT/SmallString.h"
----------------
Here and in other new files, if you haven't already, please make sure the include set is minimal for the code in the new file.

================
Comment at: llvm/unittests/Object/ObjCopyTest.cpp:35
+
+void copySimpleFileImpl(const char *YamlCreationString,
+                        std::function<bool(const Binary &File)> IsValidFormat) {
----------------
As this function is called from several places, you may want to add some tracing to it: https://github.com/google/googletest/blob/main/docs/advanced.md#adding-traces-to-assertions

If you don't, and the test fails, you may not be able to easily tell which test is causing the failure.

================
Comment at: llvm/unittests/Object/ObjCopyTest.cpp:39
+
+  // Create Object file from yaml description.
+  SmallString<0> Storage;
----------------
I believe YAML is an acronym, so should be all-caps in comments.

================
Comment at: llvm/unittests/Object/ObjCopyTest.cpp:49-50
+  int FD;
+  ASSERT_FALSE(sys::fs::createTemporaryFile("a", "out", FD, Path));
+  FileRemover Cleanup(Path);
+
----------------
I was under the impression that temporary files are supposed to be deleted automatically (I might be wrong), so you wouldn't need the `FileRemover`?

More generally, the need to create a concrete file in the unit test makes me wonder whether we could enhance the objcopy API to take an object of some kind that could represent data in memory (as an alternative to on-disk). I believe such an object hierarchy already exists within LLVM, although the name escapes me right now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88827