[Lldb-commits] [PATCH] D74657: [lldb/Plugins] Add `platform process crash-info` command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Target/Platform.h:838
+  /// \return
+  ///     A Structured Data Dictionnary containing at each entry an array for
+  ///     each different crash information type. Or a nullptr if the process has
----------------
I had a hard time understanding this sentence. Is the `crash information type` the key of the entry or the values in the array? 


================
Comment at: lldb/include/lldb/Target/Platform.h:881
   const std::unique_ptr<ModuleCache> m_module_cache;
+  StructuredData::DictionarySP m_extended_crash_info;
 
----------------
I find it suspicious that this member is stored here but not used. Does it need to be stored in the first place? Given the FetchExtendedCrashInformation I'd expect these dictionaries to be different per target? 


================
Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1520
 
+// CommandObjectPlatformProcessCrashInfo
+#pragma mark CommandObjectPlatformProcessCrashInfo
----------------
This comment is useless. 


================
Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1521
+// CommandObjectPlatformProcessCrashInfo
+#pragma mark CommandObjectPlatformProcessCrashInfo
+
----------------
As I've said in other reviews I really dislike the `#pragma mark`. It's fine if the file already uses them but I really don't want to encourage new ones being added. 


================
Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1541
+    if (!platform_sp) {
+      result.AppendError("Couldn'retrieve the selected platform");
+      return result.Succeeded();
----------------
Shouldn't you update the return status? Same below.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:1514
+    LLDB_LOG(log, "Couldn't extract crash information annotations");
+  }
+
----------------
The braces are inconsistent with the single-line ifs below. Why not wrap this log statement in the if check below? 
```
if (!annotations)
    LLDB_LOG(log, "Couldn't extract crash information annotations")
else
    m_extended_crash_info->AddItem("crash-info annotations", annotations);
```
If you want the log statement to stay close to the `ExtractCrashInfoAnnotations` call (which I think you should), you could move the `!m_extended_crash_info` check up.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:1596
+
+    if (!annotations.message2) {
+      LLDB_LOG(log, "No message2 available for module {0}.", module_name);
----------------
braces


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h:93
+  // This struct should only be available in macOS but could be used on other
+  // platforms. #ifdef HAVE_CRASHREPORTERCLIENT_H #include
+  // <CrashReporterClient.h> #endif
----------------
Assuming the header defines this struct, why not do 

```
#ifdef HAVE_CRASHREPORTERCLIENT_H 
#include <CrashReporterClient.h> 
#else 
struct CrashInfoAnnotations {
  ...
}
#endif 
```


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h:124
+  ExtractCrashInfoAnnotations(lldb_private::Target &target,
+                              lldb_private::Log *log);
+
----------------
The log is a singleton, why pass it around? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74657