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

[David Blaikie via Phabricator via llvm-commits]

dblaikie created this revision.
dblaikie added reviewers: lhames, jakehehrlich.
Herald added subscribers: llvm-commits, rupprecht, JDevlieghere, arichardson, emaste.
Herald added a reviewer: espindola.
Herald added a reviewer: alexshap.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D55235

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


Index: tools/llvm-objcopy/ELF/ELFObjcopy.cpp
===================================================================
--- tools/llvm-objcopy/ELF/ELFObjcopy.cpp
+++ tools/llvm-objcopy/ELF/ELFObjcopy.cpp
@@ -123,16 +123,16 @@
     if (Phdr.p_type != PT_NOTE)
       continue;
     Error Err = Error::success();
-    if (Err)
-      llvm_unreachable("Error::success() was an error.");
+    ArrayRef<uint8_t> Result;
     for (const auto &Note : In.notes(Phdr, Err)) {
-      if (Err)
-        return std::move(Err);
-      if (Note.getType() == NT_GNU_BUILD_ID && Note.getName() == ELF_NOTE_GNU)
-        return Note.getDesc();
+      if (Note.getType() == NT_GNU_BUILD_ID && Note.getName() == ELF_NOTE_GNU) {
+        Result = Note.getDesc();
+        break;
+      }
     }
     if (Err)
       return std::move(Err);
+    return Result;
   }
   return createStringError(llvm::errc::invalid_argument,
                            "Could not find build ID.");
Index: include/llvm/Object/ELFTypes.h
===================================================================
--- include/llvm/Object/ELFTypes.h
+++ include/llvm/Object/ELFTypes.h
@@ -631,6 +631,7 @@
   // Stop iteration and indicate an overflow.
   void stopWithOverflowError() {
     Nhdr = nullptr;
+    ErrorAsOutParameter EAOP(Err);
     *Err = make_error<StringError>("ELF note overflows container",
                                    object_error::parse_failed);
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55235.176476.patch
Type: text/x-patch
Size: 1424 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181203/9d203ce0/attachment.bin>