This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by rupprecht on Jan 30 2019, 9:43 AM.

Details

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.

Event Timeline

rupprecht created this revision.Jan 30 2019, 9:43 AM
dblaikie added inline comments.Jan 30 2019, 10:42 AM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
155–156

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 marked an inline comment as done.Jan 30 2019, 10:58 AM
rupprecht added inline comments.
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
155–156

It's here because it's happening :D

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

dblaikie added inline comments.Jan 30 2019, 11:03 AM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
155–156

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 added inline comments.Jan 30 2019, 2:20 PM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
155–156

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 added inline comments.Jan 30 2019, 3:36 PM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
155–156

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 added inline comments.Jan 30 2019, 3:42 PM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
155–156

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 added inline comments.Jan 30 2019, 3:48 PM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
155–156

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

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 updated this revision to Diff 184397.Jan 30 2019, 4:00 PM
  • Cast err to check instead of using dead if block
lhames added inline comments.Jan 30 2019, 4:03 PM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
155–156

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 added inline comments.Jan 31 2019, 3:21 AM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
157

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

rupprecht set the repository for this revision to rL LLVM.Jan 31 2019, 9:19 AM

Any new test coverage for these new error propagation paths?

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

Oh, good call!

Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2019, 3:32 PM

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 updated this revision to Diff 184743.Feb 1 2019, 7:00 AM
rupprecht marked an inline comment as done.
  • Use cantFail instead of cast to bool
rupprecht marked 3 inline comments as done.Feb 1 2019, 7:03 AM

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

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 accepted this revision.Feb 1 2019, 7:53 AM

Sounds good - thanks!

This revision is now accepted and ready to land.Feb 1 2019, 7:53 AM
This revision was automatically updated to reflect the committed changes.
rupprecht marked an inline comment as done.