[PATCH] D55235: llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration

[David Blaikie via Phabricator via llvm-commits]

dblaikie marked an inline comment as done.
dblaikie added inline comments.


================
Comment at: include/llvm/Object/ELFTypes.h:664
       : RemainingSize(Size), Err(&Err) {
+    consumeError(std::move(Err));
     assert(Start && "ELF note iterator starting at NULL");
----------------
MaskRay wrote:
> dblaikie wrote:
> > MaskRay wrote:
> > > Why does here use `consumeError` but not `ErrorAsOutParameter`?
> > ErrorAsOutParameter essentially temporarily clears the "checked" state of the Error - inclearing the state at the end of the scope (when ErrorAsOutParameter's dtor is called).
> > 
> > Using ErrorAsOutParameter here would cause the caller to need to inspect the Error state even during an early return from within a loop - essentially when the iterator is incremented successfully, that's assumed to be "checked" (we could get fussier and only do that assumption when the increment was then compared to an end iterator and returned false - but in practice this is probably close enough).
> > 
> > I know this is a bit hairy & not super clear it's the best thing, but it's my best guess so far.
> I see the point.
> 
>     for (const auto &Note : In.notes(Phdr, Err))
>       if (Note.getType() == NT_GNU_BUILD_ID && Note.getName() == ELF_NOTE_GNU)
>         return Note.getDesc();
>     if (Err)
>       return std::move(Err);
> 
> To make early return (here is one of 3 call sites of `notes`) work without checking the `Error` state, `consumeError` instead of `ErrorAsOutParameter` should be used.
> 
> How about putting `ErrorAsOutParameter EAOP(Err);` to `advanceNhdr` so that `consumeError` can be removed from the ctor and the `ErrorAsOutParameter` can be removed from `stopWithOverflowError`? This seems to be similar to other range uses.
> 
>     void advanceNhdr(const uint8_t *NhdrPos, size_t NoteSize) {
>       ErrorAsOutParameter EAOP(Err);
>       RemainingSize -= NoteSize;
> 
> With the change, the early return pattern will have to check the `Error` state.
> With the change, the early return pattern will have to check the Error state.

The goal is to ensure this is /not/ needed - because the loop is only entered if the Error is unset, so early returns that (reasonably - because they're running under a precondition that the Error is unset) don't check the Error would assert/crash. That's undesirable.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55235/new/

https://reviews.llvm.org/D55235