[ThinLTO] return error instead of crashing on invalid input

[Amir]

Summary

In https://bugs.llvm.org/show_bug.cgi?id=45636 firefox was linked
against stale object files that don't match profiled build.

Before the change ld.lld was crashing with a backtrace as:

LLVM ERROR: Function Import: link error:
  linking module flags 'ProfileSummary':
    IDs have conflicting values in 'Mutex_posix.o' and 'nsBrowserApp.o'
  PLEASE submit a bug report to https://bugs.llvm.org/ and include the
  crash backtrace.

 #0 0x000055b58915b86a llvm::sys::PrintStackTrace(llvm::raw_ostream&)
   llvm/lib/Support/Unix/Signals.inc:564:22
 ...
 #11 0x000055b589111f89 llvm::report_fatal_error(llvm::Twine const&, bool)
   llvm/lib/Support/ErrorHandling.cpp:113:27
 #12 0x000055b5891120e1 llvm/lib/Support/ErrorHandling.cpp:87:21
 #13 0x000055b58a22611f llvm::FunctionImporter::importFunctions(...)
   llvm/lib/Transforms/IPO/FunctionImport.cpp:1250:25

After the change ld.lld collects all errors during ThinLTO and reports
them as usual:

ld.lld: error: Function Import: link error:
  linking module flags 'ProfileSummary':
    IDs have conflicting values in 'Mutex_posix.o' and 'nsBrowserApp.o'

[dblaikie]

Could you add a test case for this?

(@tejohnson - might be the suitable reviewer here)

[jdoerfert]

Having a backtrace and the bug filing message seems excessive. Maybe you can even mention stale obj files in the message.
I'll add some reviewers that might want to chime in, otherwise I'll revisit this.

[MaskRay]

ping:)

[tejohnson]

Please address the earlier suggestions (needs a test case, and also agree it might be nice to have the message suggest stale objects as possible culprit).

[trofi]

In D79140#2391922, @tejohnson wrote:

Please address the earlier suggestions (needs a test case

The failure looks like a low-level profile ID mismatch when merging profiles obtained when running tests against slightly different sources. I'll need some help on how I can craft the equivalent using lit mechanism. Original 40MB compressed input to lld is at https://bugs.llvm.org/show_bug.cgi?id=45636#c1 if it's of any help.

and also agree it might be nice to have the message suggest stale objects as possible culprit).

DR description provides new error message as:

"""
ld.lld: error: Function Import: link error:

linking module flags 'ProfileSummary':
  IDs have conflicting values in 'Mutex_posix.o' and 'nsBrowserApp.o'

"""

can you suggest how it should be amended instead?

[tejohnson]

In D79140#2392732, @trofi wrote:

In D79140#2391922, @tejohnson wrote:

Please address the earlier suggestions (needs a test case

The failure looks like a low-level profile ID mismatch when merging profiles obtained when running tests against slightly different sources. I'll need some help on how I can craft the equivalent using lit mechanism. Original 40MB compressed input to lld is at https://bugs.llvm.org/show_bug.cgi?id=45636#c1 if it's of any help.

There is already a test for this particular error message in llvm/test/Linker/module-flags-6-a.ll. It uses llvm-link. Presumably you can use the same input files but invoke via lld to test?

and also agree it might be nice to have the message suggest stale objects as possible culprit).

DR description provides new error message as:

"""
ld.lld: error: Function Import: link error:

linking module flags 'ProfileSummary':
  IDs have conflicting values in 'Mutex_posix.o' and 'nsBrowserApp.o'

"""

can you suggest how it should be amended instead?

The error message that should be changed ("IDs have conflicting values") is coming from in llvm/lib/Linker/IRMover.cpp. Maybe change it to something like "IDs have conflicting values in 'Mutex_posix.o' and 'nsBrowserApp.o' (possible use of stale object files)"? Outside of development it seems as though this would only happen in the stale object case.

[trofi]

I'll unlikely get to it any time soon. My environment changed quite a bit to be able to trigger the failure.

[Amir]

Add testcase

[Amir]

I'm currently running into this issue on a buildbot, likely due to stale ccache files: https://lab.llvm.org/buildbot/#/builders/246/builds/1664

[Amir]

Rebase

[tejohnson]

Thanks for picking this up. Question on the test.

lld/test/ELF/stale-profile-error.ll

8

Does this test fail without the change in this patch? It seems the main difference is that it dies more nicely with an error rather than a crash with backtrace, so it looks like this test would also pass without this change. Is there a way to make it fail with the crash?

[MaskRay]

lld/test/ELF/stale-profile-error.ll

1

Move this to lto/ and add REQUIRES: x86

7

Use ;; for non-RUN non-CHECK comments in lld/test/.

[MaskRay]

lld/test/ELF/stale-profile-error.ll

1

Ideally use a llvm/test/ThinLTO or llvm/test/LTO. This test isn't so related to lld https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-at-the-wrong-layer

[Amir]

Fixed in f53de29862193b761f3ef8ec0627d1e70d2fe674

lld/test/ELF/stale-profile-error.ll

+; RUN: split-file %s %t
+; RUN: llvm-as %t/size1.ll -o %t/size1.bc
+; RUN: llvm-as %t/size2.ll -o %t/size2.bc
+
+; RUN: not ld.lld %t/size1.bc %t/size2.bc -S -o - 2>&1 | FileCheck %s
+
+; Test module flags error messages.
+; CHECK: linking module flags 'foo': IDs have conflicting values in '{{.*}}size2.bc' and 'ld-temp.o'
+
+;--- size1.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+!0 = !{ i32 1, !"foo", i32 37 }
+
+!llvm.module.flags = !{ !0 }
+
+;--- size2.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+!0 = !{ i32 1, !"foo", i32 38 }
+
+!llvm.module.flags = !{ !0 }

llvm/lib/Transforms/IPO/FunctionImport.cpp

if (PrintImports) {
       for (const auto *GV : GlobalsToImport)
         dbgs() << DestModule.getSourceFileName() << ": Import " << GV->getName()
                << " from " << SrcModule->getSourceFileName() << "\n";
     }
 
     if (Error Err = Mover.move(std::move(SrcModule),
                                GlobalsToImport.getArrayRef(), nullptr,
                                /*IsPerformingImport=*/true))
-      report_fatal_error(Twine("Function Import: link error: ") +
-                         toString(std::move(Err)));
+      return createStringError(errc::invalid_argument,
+                               "Function Import: link error: %s",
+                               toString(std::move(Err)).c_str());
 
     ImportedCount += GlobalsToImport.size();
     NumImportedModules++;
   }
 
   internalizeGVsAfterImport(DestModule);
 
   NumImportedFunctions += (ImportedCount - ImportedGVCount);