[LLD][ELF] Add FORCE_LLD_DIAGNOSTICS_CRASH to force LLD to crash

[bd1976llvm]

Summary

Add FORCE_LLD_DIAGNOSTICS_CRASH inspired by the existing FORCE_CLANG_DIAGNOSTICS_CRASH.

This is particularly useful for people customizing LLD as they may want to modify the crash reporting behavior.

[emaste]

LGTM

[peter.smith]

IIUC this is primarily so that a test can be written for a crash message? Possibly worth adding an additional message "Crashing due to enviroment variable FORCE_LLD_DIAGNOSTICS_CRASH" as I can imagine someone setting the environment variable then forgetting and wondering why LLD keeps crashing. Alternatively would a --crash option be easier to manage? I don't think we can replicate the reproducer that the clang driver uses with this option.

[bd1976llvm]

Added useful reminder message as suggested by Peter.

[bd1976llvm]

In D128195#3596592, @peter.smith wrote:

Alternatively would a --crash option be easier to manage? I don't think we can replicate the reproducer that the clang driver uses with this option.

I'm not opposed. I have a slight preference for an env variable as it allows you to simulate crashes on systems where it is difficult to control command line switches.

[tschuett]

Entry in ReleaseNotes or man page?

[bd1976llvm]

In D128195#3596905, @tschuett wrote:

Entry in ReleaseNotes or man page?

I'll add a ReleaseNote but I don't think we want to document this otherwise as it is really a developer tool not a feature we want anyone to rely on. Thanks.

[bd1976llvm]

Add ReleaseNote.

[jhenderson]

lld/ELF/Driver.cpp

515

Two nits:

1. not clang-formatted
2. This probably should come under the same formatting rules as errors and warning messages (see https://llvm.org/docs/CodingStandards.html#error-and-warning-messages) so should start with a lower-case letter.

lld/test/ELF/crash-report.test

2

Personal preference: I don't know about anybody else, but I'm much more used to # and ## being used in lit tests where they can be.

4

[bd1976llvm]

Addressed style comments.

[bd1976llvm]

Fix typo.

[bd1976llvm]

Remove accidentally committed comments.

[jhenderson]

LGTM, but you probably want to wait for @peter.smith and/or @MaskRay.

[peter.smith]

LGTM from me. Thanks for the update. Will be worth giving MaskRay some time to comment as well.

[MaskRay]

If this is intended for all ports, you may use lld/tools/lld/lld.cpp

lld/test/ELF/crash-report.test

8

Is this reliable on Windows?

Also, LLVM_ENABLE_BACKTRACES=off builds do not have this diagnostic.

[bd1976llvm]

Move to lld.cpp

[bd1976llvm]

lld/test/ELF/crash-report.test

8

It seems to be reliable on windows- we have been running this test for a while now downstream with no failures.

Also, LLVM_ENABLE_BACKTRACES=off builds do not have this diagnostic.

Great review comment. However, I'm not sure how to make this test pass with LLVM_ENABLE_BACKTRACES=off without adding too much complexity. Do you/anyone else have an idea? Thanks.

[MaskRay]

lld/docs/ReleaseNotes.rst

32

Since you have the differential link, add it.

lld/test/ELF/crash-report.test

8

We may need to change test/lit.cfg.py to UNSUPPORTED: the test if LLVM_ENABLE_BACKTRACES=off.

If it is too troublesome, don't check the detailed diagnostic.

[bd1976llvm]

Ensure added lit test passes for LLVM_ENABLE_BACKTRACES=off builds.
Added differential URL to release note.
Used regex so that test doesn't care about contents of the bug report URL.

[bd1976llvm]

lld/test/ELF/crash-report.test

8

It wasn't troublesome at all. Thanks for the suggestion.

[bd1976llvm]

• Removed the LLD_IN_TEST un-setting in the lit test as we have moved the code earlier now.
• Removed checking for the period/full-stop in the lit test to allow customizers to extend the sentence without adjusting this test.

lld/ELF/Driver.cpp

for (auto *arg : args.filtered(OPT_z))
     if (!isKnownZFlag(arg->getValue()))
       warn("unknown -z value: " + StringRef(arg->getValue()));
 }
 
 void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   ELFOptTable parser;
   opt::InputArgList args = parser.parse(argsArr.slice(1));
 
+  if (::getenv("FORCE_LLD_DIAGNOSTICS_CRASH")) {
+    message("crashing due to environment variable FORCE_LLD_DIAGNOSTICS_CRASH");
+    LLVM_BUILTIN_TRAP;
+  }
+
   // Interpret these flags early because error()/warn() depend on them.
   errorHandler().errorLimit = args::getInteger(args, OPT_error_limit, 20);
   errorHandler().fatalWarnings =
       args.hasFlag(OPT_fatal_warnings, OPT_no_fatal_warnings, false);
   checkZOptions(args);
 
   // Handle -help
   if (args.hasArg(OPT_help)) {

lld/docs/ReleaseNotes.rst

 =================================================
 
 ELF Improvements
 ----------------
 
 * ``-z pack-relative-relocs`` is now available to support ``DT_RELR`` for glibc 2.36+.
   (`D120701 <https://reviews.llvm.org/D120701>`_)
 * ``--no-fortran-common`` (pre 12.0.0 behavior) is now the default.
+* ``FORCE_LLD_DIAGNOSTICS_CRASH`` environment variable is now available to force LLD to crash. 
 
 Breaking changes
 ----------------
 
 * The GNU ld incompatible ``--no-define-common`` has been removed.
 * The obscure ``-dc``/``-dp`` options have been removed.
 * ``-d`` is now ignored.
 * If a prevailing COMDAT group defines STB_WEAK symbol, having a STB_GLOBAL symbol in a non-prevailing group is now rejected with a diagnostic.

lld/test/ELF/crash-report.test

+## Test the diagnostics produced when LLD crashes.
+
+## Unset LLD_IN_TEST as it engages a crash recovery context.
+# RUN: env -u LLD_IN_TEST FORCE_LLD_DIAGNOSTICS_CRASH=1 not --crash ld.lld -o /dev/null 2>&1 | FileCheck %s
+
+## Check the crash text has the correct structure.
+# CHECK: crashing due to environment variable FORCE_LLD_DIAGNOSTICS_CRASH
+# CHECK: PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
+# CHECK: Stack dump: