[Lldb-commits] [PATCH] D74657: [lldb/Plugins] Add ability to fetch crash information on crashed processes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D74657#1885361 <https://reviews.llvm.org/D74657#1885361>, @JDevlieghere wrote:

> A few small nits inline, but this LGTM if Pavel is on board.


We're getting close, but I still see some room for improvement. :)



================
Comment at: lldb/bindings/interface/SBPlatform.i:195-198
+    %feature("autodoc", "
+    Returns the platform's process extended crash information.") GetExtendedCrashInformation;
+    lldb::SBStructuredData
+    GetExtendedCrashInformation (lldb::SBTarget& target);
----------------
If we keep the this way, then the user will have to remember to get the correct platform in order to ask it for the crash info. That's not the end of the world, but it might be better if this was a method on the SBTarget class, which would automatically consult the target's current platform.


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1288-1291
+      if (!crash_info_sp) {
+        result.AppendError("Couldn't fetch the crash information");
+        result.SetStatus(eReturnStatusFailed);
+        return result.Succeeded();
----------------
So, this will print an error if we don't get a crash information for _any_ reason (process did not crash, platform does not support crash info, ...), is that right?

That seems overly aggressive. I think this should be more nuanced. Maybe if `FetchExtendedCrashInformation` returned `Expected<StructuredDataSP>`, then you could do something like
- if the result is an error => something really went wrong => print the error message
- nullptr => not an error, but we also didn't get anything => don't print anything
- an actual dictionary => hurray => print the crash info

Then the default implementation can return `nullptr`, and in `PlatformDarwin` you can precisely decide what treatment does a certain situation deserve (e.g. whether not being able to find the __crash_info section is an error or just "nothing").


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1294
+
+      crash_info_sp->Dump(strm);
+    }
----------------
Should you maybe print some kind of a header to make it clear that what follows is the crash information?


================
Comment at: lldb/test/API/functionalities/process_crash_info/TestProcessCrashInfo.py:37
+        self.expect('process status --verbose',
+                    patterns=["\"message\".*pointer being freed was not allocated"])
+
----------------
Besides the "positive" test, I think it would be also good to have some tests for the "negative" cases. E.g. a case where the process did not crash, or when we have not even launched it yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74657