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

2020-02-15 Thread 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



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74217: Add target.xml support for qXfer request.

2020-02-15 Thread Levon Ter-Grigoryan via Phabricator via lldb-commits 
PatriosTheGreat added a comment.

Hi Pavel,
Thank you for review.
Could you also commit this to master, since I don't have commit access?


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

https://reviews.llvm.org/D74217



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74670: Simplify user_id_t -> size_t

2020-02-15 Thread Jan Kratochvil via Phabricator via lldb-commits 
jankratochvil created this revision.
jankratochvil added reviewers: labath, clayborg.
jankratochvil added a project: LLDB.
Herald added a subscriber: aprantl.
Herald added a reviewer: jdoerfert.

As discussed  in D73206 
 simplifying usage of `user_id_t`.
There is even written:

  // The compile unit ID is the index of the DWARF unit.
  DWARFUnit *dwarf_cu = info->GetUnitAtIndex(comp_unit->GetID());


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74670

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
@@ -35,7 +35,7 @@
   lldb_private::DWARFContext &context);
 
   size_t GetNumUnits();
-  DWARFUnit *GetUnitAtIndex(lldb::user_id_t idx);
+  DWARFUnit *GetUnitAtIndex(size_t idx);
   DWARFUnit *GetUnitAtOffset(DIERef::Section section, dw_offset_t cu_offset,
  uint32_t *idx_ptr = nullptr);
   DWARFUnit *GetUnitContainingDIEOffset(DIERef::Section section,
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -109,7 +109,7 @@
   return m_units.size();
 }
 
-DWARFUnit *DWARFDebugInfo::GetUnitAtIndex(user_id_t idx) {
+DWARFUnit *DWARFDebugInfo::GetUnitAtIndex(size_t idx) {
   DWARFUnit *cu = nullptr;
   if (idx < GetNumUnits())
 cu = m_units[idx].get();


Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
@@ -35,7 +35,7 @@
   lldb_private::DWARFContext &context);
 
   size_t GetNumUnits();
-  DWARFUnit *GetUnitAtIndex(lldb::user_id_t idx);
+  DWARFUnit *GetUnitAtIndex(size_t idx);
   DWARFUnit *GetUnitAtOffset(DIERef::Section section, dw_offset_t cu_offset,
  uint32_t *idx_ptr = nullptr);
   DWARFUnit *GetUnitContainingDIEOffset(DIERef::Section section,
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -109,7 +109,7 @@
   return m_units.size();
 }
 
-DWARFUnit *DWARFDebugInfo::GetUnitAtIndex(user_id_t idx) {
+DWARFUnit *DWARFDebugInfo::GetUnitAtIndex(size_t idx) {
   DWARFUnit *cu = nullptr;
   if (idx < GetNumUnits())
 cu = m_units[idx].get();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits