[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 created this revision.
cameron314 added reviewers: zturner, clayborg.
Herald added a subscriber: lldb-commits.

This patch changes the order that mutexes are acquired in SBProcess such that 
the target API mutex is now always acquired before the public run lock mutex is 
acquired.

This fixes a deadlock in LLDB when calling `SBProcess::Kill()` while calling 
e.g. `SBProcess::GetThreadByID()`: `Kill` takes the target API mutex, then 
waits for the private state thread to exit, which tries to lock the public run 
lock mutex, but it's already being held by `GetThreadByID`, which is waiting in 
turn for the target API mutex.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53412

Files:
  source/API/SBProcess.cpp

Index: source/API/SBProcess.cpp
===
--- source/API/SBProcess.cpp
+++ source/API/SBProcess.cpp
@@ -197,11 +197,10 @@
   uint32_t num_threads = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-Process::StopLocker stop_locker;
-
-const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
 std::lock_guard guard(
 process_sp->GetTarget().GetAPIMutex());
+Process::StopLocker stop_locker;
+const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
 num_threads = process_sp->GetThreadList().GetSize(can_update);
   }
 
@@ -457,10 +456,10 @@
   ThreadSP thread_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-Process::StopLocker stop_locker;
-const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
 std::lock_guard guard(
 process_sp->GetTarget().GetAPIMutex());
+Process::StopLocker stop_locker;
+const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
 thread_sp = process_sp->GetThreadList().GetThreadAtIndex(index, can_update);
 sb_thread.SetThread(thread_sp);
   }
@@ -480,10 +479,10 @@
   uint32_t num_queues = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
+std::lock_guard guard(
+process_sp->GetTarget().GetAPIMutex());
 Process::StopLocker stop_locker;
 if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-  std::lock_guard guard(
-  process_sp->GetTarget().GetAPIMutex());
   num_queues = process_sp->GetQueueList().GetSize();
 }
   }
@@ -502,10 +501,10 @@
   QueueSP queue_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
+std::lock_guard guard(
+process_sp->GetTarget().GetAPIMutex());
 Process::StopLocker stop_locker;
 if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-  std::lock_guard guard(
-  process_sp->GetTarget().GetAPIMutex());
   queue_sp = process_sp->GetQueueList().GetQueueAtIndex(index);
   sb_queue.SetQueue(queue_sp);
 }
@@ -816,10 +815,10 @@
   ThreadSP thread_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-Process::StopLocker stop_locker;
-const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
 std::lock_guard guard(
 process_sp->GetTarget().GetAPIMutex());
+Process::StopLocker stop_locker;
+const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
 thread_sp = process_sp->GetThreadList().FindThreadByID(tid, can_update);
 sb_thread.SetThread(thread_sp);
   }
@@ -839,10 +838,10 @@
   ThreadSP thread_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-Process::StopLocker stop_locker;
-const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
 std::lock_guard guard(
 process_sp->GetTarget().GetAPIMutex());
+Process::StopLocker stop_locker;
+const bool can_update = stop_locker.TryLock(&process_sp->GetRunLock());
 thread_sp =
 process_sp->GetThreadList().FindThreadByIndexID(index_id, can_update);
 sb_thread.SetThread(thread_sp);
@@ -959,10 +958,10 @@
 static_cast(sb_error.get()));
 
   if (process_sp) {
+std::lock_guard guard(
+process_sp->GetTarget().GetAPIMutex());
 Process::StopLocker stop_locker;
 if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-  std::lock_guard guard(
-  process_sp->GetTarget().GetAPIMutex());
   bytes_read = process_sp->ReadMemory(addr, dst, dst_len, sb_error.ref());
 } else {
   if (log)
@@ -993,10 +992,10 @@
   size_t bytes_read = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
+std::lock_guard guard(
+process_sp->GetTarget().GetAPIMutex());
 Process::StopLocker stop_locker;
 if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-  std::lock_guard guard(
-  process_sp->GetTarget().GetAPIMutex());
   bytes_read = process_sp->ReadCStringFromMemory(addr, (char *)buf, size,
  sb_error.ref());
 } else {
@@ -1018,10 +1017,10 @@
   uint64_t value = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
+std::lock_guard guard(
+pro

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

I suppose we could but there's a few places outside of SBProcess that also use 
the run lock and API mutex; personally, I prefer it to be explicit which mutex 
is being taken first.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53412



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

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

Ping?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53412



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

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.
Herald added a project: LLDB.

Anyone?
We still have this patch applied on our recently-rebased fork with no 
problems...


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



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

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

@clayborg, I'm not sure how that would work. There's many places that lock the 
process run lock without locking the target API mutex, and vice versa.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



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

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

There's dozens of places that take the API mutex without taking the process 
mutex. Take `Kill` for example: It needs to take the API mutex, but cannot take 
the run lock since it will be taken by the private state thread. Another 
example is `HandleCommand`, which takes the API mutex but has no process that 
it could ask to lock the API mutex for it.

On the flip side, all the `SBFrame` methods lock the process run lock but not 
the API mutex. And so on.

I just really don't think this can be refactored in a useful way without 
rewriting the way all SB locks are taken, which is almost impossible to do at 
this point.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



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

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

In D53412#1450182 , @clayborg wrote:

> All of these are in SBProcess. No need to change everywhere else, I just see 
> a ton of duplicated code here in the SBProcess.cpp. If we contain those in a 
> small struct/class, then we can easily make changes as needed without 50 
> diffs in this file each time. So no need to fix all the call sites in this 
> patch, just the ones in SBProcess.cpp


Ah, that makes more sense :-) Sorry I misinterpreted your suggestion.

I can add a RAII lock-helper struct in SBProcess, but the order that locks are 
taken within it must not be modified without changing *all* the functions that 
take both locks (both directly and indirectly), which includes a significant 
portion of the SB API (and not just SBProcess).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



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

[Lldb-commits] [PATCH] D89236: [lldb] Fix bitfield "frame var" for pointers (pr47743)

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

This fixes https://bugs.llvm.org/show_bug.cgi?id=47743 for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89236

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

[Lldb-commits] [PATCH] D68641: [LLDB] Fix for synthetic children memory leak

[Cameron via Phabricator via lldb-commits]

cameron314 created this revision.
cameron314 added reviewers: labath, jingham.
cameron314 added a project: LLDB.
Herald added subscribers: lldb-commits, JDevlieghere, aprantl.

This fixes a memory leak with several synthetic children front-ends.

The lifetime of a ValueObject and all its derivative ValueObjects (children, 
clones, etc.) is managed by a ClusterManager. These objects are only destroyed 
when every shared pointer to any of the managed objects in the cluster is 
destroyed. This means that no object in the cluster can store a shared pointer 
to another object in the cluster without creating a memory leak of the entire 
cluster. However, some of the synthetic children front-end implementations do 
exactly this; this patch fixes that.

The memory leak is actually much more important than just the lost memory, 
since it prevents the file handle to the binary (.elf in my case) from being 
closed due to references to the debug info from the ValueObject hierarchy. This 
means a build system is blocked from rebuilding the binary even after LLDB has 
disconnected.

Note that most of the existing synthetic children front-ends correctly avoid 
this buggy shared-pointer pattern with explicit comments about this exact type 
of memory leak. For example, in the NSErrorSyntheticFrontEnd:

  // the child here can be "real" (i.e. an actual child of the root) or
  // synthetized from raw memory if the former, I need to store a plain pointer
  // to it - or else a loop of references will cause this entire hierarchy of
  // values to leak if the latter, then I need to store a SharedPointer to it -
  // so that it only goes away when everyone else in the cluster goes away oh
  // joy!
  ValueObject *m_child_ptr;
  ValueObjectSP m_child_sp;

and the LibCxxMapIteratorSyntheticFrontEnd:

  // this must be a ValueObject* because it is a child of the ValueObject we
  // are producing children for it if were a ValueObjectSP, we would end up
  // with a loop (iterator -> synthetic -> child -> parent == iterator) and
  // that would in turn leak memory by never allowing the ValueObjects to die
  // and free their memory

Apparently I'm not the first to lose a few hours debugging this :-)


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D68641

Files:
  source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp
  source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp
  source/Plugins/Language/CPlusPlus/LibCxxQueue.cpp
  source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp
  source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp
  source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
  source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp

Index: source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
===
--- source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
+++ source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
@@ -39,9 +39,9 @@
   bool GetSummary(Stream &stream, const TypeSummaryOptions &options);
 
 private:
-  ValueObjectSP m_ptr_obj;
-  ValueObjectSP m_obj_obj;
-  ValueObjectSP m_del_obj;
+  ValueObject* m_ptr_obj = nullptr;
+  ValueObject* m_obj_obj = nullptr;
+  ValueObject* m_del_obj = nullptr;
 
   ValueObjectSP GetTuple();
 };
