Page MenuHomePhabricator

[WIP][lld] Implement crash reproducer for ELF
Needs ReviewPublic

Authored by haowei on May 20 2021, 5:30 PM.

Details

Summary

This is still work in progress.

Compared to D102304, this implementation does not invoke lldMain twice. Instead, it tries to record the file names when lld touches any file and repackage them when lld recovered from CrashRecoveryContext. In test it generate the same reproducer tarball when compared to D102304.

The change still need more work. It currently require environment variable to make lld to invoke safeLldMain (ideally, lld should use safeLldMain when "--reproduce-on-crash") and the path of the crash reproducer can be customized. Let me know if this is the direction we want to go.

Diff Detail

Event Timeline

haowei created this revision.May 20 2021, 5:30 PM
haowei requested review of this revision.May 20 2021, 5:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2021, 5:30 PM
phosek added a comment.Jun 3 2021, 4:27 PM

I left some comments, but after reading the change, I'd suggest a slightly different structure: I think it'd be nice if we could avoid duplicating the logic between the --reproduce and --reproduce-on-crash case.

What I'm thinking is we could introduce a new function such as LinkerDriver::generateReproducer. This function would be responsible for generating the reproducer in both cases: we would always collect reproducer files and then call generateReproducer rather than writing the reproducer on demand as is the case now. That also has the advantage of not needing the std::unique_ptr<llvm::TarWriter> tar global since the TarWriter can be only created inside generateReproducer as needed.

I think we should also consider moving the llvm::CrashRecoveryContext setup from safeLldMain into each driver, for example elf::link, that's going to lead to some duplication, but the recovery context setup is fairly minimal and we could avoid having to repeat the flavor during normal execution and recovery and lldMain could remain completely agnostic to whether each driver supports crash recovery or not, that detail is fully encapsulated inside the *::link function.

lld/ELF/Driver.cpp
503–504

Do this check before instantiating the option parser which is a potentially expensive operation.

507

I'd do the early return on error. It might be also useful to signal the error back to the caller to avoid printing the output in the case of an error.

lld/tools/lld/lld.cpp
184

We might consider moving this logic into the ELF implementation so we can use ELFOptTable for option parsing.

197

I think it'd be also useful to allow explicitly setting the directory where to write the reproducer akin to Clang's -fcrash-diagnostics-dir. You could still use llvm::sys::fs::createTemporaryFile to create the file and then just move it to the final location at the end which is what Clang does.

You could either introduce another flag, for example --reproduce-dir or allow --reproduce-on-crash to take an optional value which would be the directory, either option is fine with me.

202

written

202

I think this output should be only printed after elf::createCrashReproduceTar so you don't print an output in the case that function fails for some reason.