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

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG10b851434324: [lldb] Fix JSON parser to allow empty arrays 
(authored by tetsuo-cpp, committed by teemperor).

Repository:
  rG LLVM Github Monorepo

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 _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 _func = [](T &) {}) {
+  auto json_val = ParseJSON(json_string);
+  TestJSON(json_val.get(), test_func);
+}
+
+TEST(JSON, Parse) {
+  ParseAndTestJSON("\"foo\"", [](JSONString _val) {
+EXPECT_EQ(string_val.GetData(), "foo");
+  });
+  EXPECT_THAT(ParseJSON("\"foo"), testing::IsNull());
+  ParseAndTestJSON("3", [](JSONNumber _val) {
+EXPECT_EQ(number_val.GetAsSigned(), 3);
+EXPECT_EQ(number_val.GetAsUnsigned(), 3u);
+EXPECT_EQ(number_val.GetAsDouble(), 3.0);
+  });
+  ParseAndTestJSON("-5", [](JSONNumber _val) {
+EXPECT_EQ(number_val.GetAsSigned(), -5);
+EXPECT_EQ(number_val.GetAsDouble(), -5.0);
+  });
+  ParseAndTestJSON("-6.4", [](JSONNumber _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 _val) {
+TestJSON(obj_val.GetObject("key1").get(),
+ [](JSONNumber _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 _val) {
+   EXPECT_EQ(string_val.GetData(), "foobar");
+ });
+  });
+  ParseAndTestJSON("[1, \"bar\", 3.14]", [](JSONArray _val) {
+EXPECT_EQ(array_val.GetNumElements(), 3u);
+TestJSON(array_val.GetObject(0).get(),
+ [](JSONNumber _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 _val) { EXPECT_EQ(string_val.GetData(), "bar"); });
+TestJSON(array_val.GetObject(2).get(),
+ [](JSONNumber _val) {
+   EXPECT_EQ(number_val.GetAsSigned(), 3);
+   EXPECT_EQ(number_val.GetAsUnsigned(), 3u);
+   EXPECT_EQ(number_val.GetAsDouble(), 3.14);
+ });
+  });
+  ParseAndTestJSON("[]", [](JSONArray _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 

[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

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

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

I'm not sure whose approval are you waiting for, but this LGTM.


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

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

In D68179#1745614 , @tetsuo-cpp wrote:

> 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.


Once this is approved, I can run the tests for you to make sure it didn't 
regress [I do that regardless before committing].
That said, it would be good if you got your environment set up correctly [if 
there's something you don't understand, feel free to reach out on `llvm-dev` or 
`lldb-dev`, depending on the question].


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 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 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 _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 _func = [](T &) {}) {
+  auto json_val = ParseJSON(json_string);
+  TestJSON(json_val.get(), test_func);
+}
+
+TEST(JSON, Parse) {
+  ParseAndTestJSON("\"foo\"", [](JSONString _val) {
+EXPECT_EQ(string_val.GetData(), "foo");
+  });
+  EXPECT_THAT(ParseJSON("\"foo"), testing::IsNull());
+  ParseAndTestJSON("3", [](JSONNumber _val) {
+EXPECT_EQ(number_val.GetAsSigned(), 3);
+EXPECT_EQ(number_val.GetAsUnsigned(), 3u);
+EXPECT_EQ(number_val.GetAsDouble(), 3.0);
+  });
+  ParseAndTestJSON("-5", [](JSONNumber _val) {
+EXPECT_EQ(number_val.GetAsSigned(), -5);
+EXPECT_EQ(number_val.GetAsDouble(), -5.0);
+  });
+  ParseAndTestJSON("-6.4", [](JSONNumber _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 _val) {
+TestJSON(obj_val.GetObject("key1").get(),
+ [](JSONNumber _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 _val) {
+   EXPECT_EQ(string_val.GetData(), "foobar");
+ });
+  });
+  ParseAndTestJSON("[1, \"bar\", 3.14]", [](JSONArray _val) {
+EXPECT_EQ(array_val.GetNumElements(), 3u);
+TestJSON(array_val.GetObject(0).get(),
+ [](JSONNumber _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 _val) { EXPECT_EQ(string_val.GetData(), "bar"); });
+TestJSON(array_val.GetObject(2).get(),
+ [](JSONNumber _val) {
+   EXPECT_EQ(number_val.GetAsSigned(), 3);
+   EXPECT_EQ(number_val.GetAsUnsigned(), 3u);
+   EXPECT_EQ(number_val.GetAsDouble(), 3.14);
+ });
+  });
+  ParseAndTestJSON("[]", [](JSONArray _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 , const Token );
+
   JSONValue::SP ParseJSONObject();
 
   JSONValue::SP ParseJSONArray();
Index: 

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Looks good to me. If I was writing the test, I'd probably just make it do a 
string->json->string round-trip and then verify the final string. But this is 
fine too...




Comment at: lldb/unittests/debugserver/JSONTest.cpp:16-17
+void TestJSON(JSONValue *json_val, const std::function _func) {
+  EXPECT_THAT(json_val, testing::NotNull());
+  EXPECT_TRUE(T::classof(json_val));
+  test_func(static_cast(*json_val));

You may want to change these into ASSERT_xxx to avoid crashing later if the 
expectations are not met.


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

[Davide Italiano via Phabricator via lldb-commits]

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

Thanks for adding the tests. I took a look at the patch and it seems fine to 
me. Do you need somebody to commit this for you?
@jingham or @labath may want to have another look.


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 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 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 _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 _func = [](T &) {}) {
+  auto json_val = ParseJSON(json_string);
+  TestJSON(json_val.get(), test_func);
+}
+
+TEST(JSON, Parse) {
+  ParseAndTestJSON("\"foo\"", [](JSONString _val) {
+EXPECT_EQ(string_val.GetData(), "foo");
+  });
+  EXPECT_THAT(ParseJSON("\"foo"), testing::IsNull());
+  ParseAndTestJSON("3", [](JSONNumber _val) {
+EXPECT_EQ(number_val.GetAsSigned(), 3);
+EXPECT_EQ(number_val.GetAsUnsigned(), 3u);
+EXPECT_EQ(number_val.GetAsDouble(), 3.0);
+  });
+  ParseAndTestJSON("-5", [](JSONNumber _val) {
+EXPECT_EQ(number_val.GetAsSigned(), -5);
+EXPECT_EQ(number_val.GetAsDouble(), -5.0);
+  });
+  ParseAndTestJSON("-6.4", [](JSONNumber _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 _val) {
+TestJSON(obj_val.GetObject("key1").get(),
+ [](JSONNumber _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 _val) {
+   EXPECT_EQ(string_val.GetData(), "foobar");
+ });
+  });
+  ParseAndTestJSON("[1, \"bar\", 3.14]", [](JSONArray _val) {
+EXPECT_EQ(array_val.GetNumElements(), 3u);
+TestJSON(array_val.GetObject(0).get(),
+ [](JSONNumber _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 _val) { EXPECT_EQ(string_val.GetData(), "bar"); });
+TestJSON(array_val.GetObject(2).get(),
+ [](JSONNumber _val) {
+   EXPECT_EQ(number_val.GetAsSigned(), 3);
+   EXPECT_EQ(number_val.GetAsUnsigned(), 3u);
+   EXPECT_EQ(number_val.GetAsDouble(), 3.14);
+ });
+  });
+  ParseAndTestJSON("[]", [](JSONArray _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 , const Token );
+
   JSONValue::SP ParseJSONObject();
 
   JSONValue::SP 

[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 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 , const 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 ,
+ const 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 , const 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 ,
+ const 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

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

[Jim Ingham via lldb-commits]

debugserver doesn't use llvm classes, it's its own stand-alone thing.  So 
debugserver wasn't included in Jonas' changes.

Jim


> On Oct 1, 2019, at 3:11 PM, Alex Cameron via Phabricator 
>  wrote:
> 
> 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 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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

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.


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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

It would be good as Pavel and Davide suggest to write a test directly for the 
JSON parser.  Doing so in the C++ Unit test seems the most convenient, but 
that's up to you.  But it would also be good to add a test for the particular 
"breakpoint write" -> "breakpoint read" scenario that uncovered this bug.  
There are already some tests for that in the 
functionalities/breakpoints/serialize test case, so it should be easy to add 
one there.


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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D68179#1686893 , @davide wrote:

> This needs a test. You can either write a python one as the ones in 
> `test/testcases` or a lit-style based as the ones in `lit/`.


Another option might be a c++ unit test for the JSON class (see 
`unittests/Utility/JSONTest.cpp`).


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

[Davide Italiano via Phabricator via lldb-commits]

davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.

This needs a test. You can either write a python one as the ones in 
`test/testcases` or a lit-style based as the ones in `lit/`.


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 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 , const 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 ,
+ const 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 _parser);
+static StructuredData::ObjectSP ParseJSONValue(JSONParser _parser,
+   const std::string ,
+   const JSONParser::Token );
 static StructuredData::ObjectSP ParseJSONObject(JSONParser _parser);
 static StructuredData::ObjectSP ParseJSONArray(JSONParser _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 _parser) {
   std::string value;
   const JSONParser::Token token = json_parser.GetToken(value);
+  return ParseJSONValue(json_parser, value, token);
+}
+
+static StructuredData::ObjectSP ParseJSONValue(JSONParser _parser,
+   const std::string ,
+   const JSONParser::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::SP JSONParser::ParseJSONValue(const std::string ,
+

[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 , const 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 ,
+ const 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 _parser);
+static StructuredData::ObjectSP ParseJSONValue(JSONParser _parser,
+   const std::string ,
+   const JSONParser::Token );
 static StructuredData::ObjectSP ParseJSONObject(JSONParser _parser);
 static StructuredData::ObjectSP ParseJSONArray(JSONParser _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 _parser) {
   std::string value;
   const JSONParser::Token token = json_parser.GetToken(value);
+  return ParseJSONValue(json_parser, value, token);
+}
+
+static StructuredData::ObjectSP ParseJSONValue(JSONParser _parser,
+   const std::string ,
+   const JSONParser::Token ) {
   switch (token) {
   case JSONParser::Token::ObjectStart:
 return ParseJSONObject(json_parser);
Index: lldb/source/Utility/JSON.cpp