@@ -92,17 +92,17 @@
 
   ValueObjectSP ptr_obj = tuple_frontend->GetChildAtIndex(0);
   if (ptr_obj)
-m_ptr_obj = ptr_obj->Clone(ConstString("pointer"));
+m_ptr_obj = ptr_obj->Clone(ConstString("pointer")).get();
 
   ValueObjectSP del_obj = tuple_frontend->GetChildAtIndex(1);
   if (del_obj)
-m_del_obj = del_obj->Clone(ConstString("deleter"));
+m_del_obj = del_obj->Clone(ConstString("deleter")).get();
 
   if (m_ptr_obj) {
 Status error;
 ValueObjectSP obj_obj = m_ptr_obj->Dereference(error);
 if (error.Success()) {
-  m_obj_obj = obj_obj->Clone(ConstString("object"));
+  m_obj_obj = obj_obj->Clone(ConstString("object")).get();
 }
   }
 
@@ -114,11 +114,11 @@
 lldb::ValueObjectSP
 LibStdcppUniquePtrSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx == 0)
-return m_ptr_obj;
+return m_ptr_obj->GetSP();
   if (idx == 1)
-return m_del_obj;
+return m_del_obj->GetSP();
   if (idx == 2)
-return m_obj_obj;
+return m_obj_obj->GetSP();
   return lldb::ValueObjectSP();
 }
 
Index: source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
===
--- source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
+++ source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
@@ -37,7 +37,7 @@
   size_t GetIndexOfChildWithName(ConstString name) override;
 
 private:
-  std::vector m_members;
+  std::vector m_members;
 };
 
 } // end of anonymous namespace
