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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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));
----------------
alexshap wrote:
> jhenderson wrote:
> > alexshap wrote:
> > > alexshap wrote:
> > > > jhenderson wrote:
> > > > > Test case?
> > > > again, this Error E can be non-empty if, for example, the syscalls `open` or `write` have failed (+ there might other reasons why writeArchive has returned non-success). 
> > > > I'm somehow hesitant to test / verify it here - llvm-objcopy is just not the right place for testing writeArchive error-handling. P.S. the same errors might happen when llvm-objcopy or any other tool writes the output to a any file. 
> > > but since this error is non-recoverable / must not be ignored - we should report it and exit(1) - that's what reportError does.
> > As noted again, I'm not interested in the cause of the Error, merely the fact that the function returns an Error. In an ideal world, for every conditional branch like an if, we have a test case for it triggering the branch, and a test case for it not triggering the branch, to check and show the behaviour in each instance.
> > 
> > For example, the test case here would check that when an error is reported, a message with the correct output filename is reported. Then, if somebody accidentally changes something in this area that causes the behaviour to change, there is a test to show this.
> @jhenderson 
> 
> >I'm not interested in the cause of the Error
> 
> okay... to write a test (meaningful) you should care about the root cause and how to trigger it.
> 
> You probably meant that you want to make sure that the tool doesn't ignore the error - but as noted before, Error<T> can not be ignored by design - it's hard to imagine how this can regress,  you said "if somebody accidentally changes something in this are" - in that case the code will stop working for all the tests, the destructor of an unchecked object will crash no matter if Error<T> is empty or not. 
> 
> >In an ideal world, for every conditional branch like an if 
> 
> I think that having a good test coverage is important, but it's not necessary to overdesign it. It seems to me that blindly following this approach in the real world will cause more harm than good, in particular, because of artificially inflated test suite and massive duplication.  
> 
> Having said that, I don't want to spend so much time debating this - to unblock us I can add a test if you say how an ideal (in your opinion) test should work in this particular case. 
> And the same applies to some other common reused apis (which can return an error) (i.e. Ar->children(Err))
> I don't want to duplicate their tests here again and again (since llvm-objcopy is not the right place for it in my opinion), but since you are insisting - please, provide more details how the test (which you want to add) should work and I will add it.
> 
> okay... to write a test (meaningful) you should care about the root cause and how to trigger it.

I want to test that the error message is printed correctly. I don't care what kind of error is returned here, only that I want an error message when the library call fails. In an ideal world, we'd mock out the library call, with some function that always fails. This isn't possible in this situation, so the next best option is to trigger the failure from the library in the easiest way possible. Only when neither is possible should we skip writing a test.

> You probably meant that you want to make sure that the tool doesn't ignore the error - but as noted before, Error<T> can not be ignored by design - it's hard to imagine how this can regress, you said "if somebody accidentally changes something in this are" - in that case the code will stop working for all the tests, the destructor of an unchecked object will crash no matter if Error<T> is empty or not.

It's not what I meant exactly, and it's also not quite true. Taking a simple, and perhaps extreme example, the following code doesn't print an error (so would be a test failure) but also doesn't fail due to unchecked errors:

```
if (Error E = someLibraryCall());
```
Obviously, this isn't the case in our situation, but similar things can on occasion slip in, especially when refactoring or similar.

As for this specific check, I dug down into the library code, and it actually looks to be impossible to test this on an end-to-end level, as it requires a malformed object, which I expect would have already fired an earlier check, so I'm happy with no test here. I don't have time to look at the other checks right now.


Repository:
  rL LLVM

https://reviews.llvm.org/D48413