[Lldb-commits] [PATCH] D88229: Reland "[lldb] Don't send invalid region addresses to lldb server"

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added a reviewer: JDevlieghere.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
DavidSpickett requested review of this revision.

This reverts commit c65627a1fe3be7521fc232d633bb6df577f55269 
.

The test immediately after the new invalid symbol test was
failing on Windows. This was because when we called
VirtualQueryEx to get the region info for 0x0,
even if it succeeded we would call GetLastError.

Which must have picked up the last error that was set while
trying to lookup "not_an_address". Which happened to be 2.
("The system cannot find the file specified.")

To fix this only call GetLastError when we know VirtualQueryEx
has failed. (when it returns 0, which we were also checking for anyway)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88229

Files:
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
  lldb/test/API/functionalities/memory-region/TestMemoryRegion.py


Index: lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
===
--- lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -41,6 +41,12 @@
 self.assertFalse(result.Succeeded())
 self.assertRegexpMatches(result.GetError(), "Usage: memory region 
ADDR")
 
+# Test that when the address fails to parse, we show an error and do 
not continue
+interp.HandleCommand("memory region not_an_address", result)
+self.assertFalse(result.Succeeded())
+self.assertEqual(result.GetError(),
+"error: invalid address argument \"not_an_address\": address 
expression \"not_an_address\" evaluation failed\n")
+
 # Now let's print the memory region starting at 0 which should always 
work.
 interp.HandleCommand("memory region 0x0", result)
 self.assertTrue(result.Succeeded())
Index: lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
===
--- lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
+++ lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
@@ -460,7 +460,6 @@
 info.SetMapped(MemoryRegionInfo::eNo);
   }
 
-  error.SetError(::GetLastError(), eErrorTypeWin32);
   LLDB_LOGV(log,
 "Memory region info for address {0}: readable={1}, "
 "executable={2}, writable={3}",
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -1707,6 +1707,7 @@
 "invalid address argument \"%s\": %s\n", command[0].c_str(),
 error.AsCString());
 result.SetStatus(eReturnStatusFailed);
+return false;
   }
 }
 


Index: lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
===
--- lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -41,6 +41,12 @@
 self.assertFalse(result.Succeeded())
 self.assertRegexpMatches(result.GetError(), "Usage: memory region ADDR")
 
+# Test that when the address fails to parse, we show an error and do not continue
+interp.HandleCommand("memory region not_an_address", result)
+self.assertFalse(result.Succeeded())
+self.assertEqual(result.GetError(),
+"error: invalid address argument \"not_an_address\": address expression \"not_an_address\" evaluation failed\n")
+
 # Now let's print the memory region starting at 0 which should always work.
 interp.HandleCommand("memory region 0x0", result)
 self.assertTrue(result.Succeeded())
Index: lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
===
--- lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
+++ lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
@@ -460,7 +460,6 @@
 info.SetMapped(MemoryRegionInfo::eNo);
   }
 
