[Lldb-commits] [PATCH] D70474: [Reproducer] Generate LLDB reproducer on crash

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think this looks pretty good. This is going to do a _lot_ of work in the signal handler, which may or may not work. However, at the point when this code gets invoked, we're already in undefined behavior territory anyway, so we might as well give it a shot. The dumping code can be gradually hardened later if we find that a lot of reproducers are failing to complete the dumping process.

I was expecting that the raise(SIGWHATEVER) stuff will not work on windows, but it looks like it should at least compile <https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/raise?view=vs-2019>. I have no idea what will that do at runtime, but reproducers don't work on windows anyway.



================
Comment at: lldb/source/Commands/CommandObjectReproducer.cpp:86
+        eReproducerCrashSigbus,
+        "sigbus",
+        "Bus error",
----------------
everywhere else we use capital letters for signal names


================
Comment at: lldb/source/Commands/CommandObjectReproducer.cpp:99-103
+    {
+        eReproducerCrashNone,
+        "none",
+        "None",
+    },
----------------
What's the reason for the "none" value? Can we remove it? I am assuming noone would ever want to write "reproducer (x)crash -s none"...


================
Comment at: lldb/source/Commands/CommandObjectReproducer.cpp:159
 
+class CommandObjectReproducerCrash : public CommandObjectParsed {
+public:
----------------
Maybe also call this xcrash?


================
Comment at: lldb/source/Commands/Options.td:441
 
+let Command = "reproducer crash" in {
+  def reproducer_signal : Option<"signal", "s">, Group<1>,
----------------
xcrash?


================
Comment at: lldb/tools/driver/Driver.cpp:739-744
+    llvm::outs() << lldb::SBDebugger::GetVersionString() << '\n';
+    llvm::outs() << "Reproducer written to '" << SBReproducer::GetPath()
+                 << "'\n";
+    llvm::outs()
+        << "Please have a look at the directory to assess if you're willing to "
+           "share the contained information.\n";
----------------
I'm thinking if there's a way to centralize the printing of this message. Right now it is duplicated here and in the "reproducer generate" implementation, and it's already accumulating subtle differences (e.g., this copy includes the version string while the other one does not -- it seems like this would be useful everywhere). Maybe if the Generate call takes an SBFile object -- the stream where this message gets written to, and then we have some lldb_private function writing that message?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D70474