@@ -72,7 +72,7 @@
 if (value_sp) {
   StreamString name;
   name.Printf("[%zd]", m_members.size());
-  m_members.push_back(value_sp->Clone(ConstString(name.GetString(;
+  m_members.push_back(value_sp->Clone(ConstString

[Lldb-commits] [PATCH] D68641: [LLDB] Fix for synthetic children memory leak

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

@aprantl No. A weak pointer would still fix the memory leak, but it's safe to 
use a raw pointer because we only reference objects which are in the same 
cluster as the synthetic children front-end itself. The other (leak-free) 
synthetic front-ends do this as well. We want shared/weak pointers externally, 
but not within the cluster.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68641



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

[Lldb-commits] [PATCH] D68641: [LLDB] Fix for synthetic children memory leak

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

Right, I'll add comments on the pointer declarations.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68641



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

[Lldb-commits] [PATCH] D68641: [LLDB] Fix for synthetic children memory leak

[Cameron via Phabricator via lldb-commits]

cameron314 updated this revision to Diff 223919.
cameron314 added a comment.

Added comments.


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

https://reviews.llvm.org/D68641

Files:
  source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp
  source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp
  source/Plugins/Language/CPlusPlus/LibCxxQueue.cpp
  source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp
  source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp
  source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
  source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp

Index: source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
===
--- source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
+++ source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
@@ -39,9 +39,14 @@
   bool GetSummary(Stream &stream, const TypeSummaryOptions &options);
 
 private:
-  ValueObjectSP m_ptr_obj;
-  ValueObjectSP m_obj_obj;
-  ValueObjectSP m_del_obj;
+  // The lifetime of a ValueObject and all its derivative ValueObjects
+  // (children, clones, etc.) is managed by a ClusterManager. These
+  // objects are only destroyed when every shared pointer to any of them
+  // is destroyed, so we must not store a shared pointer to any ValueObject
+  // derived from our backend ValueObject (since we're in the same cluster).
+  ValueObject* m_ptr_obj = nullptr;
+  ValueObject* m_obj_obj = nullptr;
+  ValueObject* m_del_obj = nullptr;
 
   ValueObjectSP GetTuple();
 };
@@ -92,17 +97,17 @@
 
   ValueObjectSP ptr_obj = tuple_frontend->GetChildAtIndex(0);
   if (ptr_obj)
-m_ptr_obj = ptr_obj->Clone(ConstString("pointer"));
+m_ptr_obj = ptr_obj->Clone(ConstString("pointer")).get();
 
   ValueObjectSP del_obj = tuple_frontend->GetChildAtIndex(1);
   if (del_obj)
-m_del_obj = del_obj->Clone(ConstString("deleter"));
+m_del_obj = del_obj->Clone(ConstString("deleter")).get();
 
   if (m_ptr_obj) {
 Status error;
 ValueObjectSP obj_obj = m_ptr_obj->Dereference(error);
 if (error.Success()) {
-  m_obj_obj = obj_obj->Clone(ConstString("object"));
+  m_obj_obj = obj_obj->Clone(ConstString("object")).get();
 }
   }
 
@@ -114,11 +119,11 @@
 lldb::ValueObjectSP
 LibStdcppUniquePtrSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx == 0)
-return m_ptr_obj;
+return m_ptr_obj->GetSP();
   if (idx == 1)
-return m_del_obj;
+return m_del_obj->GetSP();
   if (idx == 2)
-return m_obj_obj;
+return m_obj_obj->GetSP();
   return lldb::ValueObjectSP();
 }
 
Index: source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
===
--- source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
+++ source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
@@ -37,7 +37,12 @@
   size_t GetIndexOfChildWithName(ConstString name) override;
 
 private:
-  std::vector m_members;
+  // The lifetime of a ValueObject and all its derivative ValueObjects
+  // (children, clones, etc.) is managed by a ClusterManager. These
+  // objects are only destroyed when every shared pointer to any of them
+  // is destroyed, so we must not store a shared pointer to any ValueObject
+  // derived from our backend ValueObject (since we're in the same cluster).
+  std::vector m_members;
 };
 
 } // end of anonymous namespace
@@ -72,7 +77,7 @@
 if (value_sp) {
   StreamString name;
   name.Printf("[%zd]", m_members.size());
-  m_members.push_back(value_sp->Clone(ConstString(name.GetString(;
+  m_members.push_back(value_sp->Clone(ConstString(name.GetString())).get());
 }
   }
 }
@@ -86,7 +91,7 @@
 lldb::ValueObjectSP
 LibStdcppTupleSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx < m_members.size())
-return m_members[idx];
+return m_members[idx]->GetSP();
   return lldb::ValueObjectSP();
 }
 
Index: source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp
===
--- source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp
+++ source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp
@@ -184,7 +184,6 @@
 
 private:
   size_t m_size = 0;
-  ValueObjectSP m_base_sp;
 };
 } // namespace
 
Index: source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp
===
--- source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp
+++ source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp
@@ -30,47 +30,56 @@
   ValueObjectSP GetChildAtIndex(size_t idx) override;
 
 private:
-  std::vector m_elements;
-  ValueObjectSP m_base_sp;
+  // The lifetime of a ValueObject and all its derivative ValueObjects
+  // (children, clones, etc.) is managed by a ClusterManager. These
+  // objects are only destroyed when every shared pointer to any of them
+  // is destroyed, so we must not store a shared pointer to any ValueObject
+  // derived from our backend V

[Lldb-commits] [PATCH] D68641: [LLDB] Fix for synthetic children memory leak

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

Any objections before I commit?


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

https://reviews.llvm.org/D68641



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

[Lldb-commits] [PATCH] D68641: [LLDB] Fix for synthetic children memory leak

[Cameron via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG89386daa9571: [LLDB] Fix for synthetic children memory leak 
(authored by cameron314).

Changed prior to commit:
  https://reviews.llvm.org/D68641?vs=223919&id=224112#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68641

Files:
  lldb/source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxxQueue.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
  lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp

Index: lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
@@ -39,9 +39,14 @@
   bool GetSummary(Stream &stream, const TypeSummaryOptions &options);
 
 private:
-  ValueObjectSP m_ptr_obj;
-  ValueObjectSP m_obj_obj;
-  ValueObjectSP m_del_obj;
+  // The lifetime of a ValueObject and all its derivative ValueObjects
+  // (children, clones, etc.) is managed by a ClusterManager. These
+  // objects are only destroyed when every shared pointer to any of them
+  // is destroyed, so we must not store a shared pointer to any ValueObject
+  // derived from our backend ValueObject (since we're in the same cluster).
+  ValueObject* m_ptr_obj = nullptr;
+  ValueObject* m_obj_obj = nullptr;
+  ValueObject* m_del_obj = nullptr;
 
   ValueObjectSP GetTuple();
 };
@@ -92,17 +97,17 @@
 
   ValueObjectSP ptr_obj = tuple_frontend->GetChildAtIndex(0);
   if (ptr_obj)
-m_ptr_obj = ptr_obj->Clone(ConstString("pointer"));
+m_ptr_obj = ptr_obj->Clone(ConstString("pointer")).get();
 
   ValueObjectSP del_obj = tuple_frontend->GetChildAtIndex(1);
   if (del_obj)
-m_del_obj = del_obj->Clone(ConstString("deleter"));
+m_del_obj = del_obj->Clone(ConstString("deleter")).get();
 
   if (m_ptr_obj) {
 Status error;
 ValueObjectSP obj_obj = m_ptr_obj->Dereference(error);
 if (error.Success()) {
-  m_obj_obj = obj_obj->Clone(ConstString("object"));
+  m_obj_obj = obj_obj->Clone(ConstString("object")).get();
 }
   }
 
@@ -114,11 +119,11 @@
 lldb::ValueObjectSP
 LibStdcppUniquePtrSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx == 0)
-return m_ptr_obj;
+return m_ptr_obj->GetSP();
   if (idx == 1)
-return m_del_obj;
+return m_del_obj->GetSP();
   if (idx == 2)
-return m_obj_obj;
+return m_obj_obj->GetSP();
   return lldb::ValueObjectSP();
 }
 
