[llvm] Object: Don't error out on malformed bitcode files. (PR #96848)

[David Blaikie via llvm-commits]

================
@@ -482,16 +482,47 @@ static uint64_t computeHeadersSize(object::Archive::Kind Kind,
 }
 
 static Expected<std::unique_ptr<SymbolicFile>>
-getSymbolicFile(MemoryBufferRef Buf, LLVMContext &Context) {
+getSymbolicFile(MemoryBufferRef Buf, LLVMContext &Context,
+                object::Archive::Kind Kind) {
   const file_magic Type = identify_magic(Buf.getBuffer());
   // Don't attempt to read non-symbolic file types.
   if (!object::SymbolicFile::isSymbolicFile(Type, &Context))
     return nullptr;
   if (Type == file_magic::bitcode) {
     auto ObjOrErr = object::SymbolicFile::createSymbolicFile(
         Buf, file_magic::bitcode, &Context);
-    if (!ObjOrErr)
-      return ObjOrErr.takeError();
+    // An error reading a bitcode file most likely indicates that the file
+    // was created by a compiler from the future. Normally we don't try to
+    // implement forwards compatibility for bitcode files, but when creating an
+    // archive we can implement best-effort forwards compatibility by treating
+    // the file as a blob and not creating symbol index entries for it. lld and
+    // mold ignore the archive symbol index, so provided that you use one of
+    // these linkers, LTO will work as long as lld or the gold plugin is newer
+    // than the compiler. We only ignore errors if the archive format is one
+    // that is supported by a linker that is known to ignore the index,
+    // otherwise there's no chance of this working so we may as well error out.
+    // We print a warning on read failure so that users of linkers that rely on
+    // the symbol index can diagnose the issue.
+    //
+    // This is the same behavior as GNU ar when the linker plugin returns an
+    // error when reading the input file. If the bitcode file is actually
+    // malformed, it will be diagnosed at link time.
+    if (!ObjOrErr) {
+      switch (Kind) {
+      case object::Archive::K_BSD:
+      case object::Archive::K_GNU:
+      case object::Archive::K_GNU64:
+        llvm::logAllUnhandledErrors(ObjOrErr.takeError(), llvm::errs(),
----------------
dwblaikie wrote:

Callback based error handling with context seems plausible - by wrapping when context is added. Eg, if some previous codepath only produced errors, it might look like this:
```
Expected<std::unique_ptr<SymbolicFile>> SymFileOrErr =
    getSymbolicFile(M.Buf->getMemBufferRef(), Context);
if (!SymFileOrErr)
  return createFileError(M.MemberName, SymFileOrErr.takeError());
```
The buggy/missing context version would be to just pass in the warning callback here (from the client code):
```
Expected<std::unique_ptr<SymbolicFile>> SymFileOrErr =
    getSymbolicFile(M.Buf->getMemBufferRef(), Context, WarnHandler);
```
but then that could be improved to provide context with a wrapper, first by having the handler take an Error, not a string, then that would allow the call site to wrap for additional context:
```
Expected<std::unique_ptr<SymbolicFile>> SymFileOrErr =
    getSymbolicFile(M.Buf->getMemBufferRef(), Context, [&](Error E) {
    return WarnHandler(createFileError(M.MemberName, std::move(E)));
});
```
Which could be simplified for reuse with a utility:
```
Expected<std::unique_ptr<SymbolicFile>> SymFileOrErr =
    getSymbolicFile(M.Buf->getMemBufferRef(), Context, wrapWarningWithFile(WarnHandler));
```
Or similar.

Having clients have to trawl through text to find the "warning: " text embedded in the original text, or somehow have to add it inline in the stream after each newline, or something else seems pretty messy compared to this ^.

https://github.com/llvm/llvm-project/pull/96848