[Lldb-commits] [PATCH] D74657: [lldb/Target] Add process crash-info command

[Raphael Isemann via Phabricator via lldb-commits]

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