Index: lldb/source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibStdcppTuple.cpp
@@ -37,7 +37,12 @@
   size_t GetIndexOfChildWithName(ConstString name) override;
 
 private:
-  std::vector m_members;
+  // The lifetime of a ValueObject and all its derivative ValueObjects
+  // (children, clones, etc.) is managed by a ClusterManager. These
+  // objects are only destroyed when every shared pointer to any of them
+  // is destroyed, so we must not store a shared pointer to any ValueObject
+  // derived from our backend ValueObject (since we're in the same cluster).
+  std::vector m_members;
 };
 
 } // end of anonymous namespace
@@ -72,7 +77,7 @@
 if (value_sp) {
   StreamString name;
   name.Printf("[%zd]", m_members.size());
-  m_members.push_back(value_sp->Clone(ConstString(name.GetString(;
+  m_members.push_back(value_sp->Clone(ConstString(name.GetString())).get());
 }
   }
 }
@@ -86,7 +91,7 @@
 lldb::ValueObjectSP
 LibStdcppTupleSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
   if (idx < m_members.size())
-return m_members[idx];
+return m_members[idx]->GetSP();
   return lldb::ValueObjectSP();
 }
 
Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp
@@ -184,7 +184,6 @@
 
 private:
   size_t m_size = 0;
-  ValueObjectSP m_base_sp;
 };
 } // namespace
 
Index: lldb/source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/LibCxxTuple.cpp
@@ -30,47 +30,56 @@
   ValueObjectSP GetChildAtIndex(size_t idx) override;
 
 private:
-  std::vector m_elements;
-  ValueObjectSP m_base_sp;
+  // The lifetime of

[Lldb-commits] [PATCH] D68641: [LLDB] Fix for synthetic children memory leak

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

I've committed a fix in rG745e57c5939e 
. Sorry 
about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68641



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

[Lldb-commits] [PATCH] D68179: [lldb] Fix JSON parser to allow empty arrays

[Alex Cameron via Phabricator via lldb-commits]

tetsuo-cpp created this revision.
tetsuo-cpp added a reviewer: xbolva00.
tetsuo-cpp added projects: LLVM, LLDB.
Herald added subscribers: lldb-commits, JDevlieghere.

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=39405

  alexc@kitty:~/work/wiredtiger/build_posix$ cat breakpoint.json
  [{"Breakpoint" : {"BKPTOptions" : {"AutoContinue" : false,"ConditionText" : 
"","EnabledState" : true,"IgnoreCount" : 0,"OneShotState" : 
false},"BKPTResolver" : {"Options" : {"NameMask" : [56],"Offset" : 
0,"SkipPrologue" : true,"SymbolNames" : ["__wt_btcur_search"]},"Type" : 
"SymbolName"},"Hardware" : false,"SearchFilter" : {"Options" : {},"Type" : 
"Unconstrained","Foo" : []}}}]

**Before**

  (lldb) breakpoint read --file breakpoint.json
  error: Invalid JSON from input file: 
/home/alexc/work/wiredtiger/build_posix/breakpoint.json.

