teemperor added a comment.
I have some comments (beside my usual documentation spam). But otherwise looks
good to me.
================
Comment at: lldb/bindings/interface/SBProcess.i:196
+ SBStructuredData
+ GetCrashInfo ();
+
----------------
documentation.
================
Comment at: lldb/include/lldb/Target/Process.h:1333
+ lldb_private::StructuredData::ArraySP FetchCrashInfo();
+
----------------
Documentation pls
================
Comment at: lldb/include/lldb/Target/Process.h:2637
+ typedef struct {
+ uint64_t version; // unsigned long
----------------
Why make this a typedef'd anonymous struct?
Also maybe link to what struct this is mapping to (I assume this layout is
somewhere on disk). That would also explain the mysterious type comments behind.
================
Comment at: lldb/include/lldb/Target/Process.h:2648
+
+ typedef struct {
+ lldb::addr_t load_addr;
----------------
documentation :p
================
Comment at: lldb/include/lldb/Target/Process.h:2657
+
+ bool ExtractCrashInfoAnnotations(CrashInfoExtractor &extractor);
+
----------------
If this would return StructuredData then the structs above could be hidden in
the implementation. Just a suggestion.
(Also documentation)
================
Comment at:
lldb/packages/Python/lldbsuite/test/commands/process/crash-info/TestProcessCrashInfo.py:5
+
+from __future__ import print_function
+
----------------
You don't need that if you don't use print in your test.
================
Comment at:
lldb/packages/Python/lldbsuite/test/commands/process/crash-info/TestProcessCrashInfo.py:19
+ def setUp(self):
+ # Call super's setUp().
+ TestBase.setUp(self)
----------------
I think I removed that comment from most tests, so you can also remove it here
(it really doesn't tell me anything that isn't obvious).
================
Comment at:
lldb/packages/Python/lldbsuite/test/commands/process/crash-info/TestProcessCrashInfo.py:64
+
+ self.assertTrue("pointer being freed was not allocated" in
+ stream.GetData())
----------------
`self.assertIn` and you'll get a nice error message when this fails.
================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1482
+ : CommandObjectParsed(interpreter, "process crash-info",
+ "Fetch process' crash information from the
module.",
+ "process crash-info",
----------------
I think there should be a `the` before the `process'`
================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1490
+ bool DoExecute(Args &command, CommandReturnObject &result) override {
+ Stream &strm = result.GetOutputStream();
+ result.SetStatus(eReturnStatusSuccessFinishNoResult);
----------------
If you don't accept arguments you should check that there are no arguments or
otherwise throw an error. Currently `process crash-info banana` doesn't give an
error.
================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1506
+ if (!crash_info_sp)
+ result.AppendError("Couldn't fetch crash info.");
+
----------------
Shouldn't that have a return (otherwise this dereferences the pointer below).
================
Comment at: lldb/source/Target/Process.cpp:1133
+ LLDB_LOG(log, "{Couldn't extract crash info from Module {0}: {1}}",
+ module_name, extractor.error.AsCString());
+ continue;
----------------
You can omit `.AsCString()`. when doing LLDB_LOG
================
Comment at: lldb/source/Target/Process.cpp:1184
+ // Remove trailing newline from message
+ if (message[message.size() - 1] == '\n')
+ message.pop_back();
----------------
`.back()` ?
================
Comment at: lldb/source/Target/Process.cpp:1190
+
+ if (annotations.message2) {
+ std::string message2;
----------------
early exit/
================
Comment at: lldb/source/Target/Process.cpp:1197
+ extractor.error.Success()) {
+ if (message2[message2.size() - 1] == '\n')
+ message2.pop_back();
----------------
`.back()`
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
