llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration

[dblaikie]

Summary

Using an Error as an out parameter from an indirect operation like iteration as described in the documentation ( http://llvm.org/docs/ProgrammersManual.html#building-fallible-iterators-and-iterator-ranges ) seems to be a little fussy - so here's /one/ possible solution, though I'm not sure it's the right one.

Alternatively such APIs may be better off being switched to a standard algorithm style, where they take a lambda to do the iteration work that is then called back into (eg: "Error e = obj.for_each_note([](const Note& N) { ... });"). This would be safer than having an unwritten assumption that the user of such an iteration cannot return early from the inside of the function - and must always exit through the gift shop... I mean error checking. (even though it's guaranteed that if you're mid-way through processing an iteration, it's not in an error state).

Alternatively we'd need some other (the super untrustworthy/thing we've generally tried to avoid) error handling primitive that actually clears the error state entirely so it's safe to ignore.

[dblaikie]

Strengthen require error checking from the iterator

[dblaikie]

Ah, think I found a better solution - since lhames is out of town, this might be good enough that I'll commit it now & can leave the discussion for another time.

[dblaikie]

Ah, think I found a better solution - since lhames is out of town, this might be good enough that I'll commit it now & can leave the discussion for another time.

By clearing the error in the iterator ctor, then setting it to success or failure when the iterator is incremented to the end (or a premature error state) - any iteration that reaches the end must check the error, but early returns during the iteration don't have to check the Error.

@jakehehrlich - seems there's some missing test coverage, though. Neither of the tests in the original commit fail if the note is not found (so the iteration over teh notes does reach the end, and the error should be checked - if I remove the error checking there, seems nothing fails). Is that possible to test? Could you please add a test whenever you get a chance & let me know, so we can check that this change I'm proposing/committing would've caught missing error handling in such a case?

[dblaikie]

To comment further - the example in the docs which involves llvm::object::Archive's "range children(Error)" accessor isn't robust against this sort of problem either - if any caller returns early from within a loop over the children, I think they'll hit an assert too. (yeah, just tested that - and it fails as predicted)

[jakehehrlich]

Thanks! This looks good to me.

[jakehehrlich]

Also I'll put up a review for a change that triggers the overflow error and other things. One moment.

[MaskRay]

include/llvm/Object/ELFTypes.h

664 ↗

(On Diff #176490)

Why does here use consumeError but not ErrorAsOutParameter?

[dblaikie]

include/llvm/Object/ELFTypes.h

664 ↗

(On Diff #176490)

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.

[MaskRay]

include/llvm/Object/ELFTypes.h

664 ↗

(On Diff #176490)

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.

[dblaikie]

include/llvm/Object/ELFTypes.h

664 ↗

(On Diff #176490)

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.

llvm/trunk/include/llvm/Object/ELFTypes.h

class Elf_Note_Iterator_Impl
 
   // Advance Nhdr by NoteSize bytes, starting from NhdrPos.
   //
   // Assumes NoteSize <= RemainingSize. Ensures Nhdr->getSize() <= RemainingSize
   // upon returning. Handles stopping iteration when reaching the end of the
   // container, either cleanly or with an overflow error.
   void advanceNhdr(const uint8_t *NhdrPos, size_t NoteSize) {
     RemainingSize -= NoteSize;
-    if (RemainingSize == 0u)
+    if (RemainingSize == 0u) {
+      // Ensure that if the iterator walks to the end, the error is checked
+      // afterwards.
+      *Err = Error::success();
       Nhdr = nullptr;
-    else if (sizeof(*Nhdr) > RemainingSize)
+    } else if (sizeof(*Nhdr) > RemainingSize)
       stopWithOverflowError();
     else {
       Nhdr = reinterpret_cast<const Elf_Nhdr_Impl<ELFT> *>(NhdrPos + NoteSize);
       if (Nhdr->getSize() > RemainingSize)
         stopWithOverflowError();
+      else
+        *Err = Error::success();
     }
   }
 
   Elf_Note_Iterator_Impl() {}
   explicit Elf_Note_Iterator_Impl(Error &Err) : Err(&Err) {}
   Elf_Note_Iterator_Impl(const uint8_t *Start, size_t Size, Error &Err)
       : RemainingSize(Size), Err(&Err) {
+    consumeError(std::move(Err));
     assert(Start && "ELF note iterator starting at NULL");
     advanceNhdr(Start, 0u);
   }
 
 public:
   Elf_Note_Iterator_Impl &operator++() {
     assert(Nhdr && "incremented ELF note end iterator");
     const uint8_t *NhdrPos = reinterpret_cast<const uint8_t *>(Nhdr);
     size_t NoteSize = Nhdr->getSize();
     advanceNhdr(NhdrPos, NoteSize);
     return *this;
   }
   bool operator==(Elf_Note_Iterator_Impl Other) const {
+    if (!Nhdr)
+      (void)(bool)(*Other.Err);
+    if (!Other.Nhdr)
+      (void)(bool)(*Err);
     return Nhdr == Other.Nhdr;
   }
   bool operator!=(Elf_Note_Iterator_Impl Other) const {
     return !(*this == Other);
   }
   Elf_Note_Impl<ELFT> operator*() const {
     assert(Nhdr && "dereferenced ELF note end iterator");
     return Elf_Note_Impl<ELFT>(*Nhdr);

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

 
 template <class ELFT>
 static Expected<ArrayRef<uint8_t>>
 findBuildID(const object::ELFFile<ELFT> &In) {
   for (const auto &Phdr : unwrapOrError(In.program_headers())) {
     if (Phdr.p_type != PT_NOTE)
       continue;
     Error Err = Error::success();
-    if (Err)
-      llvm_unreachable("Error::success() was an error.");
-    for (const auto &Note : In.notes(Phdr, Err)) {
-      if (Err)
-        return std::move(Err);
+    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);
   }
   return createStringError(llvm::errc::invalid_argument,
                            "Could not find build ID.");
 }
 
 static Expected<ArrayRef<uint8_t>>