Rollup merge of #38676 - rkruppe:llvm-check-success, r=alexcrichton

Check *all* errors in LLVMRustArchiveIterator* API

Incrementing the `Archive::child_iterator` fetches and validates the next child.
This can trigger an error, which we previously checked on the *next* call to `LLVMRustArchiveIteratorNext()`.
This means we ignore the last error if we stop iterating halfway through.
This is harmless (we don't access the child, after all) but LLVM 4.0 calls `abort()` if *any* error goes unchecked, even a success value.
This means that basically any rustc invocation that opens an archive and searches through it would die.

The solution implemented here is to change the order of operations, such that
advancing the iterator and fetching the newly-validated iterator happens in the same `Next()` call.
This keeps the error handling behavior as before but ensures all `Error`s get checked.

src/rustllvm/ArchiveWrapper.cpp

@@ -33,12 +33,15 @@ struct RustArchiveMember {
 
 
 struct RustArchiveIterator {
+    bool first;
     Archive::child_iterator cur;
     Archive::child_iterator end;
 #if LLVM_VERSION_GE(3, 9)
     Error err;
 
-    RustArchiveIterator() : err(Error::success()) { }
+    RustArchiveIterator() : first(true), err(Error::success()) { }
+#else
+    RustArchiveIterator() : first(true) { }
 #endif
 };
 
@@ -120,6 +123,7 @@ LLVMRustArchiveIteratorNew(LLVMRustArchiveRef ra) {
     rai->cur = ar->child_begin(rai->err);
     if (rai->err) {
         LLVMRustSetLastError(toString(std::move(rai->err)).c_str());
+        delete rai;
         return NULL;
     }
 #endif
@@ -129,19 +133,33 @@ LLVMRustArchiveIteratorNew(LLVMRustArchiveRef ra) {
 
 extern "C" LLVMRustArchiveChildConstRef
 LLVMRustArchiveIteratorNext(LLVMRustArchiveIteratorRef rai) {
+    if (rai->cur == rai->end) return nullptr;
+
+    // Advancing the iterator validates the next child, and this can
+    // uncover an error. LLVM requires that we check all Errors,
+    // so we only advance the iterator if we actually need to fetch
+    // the next child.
+    // This means we must not advance the iterator in the *first* call,
+    // but instead advance it *before* fetching the child in all later calls.
+    if (!rai->first) {
+        ++rai->cur;
 #if LLVM_VERSION_GE(3, 9)
-    if (rai->err) {
-        LLVMRustSetLastError(toString(std::move(rai->err)).c_str());
-        return NULL;
-    }
+        if (rai->err) {
+            LLVMRustSetLastError(toString(std::move(rai->err)).c_str());
+            return nullptr;
+        }
 #endif
-    if (rai->cur == rai->end)
-        return NULL;
+    } else {
+      rai->first = false;
+    }
+
+    if (rai->cur == rai->end) return nullptr;
+
 #if LLVM_VERSION_EQ(3, 8)
     const ErrorOr<Archive::Child>* cur = rai->cur.operator->();
     if (!*cur) {
         LLVMRustSetLastError(cur->getError().message().c_str());
-        return NULL;
+        return nullptr;
     }
     const Archive::Child &child = cur->get();
 #else
@@ -149,7 +167,6 @@ LLVMRustArchiveIteratorNext(LLVMRustArchiveIteratorRef rai) {
 #endif
     Archive::Child *ret = new Archive::Child(child);
 
-    ++rai->cur;
     return ret;
 }