-  error.SetError(::GetLastError(), eErrorTypeWin32);
   LLDB_LOGV(log,
 "Memory region info for address {0}: readable={1}, "
 "executable={2}, writable={3}",
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -1707,6 +1707,7 @@
 "invalid address argument \"%s\": %s\n", command[0].c_str(),
 error.AsCString());
 result.SetStatus(

[Lldb-commits] [PATCH] D88229: Reland "[lldb] Don't send invalid region addresses to lldb server"

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: labath.
DavidSpickett added a comment.

This is probably pretty trivial but my first dive into win32 stuff so wanted to 
double check my logic before relanding.

References:
https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualqueryex
https://docs.microsoft.com/en-us/windows/win32/api/errhandlingapi/nf-errhandlingapi-getlasterror


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88229

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

[Lldb-commits] [PATCH] D88158: [lldb/examples] Add missing declaration in heap.py

[Dave Lee via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGada1e2ffa117: [lldb/examples] Add missing declaration in 
heap.py (authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88158

Files:
  lldb/examples/darwin/heap_find/heap.py


Index: lldb/examples/darwin/heap_find/heap.py
===
--- lldb/examples/darwin/heap_find/heap.py
+++ lldb/examples/darwin/heap_find/heap.py
@@ -129,6 +129,7 @@
 void *reserved1[12];
 struct malloc_introspection_t  *introspect;
 } malloc_zone_t;
+kern_return_t malloc_get_all_zones(task_t task, memory_reader_t reader, 
vm_address_t **addresses, unsigned *count);
 memory_reader_t task_peek = [](task_t task, vm_address_t remote_address, 
vm_size_t size, void **local_memory) -> kern_return_t {
 *local_memory = (void*) remote_address;
 return KERN_SUCCESS;


Index: lldb/examples/darwin/heap_find/heap.py
===
--- lldb/examples/darwin/heap_find/heap.py
+++ lldb/examples/darwin/heap_find/heap.py
@@ -129,6 +129,7 @@
 void *reserved1[12];
 struct malloc_introspection_t	*introspect;
 } malloc_zone_t;
+kern_return_t malloc_get_all_zones(task_t task, memory_reader_t reader, vm_address_t **addresses, unsigned *count);
 memory_reader_t task_peek = [](task_t task, vm_address_t remote_address, vm_size_t size, void **local_memory) -> kern_return_t {
 *local_memory = (void*) remote_address;
 return KERN_SUCCESS;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] ada1e2f - [lldb/examples] Add missing declaration in heap.py

[Dave Lee via lldb-commits]

Author: Dave Lee
Date: 2020-09-24T08:44:45-07:00
New Revision: ada1e2ffa1172ede1790b4b42ef8ab01508d3a47

URL: 
https://github.com/llvm/llvm-project/commit/ada1e2ffa1172ede1790b4b42ef8ab01508d3a47
DIFF: 
https://github.com/llvm/llvm-project/commit/ada1e2ffa1172ede1790b4b42ef8ab01508d3a47.diff

LOG: [lldb/examples] Add missing declaration in heap.py

Add missing declaration for `malloc_get_all_zones` in heap.py.

Differential Revision: https://reviews.llvm.org/D88158

Added: 


Modified: 
lldb/examples/darwin/heap_find/heap.py

Removed: 




diff  --git a/lldb/examples/darwin/heap_find/heap.py 
b/lldb/examples/darwin/heap_find/heap.py
index 8fb2a8c95927..4174d396feb6 100644
--- a/lldb/examples/darwin/heap_find/heap.py
+++ b/lldb/examples/darwin/heap_find/heap.py
@@ -129,6 +129,7 @@ def get_iterate_memory_expr(
 void *reserved1[12];
 struct malloc_introspection_t  *introspect;
 } malloc_zone_t;
+kern_return_t malloc_get_all_zones(task_t task, memory_reader_t reader, 
vm_address_t **addresses, unsigned *count);
 memory_reader_t task_peek = [](task_t task, vm_address_t remote_address, 
vm_size_t size, void **local_memory) -> kern_return_t {
 *local_memory = (void*) remote_address;
 return KERN_SUCCESS;



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

[Lldb-commits] [PATCH] D88229: Reland "[lldb] Don't send invalid region addresses to lldb server"

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added inline comments.



Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1701
   } else {
 if (command.GetArgumentCount() == 1) {
   auto load_addr_str = command[0].ref();

Actually, I would change the logic here a little bit to make it easier to read. 
Right now it is:

```
if (argc > 1 || ... ) {
} else {
  if (GetArgumentCount() == 1) {
  }
  ...
}
```

It should be:

```
if (argc > 1 || ... ) {
} else if (argc == 1) { //since argc already has the value of GetArgumentCount()
}

if (result.Succeeded()) {
 ...
}
```

This will make the function more readable, fixing the bug that you found, 
preserving most of its logic and keeping the single return.




Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp:420
 } else {
   error.SetError(::GetLastError(), eErrorTypeWin32);
   LLDB_LOG(log,

This GetLastError call is not guaranteed to return the same error as above 
since the error could be cleared by a call to GetLastError. You should instead 
do:

```
if (result == 0) {
   DWORD lastError = ::GetLastError();
   if (lastError == ERROR_INVALID_PARAMETER) {
   }
   else {
   }
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88229

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

[Lldb-commits] [PATCH] D88247: Fix memory leak in SBValue::GetAddress

[Andy Yankovsky via Phabricator via lldb-commits]

werat created this revision.
werat added a reviewer: labath.
werat added a project: LLDB.
Herald added subscribers: lldb-commits, JDevlieghere.
Herald added a reviewer: JDevlieghere.
werat requested review of this revision.

SBAddress constructor accepts a pointer to `lldb_private::Address`, but then 
dereferences it and copies the value. Allocating a temporary here leads to a 
memory leak.

Found via ASan:

Direct leak of 24 byte(s) in 1 object(s) allocated from:

  #0 0x7fc70451e19f in operator new(unsigned long) 
(/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10919f)
  #1 0x7fc6fdb560b4 in lldb::SBValue::GetAddress() 
/home/werat/git/llvm-project-upstream/lldb/source/API/SBValue.cpp:1359
  #2 0x7fc6fdb06714 in lldb::SBType::operator=(lldb::SBType const&) 
/home/werat/git/llvm-project-upstream/lldb/source/API/SBType.cpp:83
  ...


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88247

Files:
  lldb/source/API/SBValue.cpp


Index: lldb/source/API/SBValue.cpp
===
--- lldb/source/API/SBValue.cpp
+++ lldb/source/API/SBValue.cpp
@@ -1356,7 +1356,7 @@
 }
   }
 
-  return LLDB_RECORD_RESULT(SBAddress(new Address(addr)));
+  return LLDB_RECORD_RESULT(SBAddress(&addr));
 }
 
 lldb::SBData SBValue::GetPointeeData(uint32_t item_idx, uint32_t item_count) {


Index: lldb/source/API/SBValue.cpp
===
--- lldb/source/API/SBValue.cpp
+++ lldb/source/API/SBValue.cpp
@@ -1356,7 +1356,7 @@
 }
   }
 
-  return LLDB_RECORD_RESULT(SBAddress(new Address(addr)));
+  return LLDB_RECORD_RESULT(SBAddress(&addr));
 }
 
 lldb::SBData SBValue::GetPointeeData(uint32_t item_idx, uint32_t item_count) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88129: Add "break delete --disabled" to delete all currently disabled breakpoints

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/source/Commands/Options.td:232
+Desc<"Delete all breakpoints which are currently disabled.  When using the 
disabled option "
+"any breakpoints listed on the command line are EXCLUDED from deletion.">;
 }

jingham wrote:
> kastiglione wrote:
> > jingham wrote:
> > > kastiglione wrote:
> > > > jingham wrote:
> > > > > kastiglione wrote:
> > > > > > jingham wrote:
> > > > > > > kastiglione wrote:
> > > > > > > > To me, it's counter intuitive that `break delete --disabled 1` 
> > > > > > > > will not delete bp 1.
> > > > > > > The combination:
> > > > > > > 
> > > > > > > (lldb) break delete --disabled 1
> > > > > > > 
> > > > > > > could either mean 
> > > > > > > 
> > > > > > > 1) delete all breakpoints that are disabled AND breakpoint 1
> > > > > > > 2) delete all breakpoints that are disabled EXCEPT breakpoint 1
> > > > > > > 3) an error
> > > > > > > 
> > > > > > > Of those interpretations, 1 and 3 don't seem very useful, but 2 
> > > > > > > does.  
> > > > > > > 
> > > > > > > This is particularly handy when you specify a breakpoint name, 
> > > > > > > not a breakpoint.  Just make breakpoints you don't want deleted 
> > > > > > > DoNotDelete, then you can easily protect all those breakpoints.  
> > > > > > > 
> > > > > > > Note, your workaround would only be useful in this case if all 
> > > > > > > the breakpoints named DoNotDelete are currently disabled.  
> > > > > > > Otherwise you would have to remember which of the DoNotDelete 
> > > > > > > breakpoints were disabled, enable them all, do the `delete 
> > > > > > > --disabled` then  only re-disable those that were originally 
> > > > > > > disabled.  Whereas if you can pass an exclude list you can just 
> > > > > > > protect those breakpoints unconditionally regardless of their 
> > > > > > > state.
> > > > > > > 
> > > > > > > So while I agree this is a little odd, it's actually the only 
> > > > > > > option that really makes sense, it's easy to document, and I 
> > > > > > > don't think it's likely to cause mistakes. 
> > > > > > why does the first interpretation not seem useful? If I'm deleting 
> > > > > > breakpoints, I might want to delete both disabled breakpoints and 
> > > > > > one or more specific breakpoints. To do that I would probably 
> > > > > > intuitively write `break delete --disabled OthersToDelete`.
> > > > > > 
> > > > > > Could the ambiguity be removed by adding another flag? `break 
> > > > > > delete --disabled --except DoNotDelete`?
> > > > > To me "delete --disabled" is a bulk operation acting on a class of 
> > > > > breakpoints.  "This class plus one random other one" seems odd to me. 
> > > > >  
> > > > > 
> > > > > A bulk operation with exclusions makes much more sense to me.  
> > > > > 
> > > > > Adding another option complicates things without adding much value, 
> > > > > and becomes annoying if you want to specify more than one excluded 
> > > > > thing.  It would be easy to make the mistake:
> > > > > 
> > > > > (lldb) break disable --disabled --except 1 2
> > > > > 
> > > > > intending to preserve 2 but in fact deleting it.
> > > > I get that exclusions are useful, my concern is that the command 
> > > > "breakpoint delete" doesn't delete what you give it. If `break delete 
> > > > foo` deletes foo, then on the surface `break delete --disabled foo` 
> > > > should also delete foo. The flag does what it says, but also silently 
> > > > inverts the meaning of the positional args.
> > > The help for the option explicitly says that it inverts the meaning of 
> > > the positional args, there's nothing silent about it.  You wouldn't 
> > > accidentally say `break delete --disabled`, so presumably you would have 
> > > to have read the help for the option, which I don't think is susceptible 
> > > to misconstruction.
> > > 
> > > Because of that, I'm not too bothered that `break delete --disabled Foo` 
> > > behaves differently from `break delete Foo`.  And it seems the simplest 
> > > way to express the most useful thing you would want to add to just`break 
> > > delete --disable`.
> > In my experience people learn about lldb through 
> > twitter/coworkers/blogs/talks/tutorials etc, and not through `help`. Of 
> > those who learn from help, they may not read every word. It's quite 
> > possible to use this flag without having read the fine print.
> Given that misusing this command+option would result in a breakpoint NOT 
> getting deleted, I'm less concerned about the possibility of misuse.  The 
> reaction is "I did a somewhat odd thing and the odd bit I added didn't work 
> as expected (in a non-destructive way) so maybe I should read the help".  
> That doesn't seem problematic to me.
👍 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88129

___
lldb-commits mailing list
lldb-commits@lists.

[Lldb-commits] [PATCH] D88249: [lldb] Make protected ctor and method of SBAddress take a const reference instead of a pointer.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: labath, werat.
JDevlieghere requested review of this revision.

Every call to the protected SBAddress constructor takes the address of a valid 
object which means we might as well pass it as a const reference instead of a 
pointer and drop the null check.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D88249

Files:
  lldb/include/lldb/API/SBAddress.h
  lldb/source/API/SBAddress.cpp
  lldb/source/API/SBBreakpointLocation.cpp
  lldb/source/API/SBFrame.cpp
  lldb/source/API/SBFunction.cpp
  lldb/source/API/SBInstruction.cpp
  lldb/source/API/SBLineEntry.cpp
  lldb/source/API/SBQueueItem.cpp
  lldb/source/API/SBSymbol.cpp
  lldb/source/API/SBValue.cpp

Index: lldb/source/API/SBValue.cpp
===
--- lldb/source/API/SBValue.cpp
+++ lldb/source/API/SBValue.cpp
@@ -1356,7 +1356,7 @@
 }
   }
 
-  return LLDB_RECORD_RESULT(SBAddress(new Address(addr)));
+  return LLDB_RECORD_RESULT(SBAddress(Address(addr)));
 }
 
 lldb::SBData SBValue::GetPointeeData(uint32_t item_idx, uint32_t item_count) {
Index: lldb/source/API/SBSymbol.cpp
===
--- lldb/source/API/SBSymbol.cpp
+++ lldb/source/API/SBSymbol.cpp
@@ -151,7 +151,7 @@
 
   SBAddress addr;
   if (m_opaque_ptr && m_opaque_ptr->ValueIsAddress()) {
-addr.SetAddress(&m_opaque_ptr->GetAddressRef());
+addr.SetAddress(m_opaque_ptr->GetAddressRef());
   }
   return LLDB_RECORD_RESULT(addr);
 }
@@ -163,7 +163,7 @@
   if (m_opaque_ptr && m_opaque_ptr->ValueIsAddress()) {
 lldb::addr_t range_size = m_opaque_ptr->GetByteSize();
 if (range_size > 0) {
-  addr.SetAddress(&m_opaque_ptr->GetAddressRef());
+  addr.SetAddress(m_opaque_ptr->GetAddressRef());
   addr->Slide(m_opaque_ptr->GetByteSize());
 }
   }
Index: lldb/source/API/SBQueueItem.cpp
===
--- lldb/source/API/SBQueueItem.cpp
+++ lldb/source/API/SBQueueItem.cpp
@@ -80,7 +80,7 @@
 
   SBAddress result;
   if (m_queue_item_sp) {
-result.SetAddress(&m_queue_item_sp->GetAddress());
+result.SetAddress(m_queue_item_sp->GetAddress());
   }
   return LLDB_RECORD_RESULT(result);
 }
Index: lldb/source/API/SBLineEntry.cpp
===
--- lldb/source/API/SBLineEntry.cpp
+++ lldb/source/API/SBLineEntry.cpp
@@ -56,7 +56,7 @@
 
   SBAddress sb_address;
   if (m_opaque_up)
-sb_address.SetAddress(&m_opaque_up->range.GetBaseAddress());
+sb_address.SetAddress(m_opaque_up->range.GetBaseAddress());
 
   return LLDB_RECORD_RESULT(sb_address);
 }
@@ -66,7 +66,7 @@
 
   SBAddress sb_address;
   if (m_opaque_up) {
-sb_address.SetAddress(&m_opaque_up->range.GetBaseAddress());
+sb_address.SetAddress(m_opaque_up->range.GetBaseAddress());
 sb_address.OffsetAddress(m_opaque_up->range.GetByteSize());
   }
   return LLDB_RECORD_RESULT(sb_address);
Index: lldb/source/API/SBInstruction.cpp
===
--- lldb/source/API/SBInstruction.cpp
+++ lldb/source/API/SBInstruction.cpp
@@ -107,7 +107,7 @@
   SBAddress sb_addr;
   lldb::InstructionSP inst_sp(GetOpaque());
   if (inst_sp && inst_sp->GetAddress().IsValid())
-sb_addr.SetAddress(&inst_sp->GetAddress());
+sb_addr.SetAddress(inst_sp->GetAddress());
   return LLDB_RECORD_RESULT(sb_addr);
 }
 
Index: lldb/source/API/SBFunction.cpp
===
--- lldb/source/API/SBFunction.cpp
+++ lldb/source/API/SBFunction.cpp
@@ -152,7 +152,7 @@
 
   SBAddress addr;
   if (m_opaque_ptr)
-addr.SetAddress(&m_opaque_ptr->GetAddressRange().GetBaseAddress());
+addr.SetAddress(m_opaque_ptr->GetAddressRange().GetBaseAddress());
   return LLDB_RECORD_RESULT(addr);
 }
 
@@ -163,7 +163,7 @@
   if (m_opaque_ptr) {
 addr_t byte_size = m_opaque_ptr->GetAddressRange().GetByteSize();
 if (byte_size > 0) {
-  addr.SetAddress(&m_opaque_ptr->GetAddressRange().GetBaseAddress());
+  addr.SetAddress(m_opaque_ptr->GetAddressRange().GetBaseAddress());
   addr->Slide(byte_size);
 }
   }
Index: lldb/source/API/SBFrame.cpp
===
--- lldb/source/API/SBFrame.cpp
+++ lldb/source/API/SBFrame.cpp
@@ -431,7 +431,7 @@
 if (stop_locker.TryLock(&process->GetRunLock())) {
   frame = exe_ctx.GetFramePtr();
   if (frame)
-sb_addr.SetAddress(&frame->GetFrameCodeAddress());
+sb_addr.SetAddress(frame->GetFrameCodeAddress());
 }
   }
   return LLDB_RECORD_RESULT(sb_addr);
Index: lldb/source/API/SBBreakpointLocation.cpp
===
--- lldb/source/API/SBBreakpointLocation.cpp
+++ lldb/source/API/SBBreakpointLocation.cpp
@@ -80,7 +80,7 @@
 
   BreakpointLocationSP loc_sp = GetSP();
   if (loc_sp) {
-

[Lldb-commits] [PATCH] D88247: Fix memory leak in SBValue::GetAddress

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Good catch, thank you! I wanted to suggest making the `SBAddress` constructor 
take the `Address` by const-reference but wanted to see how much work that'd be 
which resulted in D88249 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88247

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

[Lldb-commits] [PATCH] D88247: Fix memory leak in SBValue::GetAddress

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

I meant to accept this, we can land this and deal with my suggestion as a 
follow up. LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88247

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

[Lldb-commits] [PATCH] D87333: [lldb/ipv6] Support running lldb tests in an ipv6-only environment.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I am fine with trying "localhost" and seeing if we run into any issues. 
Hopefully slow DNS isn't a problem anymore on Macs. Everyone else ok?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87333

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

[Lldb-commits] [PATCH] D88257: [lldb/docs] Remove manual codesigning documentation

[Dave Lee via Phabricator via lldb-commits]

kastiglione created this revision.
kastiglione added reviewers: JDevlieghere, teemperor, friss.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
kastiglione requested review of this revision.

The `macos-setup-codesign.sh` script has been in place for over two years. If 
there are no known issues, it's a good time to drop the manual steps from the 
docs.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88257

Files:
  lldb/docs/resources/build.rst


Index: lldb/docs/resources/build.rst
===
--- lldb/docs/resources/build.rst
+++ lldb/docs/resources/build.rst
@@ -575,8 +575,11 @@
 
 To use the in-tree debug server on macOS, lldb needs to be code signed. The
 Debug, DebugClang and Release builds are set to code sign using a code signing
-certificate named ``lldb_codesign``. This document explains how to set up the
-signing certificate.
+certificate named ``lldb_codesign``.
+
+Automatic setup, run:
+
+* ``scripts/macos-setup-codesign.sh``
 
 Note that it's possible to build and use lldb on macOS without setting up code
 signing by using the system's debug server. To configure lldb in this way with
@@ -589,56 +592,6 @@
 code signing using the executable's file system node, so you will need to
 delete the file so the kernel clears its cache.
 
-Automatic setup:
-
-* Run ``scripts/macos-setup-codesign.sh``
-
-Manual setup steps:
-
-* Launch /Applications/Utilities/Keychain Access.app
-* In Keychain Access select the ``login`` keychain in the ``Keychains`` list in
-  the upper left hand corner of the window.
-* Select the following menu item: Keychain Access->Certificate 
Assistant->Create a Certificate...
-* Set the following settings
-
-::
-
-   Name = lldb_codesign
-   Identity Type = Self Signed Root
-   Certificate Type = Code Signing
-
-* Click Create
-* Click Continue
-* Click Done
-* Click on the "My Certificates"
-* Double click on your new ``lldb_codesign`` certificate
-* Turn down the "Trust" disclosure triangle, scroll to the "Code Signing" trust
-  pulldown menu and select "Always Trust" and authenticate as needed using your
-  username and password.
-* Drag the new ``lldb_codesign`` code signing certificate (not the public or
-  private keys of the same name) from the ``login`` keychain to the ``System``
-  keychain in the Keychains pane on the left hand side of the main Keychain
-  Access window. This will move this certificate to the ``System`` keychain.
-  You'll have to authorize a few more times, set it to be "Always trusted" when
-  asked.
-* Remove ``~/Desktop/lldb_codesign.cer`` file on your desktop if there is one.
-* In the Keychain Access GUI, click and drag ``lldb_codesign`` in the
-  ``System`` keychain onto the desktop. The drag will create a
-  ``Desktop/lldb_codesign.cer`` file used in the next step.
-* Switch to Terminal, and run the following:
-
-::
-
-  sudo security add-trust -d -r trustRoot -p basic -p codeSign -k 
/Library/Keychains/System.keychain ~/Desktop/lldb_codesign.cer
-  rm -f ~/Desktop/lldb_codesign.cer
-
-* Drag the ``lldb_codesign`` certificate from the ``System`` keychain back into
-  the ``login`` keychain
-* Quit Keychain Access
-* Reboot
-* Clean by removing all previously creating code signed binaries and rebuild
-  lldb and you should be able to debug.
-
 When you build your LLDB for the first time, the Xcode GUI will prompt you for
 permission to use the ``lldb_codesign`` keychain. Be sure to click "Always
 Allow" on your first build. From here on out, the ``lldb_codesign`` will be


Index: lldb/docs/resources/build.rst
===
--- lldb/docs/resources/build.rst
+++ lldb/docs/resources/build.rst
@@ -575,8 +575,11 @@
 
 To use the in-tree debug server on macOS, lldb needs to be code signed. The
 Debug, DebugClang and Release builds are set to code sign using a code signing
-certificate named ``lldb_codesign``. This document explains how to set up the
-signing certificate.
+certificate named ``lldb_codesign``.
+
+Automatic setup, run:
+
+* ``scripts/macos-setup-codesign.sh``
 
 Note that it's possible to build and use lldb on macOS without setting up code
 signing by using the system's debug server. To configure lldb in this way with
@@ -589,56 +592,6 @@
 code signing using the executable's file system node, so you will need to
 delete the file so the kernel clears its cache.
 
-Automatic setup:
-
-* Run ``scripts/macos-setup-codesign.sh``
-
-Manual setup steps:
-
-* Launch /Applications/Utilities/Keychain Access.app
-* In Keychain Access select the ``login`` keychain in the ``Keychains`` list in
-  the upper left hand corner of the window.
-* Select the following menu item: Keychain Access->Certificate Assistant->Create a Certificate...
-* Set the following settings
-
-::
-
-	Name = lldb_codesign
-	Identity Type = Self Signed Root
-	Certificate Type = Code Signing
-
-* Click Create
-* Click 

[Lldb-commits] [PATCH] D88257: [lldb/docs] Remove manual codesigning documentation

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

I don't mind keeping it around, but I also haven't used the manual steps since 
the script's inception, so LGTM. We can always reinstate it if someone 
complains.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88257

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

[Lldb-commits] [PATCH] D88257: [lldb/docs] Remove manual codesigning documentation

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

I agree it's nice to have somewhere, but git history should be fine. I should 
elaborate on motivations, 1. ensure everyone is using the script so that any 
issues are surfaced, 2. prevent missteps in the manual process, which may also 
be bit rotting if everyone is using the script.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88257

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

[Lldb-commits] [lldb] a079f61 - [LLDB] Add a defensive check for member__f_

[via lldb-commits]

Author: shafik
Date: 2020-09-24T14:48:21-07:00
New Revision: a079f619b5a1959af8af37cabdea27ae542903db

URL: 
https://github.com/llvm/llvm-project/commit/a079f619b5a1959af8af37cabdea27ae542903db
DIFF: 
https://github.com/llvm/llvm-project/commit/a079f619b5a1959af8af37cabdea27ae542903db.diff

LOG: [LLDB] Add a defensive check for member__f_

I only have a crash log and was not able to come up with a test case for this.

rdar://problem/69403150

Added: 


Modified: 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp 
b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
index 8aa803a8553e..d3a25f37985f 100644
--- a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
+++ b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
@@ -154,6 +154,9 @@ CPPLanguageRuntime::FindLibCppStdFunctionCallableInfo(
 member__f_ = sub_member__f_;
   }
 
+  if (!member__f_)
+return optional_info;
+
   lldb::addr_t member__f_pointer_value = member__f_->GetValueAsUnsigned(0);
 
   optional_info.member__f_pointer_value = member__f_pointer_value;



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

[Lldb-commits] [PATCH] D88264: [intel-pt] Refactor the JSON parsing

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
Herald added a reviewer: JDevlieghere.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
wallace requested review of this revision.

Recently https://reviews.llvm.org/D88103 introduced a nice API for
converting a JSON object into C++ types, which include nice error
messaging.

I'm using that new functioniality to perform the parsing in a much more
elegant way. As a result, the code looks simpler and more maintainable,
as we aren't parsing anymore individual fields manually.

I updated the test cases accordingly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88264

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceSettingsParser.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.h
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSettingsParser.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/intelpt-trace/trace_bad4.json

Index: lldb/test/API/commands/trace/intelpt-trace/trace_bad4.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace_bad4.json
@@ -0,0 +1,12 @@
+{
+  "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "model": 79,
+  "stepping": 1
+}
+  },
+  "processes": [
+  ]
+}
Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -41,17 +41,43 @@
 # We test first an invalid type
 trace_definition_file = os.path.join(src_dir, "intelpt-trace", "trace_bad.json")
 self.expect("trace load -v " + trace_definition_file, error=True,
-  substrs=['error: JSON value is expected to be "object"', "Value", "123", "Schema"])
+  substrs=['''error: expected object at settings.processes[0]
 
-# Now we test a missing field
+Context:
+{
+  "processes": [
+/* error: expected object */
+123
+  ],
+  "trace": { ... }
+}
+
+Schema:
+{
+ "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel" | "unknown",
+  "family": integer,
+  "model": integer,
+  "stepping": integer
+}
+  },'''])
+
+# Now we test a missing field in the global settings
+trace_definition_file2 = os.path.join(src_dir, "intelpt-trace", "trace_bad2.json")
+self.expect("trace load -v " + trace_definition_file2, error=True,
+substrs=['error: missing value at settings.processes[1].triple', "Context", "Schema"])
+
+# Now we test a missing field in the intel-pt settings
 trace_definition_file2 = os.path.join(src_dir, "intelpt-trace", "trace_bad2.json")
 self.expect("trace load -v " + trace_definition_file2, error=True,
-substrs=['error: JSON object is missing the "triple" field.', "Value", "pid", "12345", "Schema"])
+substrs=['error: missing value at settings.processes[1].triple', "Context", "Schema"])
 
 # The following wrong schema will have a valid target and an invalid one. In the case of failure,
 # no targets should be created.
 self.assertEqual(self.dbg.GetNumTargets(), 0)
 trace_definition_file2 = os.path.join(src_dir, "intelpt-trace", "trace_bad3.json")
 self.expect("trace load -v " + trace_definition_file2, error=True,
-substrs=['error: JSON object is missing the "pid" field.'])
+substrs=['error: missing value at settings.processes[1].pid'])
 self.assertEqual(self.dbg.GetNumTargets(), 0)
Index: lldb/source/Target/TraceSettingsParser.cpp
===
--- lldb/source/Target/TraceSettingsParser.cpp
+++ lldb/source/Target/TraceSettingsParser.cpp
@@ -18,105 +18,6 @@
 using namespace lldb_private;
 using namespace llvm;
 
-namespace json_helpers {
-
-llvm::Error CreateWrongTypeError(const json::Value &value,
- llvm::StringRef type) {
-  std::string s;
-  llvm::raw_string_ostream os(s);
-  os << llvm::formatv("JSON value is expected to be \"{0}\".\nValue:\n{1:2}",
-  type, value);
-  os.flush();
-
-  return llvm::createStringError(std::errc::invalid_argument, os.str().c_str());
-}
-
-llvm::Expected ToIntegerOrError(const json::Value &value) {
-  llvm::Optional v = value.getAsInteger();
-  if (v.hasValue())
-return *v;
-  return CreateWrongTypeError(value, "integer");
-}
-
-llvm::Expected ToStringOrError(const json::Value &value) {
-  llvm::Optional v = value.getAsString();
-  if (v.hasValue())
-return *v;
-  return CreateWrongTypeError(value, "string");
-}
-
-llvm::Expected ToArrayOrError(const json::Value &value) {
-  if (const json::Array *v = value.getAsArray())
-return *v;
-  return Crea

[Lldb-commits] [PATCH] D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: labath, clayborg.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
jingham requested review of this revision.
Herald added a subscriber: JDevlieghere.

Prevent a crash using SBStructuredData.GetDescription.

  

For some reason SBStructuredData requires a plugin for printing, and
doesn't have a default version that just prints the current contents
as JSON or something sensible?

  

I'm not sure what is supposed to happen here, but as the code stands
now, trying to call GetDescription on an SBStructuredData will crash
when the empty weak pointer is turned into a shared pointer.

  

This patch just adds a check that the weak pointer hasn't expired
 before trying to access it.  That prevents the crash.

This fix seems fairly obvious, but I put it up here in case somebody knows what 
is 
actually supposed to happen here, the state of things doesn't make much sense 
to me.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88266

Files:
  lldb/include/lldb/Core/StructuredDataImpl.h


Index: lldb/include/lldb/Core/StructuredDataImpl.h
===
--- lldb/include/lldb/Core/StructuredDataImpl.h
+++ lldb/include/lldb/Core/StructuredDataImpl.h
@@ -69,6 +69,11 @@
 }
 
 // Grab the plugin.
+if (m_plugin_wp.expired()) {
+  error.SetErrorString("Cannot pretty print structured data: "
+   "plugin is expired.");
+  return error;
+}
 auto plugin_sp = lldb::StructuredDataPluginSP(m_plugin_wp);
 if (!plugin_sp) {
   error.SetErrorString("Cannot pretty print structured data: "


Index: lldb/include/lldb/Core/StructuredDataImpl.h
===
--- lldb/include/lldb/Core/StructuredDataImpl.h
+++ lldb/include/lldb/Core/StructuredDataImpl.h
@@ -69,6 +69,11 @@
 }
 
 // Grab the plugin.
+if (m_plugin_wp.expired()) {
+  error.SetErrorString("Cannot pretty print structured data: "
+   "plugin is expired.");
+  return error;
+}
 auto plugin_sp = lldb::StructuredDataPluginSP(m_plugin_wp);
 if (!plugin_sp) {
   error.SetErrorString("Cannot pretty print structured data: "
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88264: [intel-pt] Refactor the JSON parsing

[Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbddebca61ea7: [intel-pt] Refactor the JSON parsing (authored 
by Walter Erquinigo ).

Changed prior to commit:
  https://reviews.llvm.org/D88264?vs=294185&id=294196#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88264

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceSettingsParser.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.h
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSettingsParser.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/intelpt-trace/trace_bad4.json
  lldb/test/API/commands/trace/intelpt-trace/trace_bad5.json

Index: lldb/test/API/commands/trace/intelpt-trace/trace_bad5.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace_bad5.json
@@ -0,0 +1,32 @@
+{
+  "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "family": 6,
+  "model": 79,
+  "stepping": 1
+}
+  },
+  "processes": [
+{
+  "pid": 1234,
+  "triple": "x86_64-*-linux",
+  "threads": [
+{
+  "tid": 5678,
+  "traceFile": "3842849.trace"
+}
+  ],
+  "modules": [
+{
+  "file": "a.out",
+  "systemPath": "a.out",
+  "loadAddress": 40,
+  "uuid": "6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A"
+}
+  ]
+},
+{}
+  ]
+}
Index: lldb/test/API/commands/trace/intelpt-trace/trace_bad4.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace_bad4.json
@@ -0,0 +1,12 @@
+{
+  "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "model": 79,
+  "stepping": 1
+}
+  },
+  "processes": [
+  ]
+}
Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -39,19 +39,56 @@
 def testLoadInvalidTraces(self):
 src_dir = self.getSourceDir()
 # We test first an invalid type
-trace_definition_file = os.path.join(src_dir, "intelpt-trace", "trace_bad.json")
-self.expect("trace load -v " + trace_definition_file, error=True,
-  substrs=['error: JSON value is expected to be "object"', "Value", "123", "Schema"])
+self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad.json"), error=True,
+  substrs=['''error: expected object at settings.processes[0]
 
-# Now we test a missing field
-trace_definition_file2 = os.path.join(src_dir, "intelpt-trace", "trace_bad2.json")
-self.expect("trace load -v " + trace_definition_file2, error=True,
-substrs=['error: JSON object is missing the "triple" field.', "Value", "pid", "12345", "Schema"])
+Context:
+{
+  "processes": [
+/* error: expected object */
+123
+  ],
+  "trace": { ... }
+}
+
+Schema:
+{
+ "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel" | "unknown",
+  "family": integer,
+  "model": integer,
+  "stepping": integer
+}
+  },'''])
+
+# Now we test a missing field in the global settings
+self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad2.json"), error=True,
+substrs=['error: missing value at settings.processes[1].triple', "Context", "Schema"])
+
+# Now we test a missing field in the intel-pt settings
+self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad4.json"), error=True,
+substrs=['''error: missing value at settings.trace.pt_cpu.family
+
+Context:
+{
+  "pt_cpu": /* error: missing value */ {
+"model": 79,
+"stepping": 1,
+"vendor": "intel"
+  },
+  "type": "intel-pt"
+}''', "Schema"])
+
+# Now we test an incorrect load address in the intel-pt settings
+self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad5.json"), error=True,
+substrs=['error: expected numeric string at settings.processes[0].modules[0].loadAddress',
+ '"loadAddress": /* error: expected numeric string */ 40,', "Schema"])
 
 # The following wrong schema will have a valid target and an invalid one. In the case of failure,
 # no targets should be created.
 self.assertEqual(self.dbg.GetNumTargets(), 0)
-trace_definition_file2 = os.path.join(src_dir, "intelpt-trace", "trace_bad3.json")
-self.expect("trace load -v 

[Lldb-commits] [lldb] bddebca - [intel-pt] Refactor the JSON parsing

[Walter Erquinigo via lldb-commits]

Author: Walter Erquinigo
Date: 2020-09-24T16:35:34-07:00
New Revision: bddebca61ea73d45d5eafc38aaf9fe7605f5a9b3

URL: 
https://github.com/llvm/llvm-project/commit/bddebca61ea73d45d5eafc38aaf9fe7605f5a9b3
DIFF: 
https://github.com/llvm/llvm-project/commit/bddebca61ea73d45d5eafc38aaf9fe7605f5a9b3.diff

LOG: [intel-pt] Refactor the JSON parsing

Recently https://reviews.llvm.org/D88103 introduced a nice API for
converting a JSON object into C++ types, which include nice error
messaging.

I'm using that new functioniality to perform the parsing in a much more
elegant way. As a result, the code looks simpler and more maintainable,
as we aren't parsing anymore individual fields manually.

I updated the test cases accordingly.

Differential Revision: https://reviews.llvm.org/D88264

Added: 
lldb/test/API/commands/trace/intelpt-trace/trace_bad4.json
lldb/test/API/commands/trace/intelpt-trace/trace_bad5.json

Modified: 
lldb/include/lldb/Target/Trace.h
lldb/include/lldb/Target/TraceSettingsParser.h
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.h
lldb/source/Target/Trace.cpp
lldb/source/Target/TraceSettingsParser.cpp
lldb/test/API/commands/trace/TestTraceLoad.py

Removed: 




diff  --git a/lldb/include/lldb/Target/Trace.h 
b/lldb/include/lldb/Target/Trace.h
index 2328fba822a2..e4e9b1aa88a7 100644
--- a/lldb/include/lldb/Target/Trace.h
+++ b/lldb/include/lldb/Target/Trace.h
@@ -111,7 +111,7 @@ class Trace : public PluginInterface {
   /// \return
   ///   An error object containing the reason if there is a failure.
   llvm::Error ParseSettings(Debugger &debugger,
-const llvm::json::Object &settings,
+const llvm::json::Value &settings,
 llvm::StringRef settings_dir);
 
   /// Get the JSON schema of the settings for the trace plug-in.

diff  --git a/lldb/include/lldb/Target/TraceSettingsParser.h 
b/lldb/include/lldb/Target/TraceSettingsParser.h
index bc18c107ed83..47e6556e1273 100644
--- a/lldb/include/lldb/Target/TraceSettingsParser.h
+++ b/lldb/include/lldb/Target/TraceSettingsParser.h
@@ -23,6 +23,38 @@ namespace lldb_private {
 /// overriden and implement the plug-in specific parsing logic.
 class TraceSettingsParser {
 public:
+  struct JSONAddress {
+lldb::addr_t value;
+  };
+
+  struct JSONModule {
+std::string system_path;
+llvm::Optional file;
+JSONAddress load_address;
+llvm::Optional uuid;
+  };
+
+  struct JSONThread {
+int64_t tid;
+std::string trace_file;
+  };
+
+  struct JSONProcess {
+int64_t pid;
+std::string triple;
+std::vector threads;
+std::vector modules;
+  };
+
+  struct JSONTracePluginSettings {
+std::string type;
+  };
+
+  struct JSONTraceSettings {
+std::vector processes;
+JSONTracePluginSettings trace;
+  };
+
   TraceSettingsParser(Trace &trace) : m_trace(trace) {}
 
   virtual ~TraceSettingsParser() = default;
@@ -46,7 +78,7 @@ class TraceSettingsParser {
   /// \return
   ///   An error object containing the reason if there is a failure.
   llvm::Error ParseSettings(Debugger &debugger,
-const llvm::json::Object &settings,
+const llvm::json::Value &settings,
 llvm::StringRef settings_dir);
 
 protected:
@@ -57,33 +89,44 @@ class TraceSettingsParser {
 
   /// Method that should be overriden to parse the plug-in specific settings.
   ///
+  /// \param[in] plugin_settings
+  ///   The settings to parse specific to the plugin.
+  ///
   /// \return
   ///   An error object containing the reason if there is a failure.
-  virtual llvm::Error ParsePluginSettings() = 0;
+  virtual llvm::Error
+  ParsePluginSettings(const llvm::json::Value &plugin_settings) = 0;
+
+  /// Create a user-friendly error message upon a JSON-parsing failure using 
the
+  /// \a json::ObjectMapper functionality.
+  ///
+  /// \param[in] root
+  ///   The \a llvm::json::Path::Root used to parse the JSON \a value.
+  ///
+  /// \param[in] value
+  ///   The json value that failed to parse.
+  ///
+  /// \return
+  ///   An \a llvm::Error containing the user-friendly error message.
+  llvm::Error CreateJSONError(llvm::json::Path::Root &root,
+  const llvm::json::Value &value);
 
 private:
-  /// Resolve non-absolute paths relativejto the settings folder
+  /// Resolve non-absolute paths relativeto the settings folder
   void NormalizePath(lldb_private::FileSpec &file_spec);
+
   llvm::Error ParseProcess(lldb_private::Debugger &debugger,
-   const llvm::json::Object &process);
-  llvm::Error ParseProcesses(lldb_private::Debugger &debugger);
-  llvm::Error ParseThread(lldb::ProcessSP &process_sp,
-  const llvm::json::Object &thr

[Lldb-commits] [PATCH] D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So StructuredData::Object does have a dump function:

  void Object::Dump(lldb_private::Stream &s, bool pretty_print = true) const;

Not sure when that got added. So we actually don't even need the m_plugin_wp 
anymore if we switch to using that?




Comment at: lldb/include/lldb/Core/StructuredDataImpl.h:72-85
+if (m_plugin_wp.expired()) {
+  error.SetErrorString("Cannot pretty print structured data: "
+   "plugin is expired.");
+  return error;
+}
 auto plugin_sp = lldb::StructuredDataPluginSP(m_plugin_wp);
 if (!plugin_sp) {

All of this could become:

```
m_data_sp->Dump(stream);
```
And we can get rid of m_plugin_wp from this class?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88266

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

[Lldb-commits] [PATCH] D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I am happier removing things when I know what they were for???  Not really sure 
why this plugin architecture is there.  My guess is it is part of Todd's os_log 
facility, but I'm not really sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88266

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

[Lldb-commits] [PATCH] D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

But, sure, if nobody can come up with a use for this plugin dealie, I'll get 
rid of it.  Otherwise I guess we should make a default plugin that calls "Dump"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88266

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

[Lldb-commits] [PATCH] D88266: Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

The only place m_plugin_wp is used is for dumping, so I would vote to get rid 
of it. (At least on a non-swift LLVM checkout).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88266

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