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

[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(),
----------------
pcc wrote:

I had the same idea about wrapping the lambda but I'm not a fan of the amount of boilerplate needed when calling a function that might warn.
```
return createFileName(FileName, foo([&](Error Err) { Warn(createFileName(FileName, Err)); });
```
And you still need a post-facto fixup in the caller -- except now it's "was a callback called" and not "is a buffer empty". So I think this makes the code worse than `raw_ostream &Warn` because it's more subtle, more verbose in the callee and probably the caller, less intuitive, and shoehorns bad APIs into places where they don't fit, making it harder to remove them. But I guess it's not that much worse, it only happens in one place for now so I guess I'm not entirely opposed to doing that.

Side note, here is the usage code for an API that I would consider replacing `Error` with:
```
void foo(StringRef FileName, DiagHandler &Diag) {
  auto FDiag = Diag.withFileName(FileName);
  bar(FDiag);
  if (FDiag.hasErrors())
    return;
  baz(FDiag);
}

void bar(DiagHandler &Diag) {
   Diag.warn("some warning"); // could be promoted to error by Diag implementation
   Diag.error("some error");
}
```

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