[llvm-objcopy][NFC] More error propagation (executeObjcopyOnArchive)

[rupprecht]

Summary

Replace some reportError() calls with error propagation that was missed from rL352625.

Note this also adds an error check during Archive iteration that was being hidden by a different error check before:

for (const Archive::Child &Child : Ar.children(Err)) {
  Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
  if (!ChildOrErr)
    // This aborts, so Err is never checked
    reportError(Ar.getFileName(), ChildOrErr.takeError());

Err is being checked after the loop, so during happy runs, everything is fine. But when reportError is changed to return the error instead of aborting, the fact that Err is never checked is now noticed in tests that trigger an error during the loop.

[dblaikie]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

155–156 ↗

(On Diff #184315)

Can this happen? I thought usually the iterator would exit out - rather than giving a valid Child while also providing an error?

Or is this to workaround the "Error is not checked on early returns"? If that's the issue, we should fix the iterator - as per the thread I pulled @lhames into a while back, I think?

[rupprecht]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

155–156 ↗

(On Diff #184315)

It's here because it's happening :D

Yeah, fixing the iterator would be preferable. Do you have a link to the thread?

[dblaikie]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

155–156 ↗

(On Diff #184315)

Sorry, I meant can an error actually occur there - it seems like the error would always be "success" here - but the lack of checking that it is success causes an assertion failure later on at the early exits in the loop. So the "return Err;" here is, I think, unreachable (which is a sign maybe this isn't the right code/best path forward).

Yeah, here's the thread: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190114/619563.html

[lhames]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

155–156 ↗

(On Diff #184315)

Right: That *should* be unreachable. If 'Err' is raised, the iterator should be moved to the end of the list, so 'Err' should never be a failure value inside the loop body.

If that is happening then it sounds like the iterator is broken.

The 'Err' value *does* have to be checked after the loop body though.

[rupprecht]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

155–156 ↗

(On Diff #184315)

At least from the example that I'm looking at (ninja check-llvm-tools-llvm-objcopy, test case tools/llvm-objcopy/ELF/strip-debug.test), Err is success, and it is checked afterward (line 178 on this patch, 174 in trunk). The scenario is that Child is valid, *but* an error is created inside this loop -- the archive contains an "object" that's actually a text file, hence Child.getAsBinary() fails.

I think what I'd like to see, and sounds like what you're suggesting, is that the Archive child iterator should make sure that Err is required to be checked at the end of loop iteration (either by hitting the end or if there's an error in iterator increments), but *not* be required to be checked if the iterator is a non-end position -- it looks like that's what rL348811 did for Note iterators, but I'm struggling to understand that patch and apply it to Archive child iterators.

[lhames]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

155–156 ↗

(On Diff #184315)

My understanding is that the archive child iterator already auto-increments to end upon raising an error. If that understanding is correct then:

if (Err)
  return Err;

is dead code, and can/should be removed.

If you are seeing an instance where the return path above is actually taken the Archive's child_iterator should be changed as you described. I am happy to help with that if needed.

Err will already have to be checked outside the loop by virtual of the usual llvm::Error guarantees (i.e. failure to check results in an assertion at runtime).

[dblaikie]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

155–156 ↗

(On Diff #184315)

That's certainly what I'd suggest - and what I did to the other iterator.

Essentially comparing the iterator to an end iterator is equivalent to the "is there an error" test - so there should be no need to test for an error /again/ if you're in the loop body (you tested it that lead you to get into the loop in the first place), and if you're outside the loop, you can re-test the error (because you don't know if you exited because of hitting an error, or running out of elements) and then handle the error if there is one.

The open discussion I was hoping to rope @lhames into was about the nuances of this solution I used - which was to clear the "unchecked" state of the Error whenever the iterator is compared with an end iterator.

But what if you computed some non-end iterator and used that as your bound? Perhaps that should also clear the error state, if you were ever iterating over a subrange.

155–156 ↗

(On Diff #184315)

Ah, Lang - so the "return Err;" is dead code here. We all agree that line of code never executes.

the issue is that any attempt to return early from inside the loop results in a failure in Error's invariants - the Error goes out of scope as the early return happens, and the Error hasn't been checked & so assertion failure occurs.

Checking the Error (you could replace "if (Err) return Err;" with "(void)(bool)Err;" - but if you /remove/ it, and you exercise one of the early returns inside the loop, it would fail Errors invariants) unconditionally at the start of the loop will only ever check a success state Error, but it is important.

The other thread I linked to ( http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190114/619563.html ) explains the issue and shows my approach in another fallible iterator & I was hoping to discuss whether that seems like the right idiom/general fix going forwards.

[rupprecht]

• Cast err to check instead of using dead if block

[lhames]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

155–156 ↗

(On Diff #184315)

the issue is that any attempt to return early from inside the loop results in a failure in Error's invariants - the Error goes out of scope as the early return happens, and the Error hasn't been checked & so assertion failure occurs.

Ahh. My bad. I'm on the same page now. Let me read that thread you posted and get back to you. :)

[jhenderson]

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

157 ↗

(On Diff #184397)

I think this should be cantFail(Err), right?

[dblaikie]

Any new test coverage for these new error propagation paths?

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

157 ↗

(On Diff #184397)

Oh, good call!

[jhenderson]

In D57462#1379689, @dblaikie wrote:

Any new test coverage for these new error propagation paths?

I believe that there are actually no new errors that can be produced. They're just reported further up the stack rather than deep inside the llvm-objcopy code. Most (all?) of the error cases that can be tested should already be being tested.

[rupprecht]

• Use cantFail instead of cast to bool

[rupprecht]

In D57462#1380244, @jhenderson wrote:

In D57462#1379689, @dblaikie wrote:

Any new test coverage for these new error propagation paths?

I believe that there are actually no new errors that can be produced. They're just reported further up the stack rather than deep inside the llvm-objcopy code. Most (all?) of the error cases that can be tested should already be being tested.

Yep, these aren't new errors. And specifically tools/llvm-objcopy/ELF/strip-debug.test is what's requiring the cantFail() check, so we can use that as a test to make sure the iterator is fixed.

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

157 ↗

(On Diff #184397)

I think cantFail() is meant to wrap calls directly, e.g. cantFail(doSomething()), as it takes Error (which is uncopyable) by value (i.e. move-only).

So that would have to be cantFail(std::move(Err)). Done.

[dblaikie]

Sounds good - thanks!

llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp

 using namespace llvm::objcopy;
 
 // For regular archives this function simply calls llvm::writeArchive,
 // For thin archives it writes the archive file itself as well as its members.
 static Error deepWriteArchive(StringRef ArcName,
                               ArrayRef<NewArchiveMember> NewMembers,
                               bool WriteSymtab, object::Archive::Kind Kind,
                               bool Deterministic, bool Thin) {
-  Error E =
-      writeArchive(ArcName, NewMembers, WriteSymtab, Kind, Deterministic, Thin);
-  if (!Thin || E)
-    return E;
+  if (Error E = writeArchive(ArcName, NewMembers, WriteSymtab, Kind,
+                             Deterministic, Thin))
+    return createFileError(ArcName, std::move(E));
+
+  if (!Thin)
+    return Error::success();
+
   for (const NewArchiveMember &Member : NewMembers) {
     // Internally, FileBuffer will use the buffer created by
     // FileOutputBuffer::create, for regular files (that is the case for
     // deepWriteArchive) FileOutputBuffer::create will return OnDiskBuffer.
     // OnDiskBuffer uses a temporary file and then renames it. So in reality
     // there is no inefficiency / duplicated in-memory buffers in this case. For
     // now in-memory buffers can not be completely avoided since
     // NewArchiveMember still requires them even though writeArchive does not
return createStringError(object_error::invalid_file_type,
                              "Unsupported object file format");
 }
 
 static Error executeObjcopyOnArchive(const CopyConfig &Config,
                                      const Archive &Ar) {
   std::vector<NewArchiveMember> NewArchiveMembers;
   Error Err = Error::success();
   for (const Archive::Child &Child : Ar.children(Err)) {
+    // FIXME: Archive::child_iterator requires that Err be checked *during* loop
+    // iteration, and hence does not allow early returns.
+    cantFail(std::move(Err));
     Expected<std::unique_ptr<Binary>> ChildOrErr = Child.getAsBinary();
     if (!ChildOrErr)
-      reportError(Ar.getFileName(), ChildOrErr.takeError());
+      return createFileError(Ar.getFileName(), ChildOrErr.takeError());
     Binary *Bin = ChildOrErr->get();
 
     Expected<StringRef> ChildNameOrErr = Child.getName();
     if (!ChildNameOrErr)
-      reportError(Ar.getFileName(), ChildNameOrErr.takeError());
+      return createFileError(Ar.getFileName(), ChildNameOrErr.takeError());
 
     MemBuffer MB(ChildNameOrErr.get());
     if (Error E = executeObjcopyOnBinary(Config, *Bin, MB))
       return E;
 
     Expected<NewArchiveMember> Member =
         NewArchiveMember::getOldMember(Child, Config.DeterministicArchives);
     if (!Member)
-      reportError(Ar.getFileName(), Member.takeError());
+      return createFileError(Ar.getFileName(), Member.takeError());
     Member->Buf = MB.releaseMemoryBuffer();
     Member->MemberName = Member->Buf->getBufferIdentifier();
     NewArchiveMembers.push_back(std::move(*Member));
   }
-
   if (Err)
-    reportError(Config.InputFilename, std::move(Err));
-  if (Error E = deepWriteArchive(Config.OutputFilename, NewArchiveMembers,
-                                 Ar.hasSymbolTable(), Ar.kind(),
-                                 Config.DeterministicArchives, Ar.isThin()))
-    reportError(Config.OutputFilename, std::move(E));
-  return Error::success();
+    return createFileError(Config.InputFilename, std::move(Err));
+
+  return deepWriteArchive(Config.OutputFilename, NewArchiveMembers,
+                          Config.DeterministicArchives, Ar.isThin());
 }
 
 static void restoreDateOnFile(StringRef Filename,
                               const sys::fs::file_status &Stat) {
   int FD;
 
   if (auto EC =
           sys::fs::openFileForWrite(Filename, FD, sys::fs::CD_OpenExisting))