[PATCH] D48413: [llvm-objcopy] Add initial support for static libraries

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Overall, I'm happy with this change now, apart from the outstanding inline comments (I think thin archive testing plus what I mention in my newest comments). Thanks for the effort and patience!



================
Comment at: test/tools/llvm-objcopy/basic-archive-copy.test:1
+# RUN: yaml2obj %s > %t
+
----------------
jakehehrlich wrote:
> Can you add a test that has multiple objects in it? You can use one of the binaries from Input or you can duplicate the same binary.
I'd like to see one more test case using a switch that results in some modification: the first member can be modified, but the second cannot (e.g. because it is a text file). This should result in an error and no manipulation of the first object either, I think (though I again could be persuaded that all members should be manipulated if possible).


================
Comment at: test/tools/llvm-objcopy/basic-archive-copy.test:9
+# RUN: llvm-ar p %t2.a > %t3
+# RUN: cmp %t2 %t3
+
----------------
alexshap wrote:
> jakehehrlich wrote:
> > For a multi object target it would be cool to build the archive two ways. 1) un-objcopied bins -> archive -> copied archive and 2) objcopy the binaries -> archive and then check that those files are the same. 
> the archive files won't be the same since they internally contain the names of the files.
You could move them to some common name before adding them to the archive maybe?


================
Comment at: tools/llvm-objcopy/Object.cpp:39
+  handleAllErrors(BufferOrErr.takeError(), [this](const ErrorInfoBase &) {
+    error("failed to open " + getName());
+  });
----------------
alexshap wrote:
> alexshap wrote:
> > jhenderson wrote:
> > > This should use the error info's message. Otherwise, the user won't be able to know what is wrong. I think this is probably also one that is easy to test, by specifying a location in a non-existent directory?
> > the thing is that I moved this from the previous version (before my patch) without any of may changes, but okay, I'll fix it
> typo: may -> my
Sorry, I should have been clearer. I meant, I'd use the error info's message prefixed with the original "failed to open <file>", so:

`failed to open foo.a: no such file or directory`

or whatever.

Actually, this just occurred to me - is the "no such file or directory" a platform independent error message? Otherwise we won't be able to test as easily (in that case I'd keep the extra information printing, but test only up to the colon).


================
Comment at: tools/llvm-objcopy/Object.h:160
+
+class MemBuffer : public Buffer {
+  std::unique_ptr<WritableMemoryBuffer> Out;
----------------
alexshap wrote:
> jhenderson wrote:
> > I think you should just call this "MemoryBuffer" if that name doesn't clash with anything already.
> it clashes with http://llvm.org/doxygen/classllvm_1_1MemoryBuffer.html. Imo the organization of the namespace needs to be fixed  (i think it would be better to move llvm-objcopy's internals into the namespace llvm::objcopy (+ add missing "static" in many places + make use of the anonymous namespace where it's appropriate), but this stuff is unrelated to my changes (and has been an issue from the early beginning (even before i started working on llvm-objcopy)) - so if i have time I'll probably try to fix it in a separate patch
+1 for the namespacing and adding static in places, but not as part of this change.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:447
 
-std::unique_ptr<Reader> CreateReader(StringRef InputFilename,
-                                     ElfType &OutputElfType) {
-  // Right now we can only read ELF files so there's only one reader;
-  auto Out = llvm::make_unique<ELFReader>(InputFilename);
-  // We need to set the default ElfType for output.
-  OutputElfType = Out->getElfType();
-  return std::move(Out);
-}
+void ExecuteElfObjcopyOnBinary(const CopyConfig &Config, Binary &Binary,
+                               Buffer &Out) {
----------------
I quite like this solution, thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D48413