[PATCH] D102892: [WIP][lld] Implement crash reproducer for ELF

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

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.



================
Comment at: lld/ELF/Driver.cpp:503-504
+
+  if (!path)
+    return;
+  Expected<std::unique_ptr<TarWriter>> errOrWriter =
----------------
Do this check before instantiating the option parser which is a potentially expensive operation.


================
Comment at: lld/ELF/Driver.cpp:507
+      TarWriter::create(path, path::stem(path));
+  if (errOrWriter) {
+    tar = std::move(*errOrWriter);
----------------
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.


================
Comment at: lld/tools/lld/lld.cpp:184
+      bool hasReproduceOnCrash = false;
+      for (const char *arg : args) {
+        std::string arg_str(arg);
----------------
We might consider moving this logic into the ELF implementation so we can use `ELFOptTable` for option parsing.


================
Comment at: lld/tools/lld/lld.cpp:197
+        std::error_code EC =
+            llvm::sys::fs::createTemporaryFile("lld", "tar", reproducerPath);
+        if (EC) {
----------------
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.


================
Comment at: lld/tools/lld/lld.cpp:202
+        }
+        stdoutOS << "reproducer writes to \"" << reproducerPath << "\"\n";
+        elf::createCrashReproduceTar(std::string(reproducerPath).data(), args);
----------------
written


================
Comment at: lld/tools/lld/lld.cpp:202
+        }
+        stdoutOS << "reproducer writes to \"" << reproducerPath << "\"\n";
+        elf::createCrashReproduceTar(std::string(reproducerPath).data(), args);
----------------
phosek wrote:
> written
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102892/new/

https://reviews.llvm.org/D102892