Fix a bunch of assert-on-invalid-bitcode regressions after 315483

[thakis]

Summary

Expected<> calls assertIsChecked() in its dtor, and operator bool() only calls setChecked() if there's no error. So for functions that don't return an error itself, the Expected<> version needs explicit code to disarm the error that the ErrorOr<> code didn't need.

I didn't audit other revisions that converted ErrorOr<> to Expected<>, there are possibly more changes like this needed.

This fixes an assert from libLTO.dylib while linking chrome/iOS.

[thakis]

lib/LTO/LTOModule.cpp

80–84

...I'll come in again.

[thakis]

fix copy-pasto

[thakis]

rL315477, rL315475, rL315474, rL315473, rL315376, rL315354 seem to not have introduced bugs of this type as far as I can tell.

[thakis]

Now with test (only for one of the 4 changes, but the one we hit in practice. I don't see how to test the other 3). Feeling pretty good about this now.

[thakis]

Now with better test. I'm going to land this; happy to address post-commit review comments of course.

[thakis]

317010

[thakis]

I'm not subscribed to llvm-commits so replying here:

What happened to the errorToBool idea?

Apologies I didn't see that suggestion (it's here: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171030/498706.html) because it didn't make it to phab somehow. (Weren't email replies supposed to show up on phab?)

errorToErrorCode() takes an error. The new function here would take an Expected, right? Do you want

return expectedToBool(BCData);

in the first two functions? If so, that wouldn't make the 3rd any shorter. If we had an errorToBool(), the first three would look like

if (!BCData)
  return errorToBool(BCData.takeError());

(followed by

return true;

in the first two.)

None of these would help with the 4th question. Which one do you want? To me it's not obvious which one will pan out best, so it feels a bit like premature abstraction to me. But happy to implement whatever you want, I just don't understand what it is yet.

[thakis]

It could be just

errorToBool(BCData.takeError());

and errorToBool() can handle ErrorSuccess, no?

Ah! And then errorToBool() would return true for error or for non-error? I.e. should isBitcodeFile() end in

return errorToBool(BCData.takeError());

or

return !errorToBool(BCData.takeError());

It's not clear to me from the name errorToBool() if true should mean "all good" or "it's an error". The name sounds like the latter, but then all of its current callers will return its result negated.

[thakis]

(addressing comments at https://reviews.llvm.org/D42422)

lib/LTO/LTOModule.cpp

 
 LTOModule::~LTOModule() {}
 
 /// isBitcodeFile - Returns 'true' if the file (or memory contents) is LLVM
 /// bitcode.
 bool LTOModule::isBitcodeFile(const void *Mem, size_t Length) {
   Expected<MemoryBufferRef> BCData = IRObjectFile::findBitcodeInMemBuffer(
       MemoryBufferRef(StringRef((const char *)Mem, Length), "<mem>"));
-  return bool(BCData);
+  if (!BCData) {
+    consumeError(BCData.takeError());
+    return false;
+  }
+  return true;
 }
 
 bool LTOModule::isBitcodeFile(StringRef Path) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
       MemoryBuffer::getFile(Path);
   if (!BufferOrErr)
     return false;
 
   Expected<MemoryBufferRef> BCData = IRObjectFile::findBitcodeInMemBuffer(
       BufferOrErr.get()->getMemBufferRef());
-  return bool(BCData);
+  if (!BCData) {
+    consumeError(BCData.takeError());
+    return false;
+  }
+  return true;
 }
 
 bool LTOModule::isThinLTO() {
   Expected<BitcodeLTOInfo> Result = getBitcodeLTOInfo(MBRef);
   if (!Result) {
     logAllUnhandledErrors(Result.takeError(), errs(), "");
     return false;
   }
   return Result->IsThinLTO;
 }
 
 bool LTOModule::isBitcodeForTarget(MemoryBuffer *Buffer,
                                    StringRef TriplePrefix) {
   Expected<MemoryBufferRef> BCOrErr =
       IRObjectFile::findBitcodeInMemBuffer(Buffer->getMemBufferRef());
-  if (!BCOrErr)
+  if (!BCOrErr) {
+    consumeError(BCOrErr.takeError());
     return false;
+  }
   LLVMContext Context;
   ErrorOr<std::string> TripleOrErr =
       expectedToErrorOrAndEmitErrors(Context, getBitcodeTargetTriple(*BCOrErr));
   if (!TripleOrErr)
     return false;
   return StringRef(*TripleOrErr).startswith(TriplePrefix);
 }
 
 std::string LTOModule::getProducerString(MemoryBuffer *Buffer) {
   Expected<MemoryBufferRef> BCOrErr =
       IRObjectFile::findBitcodeInMemBuffer(Buffer->getMemBufferRef());
-  if (!BCOrErr)
+  if (!BCOrErr) {
+    consumeError(BCOrErr.takeError());
     return "";
+  }
   LLVMContext Context;
   ErrorOr<std::string> ProducerOrErr = expectedToErrorOrAndEmitErrors(
       Context, getBitcodeProducerString(*BCOrErr));
   if (!ProducerOrErr)
     return "";
   return *ProducerOrErr;
 }
 

test/tools/lto/no-bitcode.s

+; libLTO.dylib shouldn't assert on invalid inputs.
+; RUN: llvm-mc -triple=arm64-apple-ios7.0.0 -filetype=obj -o %t.o
+; RUN: llvm-ar r %t.a %t.o
+; RUN: %ld64 -lto_library %llvmshlibdir/libLTO.dylib -arch x86_64 -dylib -mllvm -O0 -o %t.dylib %t.a