[lldb] Fix JSON parser to allow empty arrays

[tetsuo-cpp]

Summary

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 = 0x00007ffff576ab2f

[davide]

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

[labath]

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

[jingham]

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.

[JDevlieghere]

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.

[tetsuo-cpp]

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.

[tetsuo-cpp]

Rebased onto trunk.

[tetsuo-cpp]

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?

[tetsuo-cpp]

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.

[tetsuo-cpp]

Friendly ping. Could I please get this looked at?

[davide]

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.

[labath]

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

lldb/unittests/debugserver/JSONTest.cpp

17–18

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

[tetsuo-cpp]

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.

[davide]

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

[labath]

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

[tetsuo-cpp]

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.

lldb/tools/debugserver/source/JSON.h

public:
 
   int GetEscapedChar(bool &was_escaped);
 
   Token GetToken(std::string &value);
 
   JSONValue::SP ParseJSONValue();
 
 protected:
+  JSONValue::SP ParseJSONValue(const std::string &value, const Token &token);
+
   JSONValue::SP ParseJSONObject();
 
   JSONValue::SP ParseJSONArray();
 };
 
 #endif // utility_JSON_h_

lldb/tools/debugserver/source/JSON.cpp

JSONValue::SP JSONParser::ParseJSONArray() {
   // The "JSONParser::Token::ObjectStart" token should have already been
   // consumed
   // by the time this function is called
   std::unique_ptr<JSONArray> array_up(new JSONArray());
 
   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) {
       return JSONValue::SP(array_up.release());
     } else {
       break;
     }
   }
   return JSONValue::SP();
 }
 
 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();
 
   case JSONParser::Token::ArrayStart:
     return ParseJSONArray();
 
   case JSONParser::Token::Integer: {

lldb/unittests/debugserver/CMakeLists.txt

 # Note: debugserver is a Darwin-only implementation of a remote debugging
 # server. It is not intended to be used on other platforms. The tests are here
 # because using the LLDB Host API is convienent and allows testing of both parts
 # of the debugserver communication path. If you are looking for a non-darwin
 # remote debugging server, please use lldb-server.
 
 include_directories(${LLDB_SOURCE_DIR}/tools/debugserver/source
                     ${LLDB_SOURCE_DIR}/tools/debugserver/source/MacOSX)
 
 add_lldb_unittest(debugserverTests
+  JSONTest.cpp
   RNBSocketTest.cpp
   debugserver_LogCallback.cpp
 
   LINK_LIBS
     lldbDebugserverCommon
     lldbHost
   LINK_COMPONENTS
     Support
   )
 
 if(IOS)
   set_property(TARGET debugserverTests APPEND PROPERTY COMPILE_DEFINITIONS
       WITH_LOCKDOWN
       WITH_FBS
       WITH_BKS
       )
-  
   add_lldb_unittest(debugserverNonUITests
+    JSONTest.cpp
     RNBSocketTest.cpp
     debugserver_LogCallback.cpp
 
     LINK_LIBS
       lldbDebugserverCommon_NonUI
       lldbHost
     LINK_COMPONENTS
       Support
     )
 endif()

lldb/unittests/debugserver/JSONTest.cpp

+//===-- 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 <typename T>
+void TestJSON(JSONValue *json_val, const std::function<void(T &)> &test_func) {
+  ASSERT_THAT(json_val, testing::NotNull());
+  ASSERT_TRUE(T::classof(json_val));
+  test_func(static_cast<T &>(*json_val));
+}
+
+JSONValue::SP ParseJSON(const char *json_string) {
+  return JSONParser(json_string).ParseJSONValue();
+}
+
+template <typename T>
+void ParseAndTestJSON(
+    const char *json_string,
+    const std::function<void(T &)> &test_func = [](T &) {}) {
+  auto json_val = ParseJSON(json_string);
+  TestJSON<T>(json_val.get(), test_func);
+}
+
+TEST(JSON, Parse) {
+  ParseAndTestJSON<JSONString>("\"foo\"", [](JSONString &string_val) {
+    EXPECT_EQ(string_val.GetData(), "foo");
+  });
+  EXPECT_THAT(ParseJSON("\"foo"), testing::IsNull());
+  ParseAndTestJSON<JSONNumber>("3", [](JSONNumber &number_val) {
+    EXPECT_EQ(number_val.GetAsSigned(), 3);
+    EXPECT_EQ(number_val.GetAsUnsigned(), 3u);
+    EXPECT_EQ(number_val.GetAsDouble(), 3.0);
+  });
+  ParseAndTestJSON<JSONNumber>("-5", [](JSONNumber &number_val) {
+    EXPECT_EQ(number_val.GetAsSigned(), -5);
+    EXPECT_EQ(number_val.GetAsDouble(), -5.0);
+  });
+  ParseAndTestJSON<JSONNumber>("-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<JSONTrue>("true");
+  ParseAndTestJSON<JSONFalse>("false");
+  ParseAndTestJSON<JSONNull>("null");
+  ParseAndTestJSON<JSONObject>(
+      "{ \"key1\": 4, \"key2\": \"foobar\" }", [](JSONObject &obj_val) {
+        TestJSON<JSONNumber>(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<JSONString>(obj_val.GetObject("key2").get(),
+                             [](JSONString &string_val) {
+                               EXPECT_EQ(string_val.GetData(), "foobar");
+                             });
+      });
+  ParseAndTestJSON<JSONArray>("[1, \"bar\", 3.14]", [](JSONArray &array_val) {
+    EXPECT_EQ(array_val.GetNumElements(), 3u);
+    TestJSON<JSONNumber>(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<JSONString>(
+        array_val.GetObject(1).get(),
+        [](JSONString &string_val) { EXPECT_EQ(string_val.GetData(), "bar"); });
+    TestJSON<JSONNumber>(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>("[]", [](JSONArray &array_val) {
+    EXPECT_EQ(array_val.GetNumElements(), 0u);
+  });
+}