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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added a comment.

James, let me try to address your concerns:

1. so basically many llvm-objcopy/llvm-strip features (if not almost everything) are applicable to static libraries as well. Roughly speaking  `llvm-objcopy <some options> lib.a` is equivalent to the following: unpack liba.a -> for each file F run `llvm-objcopy <some options> F F` -> pack all the files F into lib.a again.

However, now we have smth about 100 tests for llvm-objcopy and modifying every single test (adding extra runs for static libraries) is a lot of work and doesn't seem to be a good idea at all to me / doesn't seem to provide a lot of value. Basically in that case we would test the same code path again and again but the testing time will increase significantly. Keeping this in my mind I decided to add tests for the real-world use-case first (stripping static libraries) because it's being used in a real-world project right now.

2. the apis which return Expected<T> are ubiquitous and these types must be checked (otherwise the destructor will call terminate()),

even in llvm-objcopy we have smth like ~80 checks if i'm not mistaken (i was looking at grep -r -n -i Error ./* | wc -l), but we have ~100 tests. So very obviously 100 tests don't cover every single case even right now (and, in fact, it's far from it). I'm not sure that adding what you called "missing" tests is worth doing in this patch. For some erros (for example, Ar->children(Error)) I'm not even sure if for such a test llvm-objcopy is the right place. However, having said that, I think it's necessary to add a test for a malformed archive and for an archive with "mixed" content (.o mixed with smth else). Would that be satisfiable for you ?



================
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
----------------
jhenderson wrote:
> 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.
%basename_t.tmp is expanded by lit into the basename of %t .
so llvm-ar p %t.a %basename_t.tmp prints the content of a given file from the archive.
-x will extract a file into the current folder so it will overwrite the existing file, I somehow didn't like this.
 


Repository:
  rL LLVM

https://reviews.llvm.org/D48413