This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by bd1976llvm on Jun 20 2022, 5:58 AM.

Details

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.

Diff Detail

Event Timeline

bd1976llvm created this revision.Jun 20 2022, 5:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 5:58 AM
bd1976llvm requested review of this revision.Jun 20 2022, 5:58 AM

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.

Added useful reminder message as suggested by Peter.

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.

Entry in ReleaseNotes or man page?

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.

Add ReleaseNote.

jhenderson added inline comments.Jun 21 2022, 12:29 AM
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

Addressed style comments.

bd1976llvm marked 3 inline comments as done.

Fix typo.

Remove accidentally committed comments.

jhenderson accepted this revision.Jun 21 2022, 5:53 AM

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

This revision is now accepted and ready to land.Jun 21 2022, 5:53 AM
peter.smith accepted this revision.Jun 21 2022, 6:02 AM

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

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.

Move to lld.cpp

bd1976llvm added inline comments.Jun 22 2022, 4:22 PM
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 accepted this revision.Jun 22 2022, 4:28 PM
MaskRay added inline comments.
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.

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 marked 2 inline comments as done.Jun 23 2022, 6:41 AM
bd1976llvm added inline comments.
lld/test/ELF/crash-report.test
8

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

  • 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.
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 1:43 AM