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

Jake Ehrlich via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 28 15:46:43 PDT 2018


jakehehrlich added inline comments.


================
Comment at: test/tools/llvm-objcopy/basic-archive-copy.test:1
+# RUN: yaml2obj %s > %t
+
----------------
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.


================
Comment at: test/tools/llvm-objcopy/basic-archive-copy.test:9
+# RUN: llvm-ar p %t2.a > %t3
+# RUN: cmp %t2 %t3
+
----------------
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. 


================
Comment at: tools/llvm-objcopy/Object.h:130
+
+class Buffer {
+  StringRef Name;
----------------
jhenderson wrote:
> jhenderson wrote:
> > alexshap wrote:
> > > alexshap wrote:
> > > > jhenderson wrote:
> > > > > I'm probably missing something, but what's wrong with the existing buffer classes in llvm? Would it make more sense to extend them in the general case, rather than a specific reimplementation here?
> > > > > 
> > > > > It would be nice to see some comments describing what the interface functions do.
> > > > it's not a reimplementation per se. FileOutputBuffer and WritableMemoryBuffer share a big portion of the interface but don't have a common base class. They are used in many places across  llvm - so i didn't want to change them for now. I've tried several approaches, I liked this one for several reasons: (imo) it's cleaner than the other options, the concerns are nicely factored out, the writers don't depend on the implementation of the buffers (but depend on the interface). 
> > > i'd prefer to add a TODO here to refactor the buffer classes in llvm and then get rid of these, rather than blocking this patch or doing it here. 
> > Okay, that's fair, I think, though I'd like @jakehehrlich's thoughts on it. I would like a comment along with the TODO explaining why we chose do it this way then.
> I think you've been a bit premature in wrapping with this comment. You can just write it all on a single line, and then let clang-format wrap it appropriately.
I'm fine with just a TODO, refactoring that in LLVM sounds like a massive refactor that could make some people unhappy about vtables and size increases and such. The right way forwards is to make LLVM support both via a common interface though.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:454-484
+    std::vector<NewArchiveMember> NewArchiveMembers;
+    Error Err = Error::success();
+    for (auto &Child : Ar->children(Err)) {
+      Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
+      if (!ChildOrErr)
+        reportError(Ar->getFileName(), ChildOrErr.takeError());
+      Expected<StringRef> ChildNameOrErr = Child.getName();
----------------
jhenderson wrote:
> This should be in its own function, I think.
+1 on this point.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:485
 
 void ExecuteElfObjcopy(const CopyConfig &Config) {
+  Expected<OwningBinary<llvm::object::Binary>> BinaryOrErr =
----------------
So I had this idea that CopyConfig would eventually contain (or be wrapped by something that does) the things that do the Reader and Writer parts of this. The idea was that then there could be one function that took one object describing what needs to happen. That function would then split off if it found an archive and invoke itself on each of the elements of the archive. Could you consider the applicability of doing that here? E.h. Could ExecuteElfObjcopyOnArchive be modified to recursively call ExecuteElfObjcopy on it's children? This would mean lifting file reading up to a higher layer.


Repository:
  rL LLVM

https://reviews.llvm.org/D48413





More information about the llvm-commits mailing list