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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to