**After**

  (lldb) breakpoint read --file breakpoint.json
  New breakpoints:
  Breakpoint 1: where = libwiredtiger-3.2.2.so`__wt_btcur_search + 15 at 
bt_cursor.c:522:5, address = 0x7576ab2f


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D68179

Files:
  lldb/include/lldb/Utility/JSON.h
  lldb/source/Utility/JSON.cpp
  lldb/source/Utility/StructuredData.cpp
  lldb/tools/debugserver/source/JSON.cpp
  lldb/tools/debugserver/source/JSON.h

Index: lldb/tools/debugserver/source/JSON.h
===
--- lldb/tools/debugserver/source/JSON.h
+++ lldb/tools/debugserver/source/JSON.h
@@ -292,6 +292,8 @@
   JSONValue::SP ParseJSONValue();
 
 protected:
+  JSONValue::SP ParseJSONValue(const std::string &value, const Token &token);
+
   JSONValue::SP ParseJSONObject();
 
   JSONValue::SP ParseJSONArray();
Index: lldb/tools/debugserver/source/JSON.cpp
===
--- lldb/tools/debugserver/source/JSON.cpp
+++ lldb/tools/debugserver/source/JSON.cpp
@@ -516,13 +516,16 @@
   std::string value;
   std::string key;
   while (true) {
-JSONValue::SP value_sp = ParseJSONValue();
+JSONParser::Token token = GetToken(value);
+if (token == JSONParser::Token::ArrayEnd)
+  return JSONValue::SP(array_up.release());
+JSONValue::SP value_sp = ParseJSONValue(value, token);
 if (value_sp)
   array_up->AppendObject(value_sp);
 else
   break;
 
-JSONParser::Token token = GetToken(value);
+token = GetToken(value);
 if (token == JSONParser::Token::Comma) {
   continue;
 } else if (token == JSONParser::Token::ArrayEnd) {
@@ -537,6 +540,13 @@
 JSONValue::SP JSONParser::ParseJSONValue() {
   std::string value;
   const JSONParser::Token token = GetToken(value);
+  return ParseJSONValue(value, token);
+}
+
+JSONValue::SP JSONParser::ParseJSONValue(const std::string &value,
+ const Token &token) {
+  std::string value;
+  const JSONParser::Token token = GetToken(value);
   switch (token) {
   case JSONParser::Token::ObjectStart:
 return ParseJSONObject();
Index: lldb/source/Utility/StructuredData.cpp
===
--- lldb/source/Utility/StructuredData.cpp
+++ lldb/source/Utility/StructuredData.cpp
@@ -24,6 +24,9 @@
 
 // Functions that use a JSONParser to parse JSON into StructuredData
 static StructuredData::ObjectSP ParseJSONValue(JSONParser &json_parser);
+static StructuredData::ObjectSP ParseJSONValue(JSONParser &json_parser,
+   const std::string &value,
+   const JSONParser::Token &token);
 static StructuredData::ObjectSP ParseJSONObject(JSONParser &json_parser);
 static StructuredData::ObjectSP ParseJSONArray(JSONParser &json_parser);
 
@@ -83,13 +86,17 @@
   std::string value;
   std::string key;
   while (true) {
-StructuredData::ObjectSP value_sp = ParseJSONValue(json_parser);
+JSONParser::Token token = json_parser.GetToken(value);
+if (token == JSONParser::Token::ArrayEnd)
+  return StructuredData::ObjectSP(array_up.release());
+StructuredData::ObjectSP value_sp =
+ParseJSONValue(json_parser, value, token);
 if (value_sp)
   array_up->AddItem(value_sp);
 else
   break;
 
-JSONParser::Token token = json_parser.GetToken(value);
+token = json_parser.GetToken(value);
 if (token == JSONParser::Token::Comma) {
   continue;
 } else if (token == JSONParser::Token::ArrayEnd) {
@@ -104,6 +111,12 @@
 static StructuredData::ObjectSP ParseJSONValue(JSONParser &json_parser) {
   std::string value;
   const JSONParser::Token token = json_parser.GetToken(value);
+  return ParseJSONValue(json_parser, value, token);
+}
+
+static StructuredData::ObjectSP ParseJSONValue(JSONParser &json_parser,
+   const std::string &value,
+   const JSONParser::Token &token) {
   switch (token) {
   case JSONParser::Token::ObjectStart:
 ret

[Lldb-commits] [PATCH] D68179: [lldb] Fix JSON parser to allow empty arrays

[Alex Cameron via Phabricator via lldb-commits]

tetsuo-cpp updated this revision to Diff 81.

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

https://reviews.llvm.org/D68179

Files:
  lldb/include/lldb/Utility/JSON.h
  lldb/source/Utility/JSON.cpp
  lldb/source/Utility/StructuredData.cpp
  lldb/tools/debugserver/source/JSON.cpp
  lldb/tools/debugserver/source/JSON.h

Index: lldb/tools/debugserver/source/JSON.h
===
--- lldb/tools/debugserver/source/JSON.h
+++ lldb/tools/debugserver/source/JSON.h
@@ -292,6 +292,8 @@
   JSONValue::SP ParseJSONValue();
 
 protected:
+  JSONValue::SP ParseJSONValue(const std::string &value, const Token &token);
+
   JSONValue::SP ParseJSONObject();
 
   JSONValue::SP ParseJSONArray();
Index: lldb/tools/debugserver/source/JSON.cpp
===
--- lldb/tools/debugserver/source/JSON.cpp
+++ lldb/tools/debugserver/source/JSON.cpp
@@ -516,13 +516,16 @@
   std::string value;
   std::string key;
   while (true) {
-JSONValue::SP value_sp = ParseJSONValue();
+JSONParser::Token token = GetToken(value);
+if (token == JSONParser::Token::ArrayEnd)
+  return JSONValue::SP(array_up.release());
+JSONValue::SP value_sp = ParseJSONValue(value, token);
 if (value_sp)
   array_up->AppendObject(value_sp);
 else
   break;
 
-JSONParser::Token token = GetToken(value);
+token = GetToken(value);
 if (token == JSONParser::Token::Comma) {
   continue;
 } else if (token == JSONParser::Token::ArrayEnd) {
@@ -537,6 +540,11 @@
 JSONValue::SP JSONParser::ParseJSONValue() {
   std::string value;
   const JSONParser::Token token = GetToken(value);
+  return ParseJSONValue(value, token);
+}
+
+JSONValue::SP JSONParser::ParseJSONValue(const std::string &value,
+ const Token &token) {
   switch (token) {
   case JSONParser::Token::ObjectStart:
 return ParseJSONObject();
Index: lldb/source/Utility/StructuredData.cpp
===
--- lldb/source/Utility/StructuredData.cpp
+++ lldb/source/Utility/StructuredData.cpp
@@ -24,6 +24,9 @@
 
 // Functions that use a JSONParser to parse JSON into StructuredData
 static StructuredData::ObjectSP ParseJSONValue(JSONParser &json_parser);
+static StructuredData::ObjectSP ParseJSONValue(JSONParser &json_parser,
+   const std::string &value,
+   const JSONParser::Token &token);
 static StructuredData::ObjectSP ParseJSONObject(JSONParser &json_parser);
 static StructuredData::ObjectSP ParseJSONArray(JSONParser &json_parser);
 
@@ -83,13 +86,17 @@
   std::string value;
   std::string key;
   while (true) {
-StructuredData::ObjectSP value_sp = ParseJSONValue(json_parser);
+JSONParser::Token token = json_parser.GetToken(value);
+if (token == JSONParser::Token::ArrayEnd)
+  return StructuredData::ObjectSP(array_up.release());
+StructuredData::ObjectSP value_sp =
+ParseJSONValue(json_parser, value, token);
 if (value_sp)
   array_up->AddItem(value_sp);
 else
   break;
 
-JSONParser::Token token = json_parser.GetToken(value);
+token = json_parser.GetToken(value);
 if (token == JSONParser::Token::Comma) {
   continue;
 } else if (token == JSONParser::Token::ArrayEnd) {
@@ -104,6 +111,12 @@
 static StructuredData::ObjectSP ParseJSONValue(JSONParser &json_parser) {
   std::string value;
   const JSONParser::Token token = json_parser.GetToken(value);
+  return ParseJSONValue(json_parser, value, token);
+}
+
+static StructuredData::ObjectSP ParseJSONValue(JSONParser &json_parser,
+   const std::string &value,
+   const JSONParser::Token &token) {
   switch (token) {
   case JSONParser::Token::ObjectStart:
 return ParseJSONObject(json_parser);
Index: lldb/source/Utility/JSON.cpp
===
--- lldb/source/Utility/JSON.cpp
+++ lldb/source/Utility/JSON.cpp
@@ -485,13 +485,16 @@
   std::string value;
   std::string key;
   while (true) {
-JSONValue::SP value_sp = ParseJSONValue();
+JSONParser::Token token = GetToken(value);
+if (token == JSONParser::Token::ArrayEnd)
+  return JSONValue::SP(array_up.release());
+JSONValue::SP value_sp = ParseJSONValue(value, token);
 if (value_sp)
   array_up->AppendObject(value_sp);
 else
   break;
 
-JSONParser::Token token = GetToken(value);
+token = GetToken(value);
 if (token == JSONParser::Token::Comma) {
   continue;
 } else if (token == JSONParser::Token::ArrayEnd) {
@@ -506,6 +509,11 @@
 JSONValue::SP JSONParser::ParseJSONValue() {
   std::string value;
   const JSONParser::Token token = GetToken(value);
+  return ParseJSONValue(value, token);
+}
+
+JSONValue::S

[Lldb-commits] [PATCH] D68179: [lldb] Fix JSON parser to allow empty arrays

[Alex Cameron via Phabricator via lldb-commits]

tetsuo-cpp added a comment.

In D68179#1690073 , @JDevlieghere 
wrote:

> Hey, I just want to give you a heads up that I'm in the process to replace 
> LLDB's JSON implementation with the one from LLVM. The parts in 
> StructuredData are already gone (r373359, r373360) and I'm currently working 
> on the other uses in LLDB, except for debugserver which I'm not planning to 
> touch for now.


Thanks for letting me know.
Did you want the `debugserver` portion of this change? Or should I just close 
this.


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

https://reviews.llvm.org/D68179



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

[Lldb-commits] [PATCH] D68179: [lldb] Fix JSON parser to allow empty arrays

[Alex Cameron via Phabricator via lldb-commits]

tetsuo-cpp updated this revision to Diff 222923.
tetsuo-cpp added a comment.

Rebased onto trunk.


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

https://reviews.llvm.org/D68179

Files:
  lldb/tools/debugserver/source/JSON.cpp
  lldb/tools/debugserver/source/JSON.h


Index: lldb/tools/debugserver/source/JSON.h
===
--- lldb/tools/debugserver/source/JSON.h
+++ lldb/tools/debugserver/source/JSON.h
@@ -292,6 +292,8 @@
   JSONValue::SP ParseJSONValue();
 
 protected:
+  JSONValue::SP ParseJSONValue(const std::string &value, const Token &token);
+
   JSONValue::SP ParseJSONObject();
 
   JSONValue::SP ParseJSONArray();
Index: lldb/tools/debugserver/source/JSON.cpp
===
--- lldb/tools/debugserver/source/JSON.cpp
+++ lldb/tools/debugserver/source/JSON.cpp
@@ -516,13 +516,16 @@
   std::string value;
   std::string key;
   while (true) {
-JSONValue::SP value_sp = ParseJSONValue();
+JSONParser::Token token = GetToken(value);
+if (token == JSONParser::Token::ArrayEnd)
+  return JSONValue::SP(array_up.release());
+JSONValue::SP value_sp = ParseJSONValue(value, token);
 if (value_sp)
   array_up->AppendObject(value_sp);
 else
   break;
 
-JSONParser::Token token = GetToken(value);
+token = GetToken(value);
 if (token == JSONParser::Token::Comma) {
   continue;
 } else if (token == JSONParser::Token::ArrayEnd) {
@@ -537,6 +540,11 @@
 JSONValue::SP JSONParser::ParseJSONValue() {
   std::string value;
   const JSONParser::Token token = GetToken(value);
+  return ParseJSONValue(value, token);
+}
+
+JSONValue::SP JSONParser::ParseJSONValue(const std::string &value,
+ const Token &token) {
   switch (token) {
   case JSONParser::Token::ObjectStart:
 return ParseJSONObject();


Index: lldb/tools/debugserver/source/JSON.h
===
--- lldb/tools/debugserver/source/JSON.h
+++ lldb/tools/debugserver/source/JSON.h
@@ -292,6 +292,8 @@
   JSONValue::SP ParseJSONValue();
 
 protected:
+  JSONValue::SP ParseJSONValue(const std::string &value, const Token &token);
+
   JSONValue::SP ParseJSONObject();
 
   JSONValue::SP ParseJSONArray();
Index: lldb/tools/debugserver/source/JSON.cpp
===
--- lldb/tools/debugserver/source/JSON.cpp
+++ lldb/tools/debugserver/source/JSON.cpp
@@ -516,13 +516,16 @@
   std::string value;
   std::string key;
   while (true) {
-JSONValue::SP value_sp = ParseJSONValue();
+JSONParser::Token token = GetToken(value);
+if (token == JSONParser::Token::ArrayEnd)
+  return JSONValue::SP(array_up.release());
+JSONValue::SP value_sp = ParseJSONValue(value, token);
 if (value_sp)
   array_up->AppendObject(value_sp);
 else
   break;
 
-JSONParser::Token token = GetToken(value);
+token = GetToken(value);
 if (token == JSONParser::Token::Comma) {
   continue;
 } else if (token == JSONParser::Token::ArrayEnd) {
@@ -537,6 +540,11 @@
 JSONValue::SP JSONParser::ParseJSONValue() {
   std::string value;
   const JSONParser::Token token = GetToken(value);
+  return ParseJSONValue(value, token);
+}
+
+JSONValue::SP JSONParser::ParseJSONValue(const std::string &value,
+ const Token &token) {
   switch (token) {
   case JSONParser::Token::ObjectStart:
 return ParseJSONObject();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D68179: [lldb] Fix JSON parser to allow empty arrays

[Alex Cameron via Phabricator via lldb-commits]

tetsuo-cpp added a comment.

Thank you for the suggestions!

I'm assuming that since only the `debugserver` portion of this change is left, 
I should write a test specifically for that. I had a quick look and it wasn't 
obvious how to do that but I will spend some more time on it this weekend.

@jingham
Regarding the idea of a serialization test, I've only ever triggered the bug by 
manually adding an empty array to some breakpoint JSON. I'm not sure how the 
reporter stumbled across the issue "naturally". Is it ok to have a 
serialization test where the test itself edits the JSON after it has been 
written? Or would it be better if I find some scenario where LLDB writes JSON 
with an empty array and trigger the bug by reading it back in?


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

https://reviews.llvm.org/D68179



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

[Lldb-commits] [PATCH] D68179: [lldb] Fix JSON parser to allow empty arrays

[Alex Cameron via Phabricator via lldb-commits]

tetsuo-cpp updated this revision to Diff 228561.
Herald added a subscriber: mgorny.

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

https://reviews.llvm.org/D68179

Files:
  lldb/tools/debugserver/source/JSON.cpp
  lldb/tools/debugserver/source/JSON.h
  lldb/unittests/debugserver/CMakeLists.txt
  lldb/unittests/debugserver/JSONTest.cpp

Index: lldb/unittests/debugserver/JSONTest.cpp
===
--- /dev/null
+++ lldb/unittests/debugserver/JSONTest.cpp
@@ -0,0 +1,89 @@
+//===-- JSONTest.cpp ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "JSON.h"
+
+template 
+void TestJSON(JSONValue *json_val, const std::function &test_func) {
+  EXPECT_THAT(json_val, testing::NotNull());
+  EXPECT_TRUE(T::classof(json_val));
+  test_func(static_cast(*json_val));
+}
+
+JSONValue::SP ParseJSON(const char *json_string) {
+  return JSONParser(json_string).ParseJSONValue();
+}
+
+template 
+void ParseAndTestJSON(
+const char *json_string,
+const std::function &test_func = [](T &) {}) {
+  auto json_val = ParseJSON(json_string);
+  TestJSON(json_val.get(), test_func);
+}
+
+TEST(JSON, Parse) {
+  ParseAndTestJSON("\"foo\"", [](JSONString &string_val) {
+EXPECT_EQ(string_val.GetData(), "foo");
+  });
+  EXPECT_THAT(ParseJSON("\"foo"), testing::IsNull());
+  ParseAndTestJSON("3", [](JSONNumber &number_val) {
+EXPECT_EQ(number_val.GetAsSigned(), 3);
+EXPECT_EQ(number_val.GetAsUnsigned(), 3u);
+EXPECT_EQ(number_val.GetAsDouble(), 3.0);
+  });
+  ParseAndTestJSON("-5", [](JSONNumber &number_val) {
+EXPECT_EQ(number_val.GetAsSigned(), -5);
+EXPECT_EQ(number_val.GetAsDouble(), -5.0);
+  });
+  ParseAndTestJSON("-6.4", [](JSONNumber &number_val) {
+EXPECT_EQ(number_val.GetAsSigned(), -6);
+EXPECT_EQ(number_val.GetAsDouble(), -6.4);
+  });
+  EXPECT_THAT(ParseJSON("-1.2.3"), testing::IsNull());
+  ParseAndTestJSON("true");
+  ParseAndTestJSON("false");
+  ParseAndTestJSON("null");
+  ParseAndTestJSON(
+  "{ \"key1\": 4, \"key2\": \"foobar\" }", [](JSONObject &obj_val) {
+TestJSON(obj_val.GetObject("key1").get(),
+ [](JSONNumber &number_val) {
+   EXPECT_EQ(number_val.GetAsSigned(), 4);
+   EXPECT_EQ(number_val.GetAsUnsigned(), 4u);
+   EXPECT_EQ(number_val.GetAsDouble(), 4.0);
+ });
+TestJSON(obj_val.GetObject("key2").get(),
+ [](JSONString &string_val) {
+   EXPECT_EQ(string_val.GetData(), "foobar");
+ });
+  });
+  ParseAndTestJSON("[1, \"bar\", 3.14]", [](JSONArray &array_val) {
+EXPECT_EQ(array_val.GetNumElements(), 3u);
+TestJSON(array_val.GetObject(0).get(),
+ [](JSONNumber &number_val) {
+   EXPECT_EQ(number_val.GetAsSigned(), 1);
+   EXPECT_EQ(number_val.GetAsUnsigned(), 1u);
+   EXPECT_EQ(number_val.GetAsDouble(), 1.0);
+ });
+TestJSON(
+array_val.GetObject(1).get(),
+[](JSONString &string_val) { EXPECT_EQ(string_val.GetData(), "bar"); });
+TestJSON(array_val.GetObject(2).get(),
+ [](JSONNumber &number_val) {
+   EXPECT_EQ(number_val.GetAsSigned(), 3);
+   EXPECT_EQ(number_val.GetAsUnsigned(), 3u);
+   EXPECT_EQ(number_val.GetAsDouble(), 3.14);
+ });
+  });
+  ParseAndTestJSON("[]", [](JSONArray &array_val) {
+EXPECT_EQ(array_val.GetNumElements(), 0u);
+  });
+}
Index: lldb/unittests/debugserver/CMakeLists.txt
===
--- lldb/unittests/debugserver/CMakeLists.txt
+++ lldb/unittests/debugserver/CMakeLists.txt
@@ -8,6 +8,7 @@
 ${LLDB_SOURCE_DIR}/tools/debugserver/source/MacOSX)
 
 add_lldb_unittest(debugserverTests
+  JSONTest.cpp
   RNBSocketTest.cpp
   debugserver_LogCallback.cpp
 
@@ -24,8 +25,9 @@
   WITH_FBS
   WITH_BKS
   )
-  
+
   add_lldb_unittest(debugserverNonUITests
+JSONTest.cpp
 RNBSocketTest.cpp
 debugserver_LogCallback.cpp
 
Index: lldb/tools/debugserver/source/JSON.h
===
--- lldb/tools/debugserver/source/JSON.h
+++ lldb/tools/debugserver/source/JSON.h
@@ -292,6 +292,8 @@
   JSONValue::SP ParseJSONValue();
 
 protected:
+  JSONValue::SP ParseJSONValue(const std::

[Lldb-commits] [PATCH] D68179: [lldb] Fix JSON parser to allow empty arrays

[Alex Cameron via Phabricator via lldb-commits]

tetsuo-cpp added a comment.

I've had a try at writing some unit tests for the JSON parser in `debugserver`, 
including the empty array case which I'm fixing in this patch.


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

https://reviews.llvm.org/D68179



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

[Lldb-commits] [PATCH] D68179: [lldb] Fix JSON parser to allow empty arrays

[Alex Cameron via Phabricator via lldb-commits]

tetsuo-cpp added a comment.

Friendly ping. Could I please get this looked at?


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

https://reviews.llvm.org/D68179



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

[Lldb-commits] [PATCH] D68179: [lldb] Fix JSON parser to allow empty arrays

[Alex Cameron via Phabricator via lldb-commits]

tetsuo-cpp updated this revision to Diff 229291.

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

https://reviews.llvm.org/D68179

Files:
  lldb/tools/debugserver/source/JSON.cpp
  lldb/tools/debugserver/source/JSON.h
  lldb/unittests/debugserver/CMakeLists.txt
  lldb/unittests/debugserver/JSONTest.cpp

Index: lldb/unittests/debugserver/JSONTest.cpp
===
--- /dev/null
+++ lldb/unittests/debugserver/JSONTest.cpp
@@ -0,0 +1,89 @@
+//===-- JSONTest.cpp *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "JSON.h"
+
+template 
+void TestJSON(JSONValue *json_val, const std::function &test_func) {
+  ASSERT_THAT(json_val, testing::NotNull());
+  ASSERT_TRUE(T::classof(json_val));
+  test_func(static_cast(*json_val));
+}
+
+JSONValue::SP ParseJSON(const char *json_string) {
+  return JSONParser(json_string).ParseJSONValue();
+}
+
+template 
+void ParseAndTestJSON(
+const char *json_string,
+const std::function &test_func = [](T &) {}) {
+  auto json_val = ParseJSON(json_string);
+  TestJSON(json_val.get(), test_func);
+}
+
+TEST(JSON, Parse) {
+  ParseAndTestJSON("\"foo\"", [](JSONString &string_val) {
+EXPECT_EQ(string_val.GetData(), "foo");
+  });
+  EXPECT_THAT(ParseJSON("\"foo"), testing::IsNull());
+  ParseAndTestJSON("3", [](JSONNumber &number_val) {
+EXPECT_EQ(number_val.GetAsSigned(), 3);
+EXPECT_EQ(number_val.GetAsUnsigned(), 3u);
+EXPECT_EQ(number_val.GetAsDouble(), 3.0);
+  });
+  ParseAndTestJSON("-5", [](JSONNumber &number_val) {
+EXPECT_EQ(number_val.GetAsSigned(), -5);
+EXPECT_EQ(number_val.GetAsDouble(), -5.0);
+  });
+  ParseAndTestJSON("-6.4", [](JSONNumber &number_val) {
+EXPECT_EQ(number_val.GetAsSigned(), -6);
+EXPECT_EQ(number_val.GetAsDouble(), -6.4);
+  });
+  EXPECT_THAT(ParseJSON("-1.2.3"), testing::IsNull());
+  ParseAndTestJSON("true");
+  ParseAndTestJSON("false");
+  ParseAndTestJSON("null");
+  ParseAndTestJSON(
+  "{ \"key1\": 4, \"key2\": \"foobar\" }", [](JSONObject &obj_val) {
+TestJSON(obj_val.GetObject("key1").get(),
+ [](JSONNumber &number_val) {
+   EXPECT_EQ(number_val.GetAsSigned(), 4);
+   EXPECT_EQ(number_val.GetAsUnsigned(), 4u);
+   EXPECT_EQ(number_val.GetAsDouble(), 4.0);
+ });
+TestJSON(obj_val.GetObject("key2").get(),
+ [](JSONString &string_val) {
+   EXPECT_EQ(string_val.GetData(), "foobar");
+ });
+  });
+  ParseAndTestJSON("[1, \"bar\", 3.14]", [](JSONArray &array_val) {
+EXPECT_EQ(array_val.GetNumElements(), 3u);
+TestJSON(array_val.GetObject(0).get(),
+ [](JSONNumber &number_val) {
+   EXPECT_EQ(number_val.GetAsSigned(), 1);
+   EXPECT_EQ(number_val.GetAsUnsigned(), 1u);
+   EXPECT_EQ(number_val.GetAsDouble(), 1.0);
+ });
+TestJSON(
+array_val.GetObject(1).get(),
+[](JSONString &string_val) { EXPECT_EQ(string_val.GetData(), "bar"); });
+TestJSON(array_val.GetObject(2).get(),
+ [](JSONNumber &number_val) {
+   EXPECT_EQ(number_val.GetAsSigned(), 3);
+   EXPECT_EQ(number_val.GetAsUnsigned(), 3u);
+   EXPECT_EQ(number_val.GetAsDouble(), 3.14);
+ });
+  });
+  ParseAndTestJSON("[]", [](JSONArray &array_val) {
+EXPECT_EQ(array_val.GetNumElements(), 0u);
+  });
+}
Index: lldb/unittests/debugserver/CMakeLists.txt
===
--- lldb/unittests/debugserver/CMakeLists.txt
+++ lldb/unittests/debugserver/CMakeLists.txt
@@ -8,6 +8,7 @@
 ${LLDB_SOURCE_DIR}/tools/debugserver/source/MacOSX)
 
 add_lldb_unittest(debugserverTests
+  JSONTest.cpp
   RNBSocketTest.cpp
   debugserver_LogCallback.cpp
 
@@ -24,8 +25,9 @@
   WITH_FBS
   WITH_BKS
   )
-  
+
   add_lldb_unittest(debugserverNonUITests
+JSONTest.cpp
 RNBSocketTest.cpp
 debugserver_LogCallback.cpp
 
Index: lldb/tools/debugserver/source/JSON.h
===
--- lldb/tools/debugserver/source/JSON.h
+++ lldb/tools/debugserver/source/JSON.h
@@ -292,6 +292,8 @@
   JSONValue::SP ParseJSONValue();
 
 protected:
+  JSONValue::SP ParseJSONValue(const std::string &value, const Token &to

[Lldb-commits] [PATCH] D68179: [lldb] Fix JSON parser to allow empty arrays

[Alex Cameron via Phabricator via lldb-commits]

tetsuo-cpp marked an inline comment as done.
tetsuo-cpp added a comment.

Thanks for looking at this. I will need someone to commit it for me.
However, I've been having issues with the test suite on my MacBook. 
`check-lldb-unit` works for me but `check-llvm` and `check-lldb` are hitting 
issues because of something wrong with my environment. So I'll need to sort 
this out first to verify that I haven't broken anything before it's committed. 
I'm hoping to spend some time this weekend to debug my setup.


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

https://reviews.llvm.org/D68179



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

[Lldb-commits] [PATCH] D68179: [lldb] Fix JSON parser to allow empty arrays

[Alex Cameron via Phabricator via lldb-commits]

tetsuo-cpp added a comment.

Thanks for the reviews!
@davide could you please commit this on my behalf?

Thanks for the advice. I intend to continue working on LLDB when I can so I 
will spend some time to fix the test suite on my Mac.


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

https://reviews.llvm.org/D68179



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