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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
