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

[David Blaikie via llvm-commits]

Author: dblaikie
Date: Mon Dec 10 16:09:06 2018
New Revision: 348811

URL: http://llvm.org/viewvc/llvm-project?rev=348811&view=rev
Log:
llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration

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.

Fleshed this solution out a bit further during review - it now relies on
op==/op!= comparison as the equivalent to "if (Err)" testing the Error.
So just like an Error must be checked (even if it's in a success state),
the Error hiding in the iterator must be checked after each increment
(including by comparison with another iterator - perhaps this could be
constrained to only checking if the iterator is compared to the end
iterator? Not sure it's too important).

So now even just creating the iterator and not incrementing it at all
should still assert because the Error has not been checked.

Reviewers: lhames, jakehehrlich

Differential Revision: https://reviews.llvm.org/D55235

Modified:
    llvm/trunk/include/llvm/Object/ELFTypes.h
    llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

Modified: llvm/trunk/include/llvm/Object/ELFTypes.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/ELFTypes.h?rev=348811&r1=348810&r2=348811&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Object/ELFTypes.h (original)
+++ llvm/trunk/include/llvm/Object/ELFTypes.h Mon Dec 10 16:09:06 2018
@@ -642,14 +642,19 @@ class Elf_Note_Iterator_Impl
   // 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();
     }
   }
 
@@ -657,6 +662,7 @@ class 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);
   }
@@ -670,6 +676,10 @@ public:
     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 {

Modified: llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp?rev=348811&r1=348810&r2=348811&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp (original)
+++ llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp Mon Dec 10 16:09:06 2018
@@ -123,14 +123,9 @@ findBuildID(const object::ELFFile<ELFT>
     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);
   }