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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

This functionality is presumably not unique to llvm-strip is it? My main thinking is that everything so far has been written as a feature in llvm-objcopy first and tested on that basis, and then later on llvm-strip, and I feel like that would be a sensible way round of doing this here too.

What happens to non-object files in archives when we try to manipulate them? Please add a test for this.

There are a number of cases where you are reporting an error. Are any of them untestable? Please add any missing tests for them too.

Would the changes related to the Buffer make sense as a separate and earlier refactor? It would help reduce the number of changes in this review.



================
Comment at: test/tools/llvm-objcopy/strip-all.test:27
+# RUN: llvm-strip %t.a
+# RUN: llvm-ar -p %t.a %basename_t.tmp >%t6
+# RUN: cmp %t2 %t6
----------------
The "-" part of "-p" is unnecessary so can be removed (either that or you should add it to "crs" for consistency).

What is the purpose of "%basename_t.tmp"?

Does lit use it's own redirection mechanism? I ask because on Windows Powershell encodes files created via ">" as UTF-16, which obviously isn't suitable for binary files. I'd be more inclined to use the extract operation (x) to achieve the same thing, as it's less likely to cause a problem.


================
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();
----------------
This should be in its own function, I think.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:456
+    Error Err = Error::success();
+    for (auto &Child : Ar->children(Err)) {
+      Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
----------------
Probably need to avoid using auto here, as the type isn't obvious to me.

Ditto at the uses of auto further down.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:468
+          CreateWriter(Config, *ChildObj, MB, ChildReader.getElfType());
+      HandleArgs(Config, *ChildObj, ChildReader, ChildReader.getElfType());
+      ChildWriter->finalize();
----------------
Can we swap this to before CreateWriter?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:479-480
+    }
+    if (Err)
+      reportError(Config.InputFilename, std::move(Err));
+    if (Error E = writeArchive(Config.OutputFilename, NewArchiveMembers, true,
----------------
Under what circumstances can we get a non-success value of Err to here?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:481-483
+    if (Error E = writeArchive(Config.OutputFilename, NewArchiveMembers, true,
+                               Ar->kind(), true, Ar->isThin()))
+      reportError(Config.OutputFilename, std::move(E));
----------------
Test case?


Repository:
  rL LLVM

https://reviews.llvm.org/D48413