[lldb] Correctly escape newlines and backslashes in the JSON serializer

[kubamracek]

Summary

JSON serializer fails to escape newlines and backslashes. Let's fix that.

[joerg]

Please see the discussion in D31992. This patch seems to go in the wrong direction.

[kubamracek]

I see. The approach to escape as little as possible can be valid, I guess.

In any case, this patch seems to do exactly this. We must escape backslashes, otherwise we’ll produce invalid output: imagine a backslash at the end of the string. Same about newlines: there are parsers that will refuse unescaped newlines.

[joerg]

My suggestion would be to just use the YAML writer for now and leave a comment to make it clear that this is really JSON only.

[beanz]

In D34322#783532, @joerg wrote:

My suggestion would be to just use the YAML writer for now and leave a comment to make it clear that this is really JSON only.

Our YAML parser can parse JSON because YAML is a superset of JSON, but I don't believe the YAML writer supports writing out inline-style YAML which would be compatible with a JSON parser. That might make the LLVM YAML writer unsuitable for @kubamracek's use case.

[joerg]

I don't disagree with you, but please see the referenced review for further details.

I do not want the amount of adhoc JSON encoders to grow further. The YAML encoder works fine for most of the things, but it is easier to review calls to it than it is to find other instances of adhoc JSON encoding.

[beanz]

I get your perspective, but holding up this relatively small patch that fixes a bug in existing code on an architectural disagreement seems like excessive bike shedding. If we assume that JSON is required for the use case would you have Kuba write a full JSON parser in LLVM to satisfy your distaste over the fact that we have multiple JSON parsers already? That seems like an unreasonable request just to fix a few small bugs in existing code.

Fundamentally I think we probably should migrate toward having a single JSON parser implemented in LLVMSupport, but I don't think this is the patch review to pick that fight over.

[joerg]

Not parsers, encoders. Note that the escaping is not correct for control characters other than \n.

[jasonmolenda]

LGTM.

lldb.xcodeproj/project.pbxproj

/* Begin PBXBuildFile section */
     6D95DC011B9DC057000E318A /* HashedNameToDIE.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6D95DBFE1B9DC057000E318A /* HashedNameToDIE.cpp */; };
     6D95DC021B9DC057000E318A /* SymbolFileDWARFDwo.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6D95DBFF1B9DC057000E318A /* SymbolFileDWARFDwo.cpp */; };
     6D99A3631BBC2F3200979793 /* ArmUnwindInfo.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6D99A3621BBC2F3200979793 /* ArmUnwindInfo.cpp */; };
     6D9AB3DD1BB2B74E003F2289 /* TypeMap.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6D9AB3DC1BB2B74E003F2289 /* TypeMap.cpp */; };
     6DEC6F391BD66D750091ABA6 /* TaskPool.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 6DEC6F381BD66D750091ABA6 /* TaskPool.cpp */; };
     8C26C4261C3EA5F90031DF7C /* ThreadSanitizerRuntime.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8C26C4241C3EA4340031DF7C /* ThreadSanitizerRuntime.cpp */; };
     8C2D6A53197A1EAF006989C9 /* MemoryHistory.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8C2D6A52197A1EAF006989C9 /* MemoryHistory.cpp */; };
     8C2D6A5E197A250F006989C9 /* MemoryHistoryASan.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8C2D6A5A197A1FDC006989C9 /* MemoryHistoryASan.cpp */; };
+    8C3BD9A01EF5D1FF0016C343 /* JSONTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8C3BD99F1EF5D1B50016C343 /* JSONTest.cpp */; };
     8C3BD9961EF45DA50016C343 /* MainThreadCheckerRuntime.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8C3BD9951EF45D9B0016C343 /* MainThreadCheckerRuntime.cpp */; };
     8CCB017E19BA28A80009FD44 /* ThreadCollection.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8CCB017A19BA283D0009FD44 /* ThreadCollection.cpp */; };
     8CCB018219BA4E270009FD44 /* SBThreadCollection.h in Headers */ = {isa = PBXBuildFile; fileRef = 8CCB018119BA4E210009FD44 /* SBThreadCollection.h */; settings = {ATTRIBUTES = (Public, ); }; };
     8CCB018319BA51BF0009FD44 /* SBThreadCollection.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8CCB017F19BA4DD00009FD44 /* SBThreadCollection.cpp */; };
     8CF02AE919DCC01900B14BE0 /* InstrumentationRuntime.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8CF02ADF19DCBF3B00B14BE0 /* InstrumentationRuntime.cpp */; };
     8CF02AEA19DCC02100B14BE0 /* AddressSanitizerRuntime.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8CF02AE519DCBF8400B14BE0 /* AddressSanitizerRuntime.cpp */; };
     8CF02AEF19DD16B100B14BE0 /* InstrumentationRuntimeStopInfo.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8CF02AED19DD15CF00B14BE0 /* InstrumentationRuntimeStopInfo.cpp */; };
     9404957A1BEC497E00926025 /* NSError.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 940495781BEC497E00926025 /* NSError.cpp */; };

     6DEC6F381BD66D750091ABA6 /* TaskPool.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = TaskPool.cpp; path = source/Utility/TaskPool.cpp; sourceTree = "<group>"; };
     6DEC6F3A1BD66D950091ABA6 /* TaskPool.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = TaskPool.h; path = include/lldb/Utility/TaskPool.h; sourceTree = "<group>"; };
     8C26C4241C3EA4340031DF7C /* ThreadSanitizerRuntime.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; name = ThreadSanitizerRuntime.cpp; path = ThreadSanitizer/ThreadSanitizerRuntime.cpp; sourceTree = "<group>"; };
     8C26C4251C3EA4340031DF7C /* ThreadSanitizerRuntime.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ThreadSanitizerRuntime.h; path = ThreadSanitizer/ThreadSanitizerRuntime.h; sourceTree = "<group>"; };
     8C2D6A52197A1EAF006989C9 /* MemoryHistory.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = MemoryHistory.cpp; path = source/Target/MemoryHistory.cpp; sourceTree = "<group>"; };
     8C2D6A54197A1EBE006989C9 /* MemoryHistory.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = MemoryHistory.h; path = include/lldb/Target/MemoryHistory.h; sourceTree = "<group>"; };
     8C2D6A5A197A1FDC006989C9 /* MemoryHistoryASan.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = MemoryHistoryASan.cpp; sourceTree = "<group>"; };
     8C2D6A5B197A1FDC006989C9 /* MemoryHistoryASan.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MemoryHistoryASan.h; sourceTree = "<group>"; };
+    8C3BD99F1EF5D1B50016C343 /* JSONTest.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = JSONTest.cpp; sourceTree = "<group>"; };
     8C3BD9931EF45D9B0016C343 /* MainThreadCheckerRuntime.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = MainThreadCheckerRuntime.h; sourceTree = "<group>"; };
     8C3BD9951EF45D9B0016C343 /* MainThreadCheckerRuntime.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = MainThreadCheckerRuntime.cpp; sourceTree = "<group>"; };
     8CCB017A19BA283D0009FD44 /* ThreadCollection.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = ThreadCollection.cpp; path = source/Target/ThreadCollection.cpp; sourceTree = "<group>"; };
     8CCB017C19BA289B0009FD44 /* ThreadCollection.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ThreadCollection.h; path = include/lldb/Target/ThreadCollection.h; sourceTree = "<group>"; };
     8CCB017F19BA4DD00009FD44 /* SBThreadCollection.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; name = SBThreadCollection.cpp; path = source/API/SBThreadCollection.cpp; sourceTree = "<group>"; };
     8CCB018119BA4E210009FD44 /* SBThreadCollection.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SBThreadCollection.h; path = include/lldb/API/SBThreadCollection.h; sourceTree = "<group>"; };
     8CCB018419BA54930009FD44 /* SBThreadCollection.i */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.c.preprocessed; path = SBThreadCollection.i; sourceTree = "<group>"; };
     8CF02ADF19DCBF3B00B14BE0 /* InstrumentationRuntime.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; name = InstrumentationRuntime.cpp; path = source/Target/InstrumentationRuntime.cpp; sourceTree = "<group>"; };
/* Begin PBXGroup section */
     };
     2321F9421BDD343A00BA9A93 /* Utility */ = {
       isa = PBXGroup;
       children = (
         2321F9431BDD346100BA9A93 /* CMakeLists.txt */,
         23CB15041D66CD9200EDDDE1 /* Inputs */,
         2321F9441BDD346100BA9A93 /* StringExtractorTest.cpp */,
         2321F9451BDD346100BA9A93 /* TaskPoolTest.cpp */,
+        8C3BD99F1EF5D1B50016C343 /* JSONTest.cpp */,
         2321F9461BDD346100BA9A93 /* UriParserTest.cpp */,
       );
       path = Utility;
       sourceTree = "<group>";
     };
     2321F94B1BDD35D500BA9A93 /* Python */ = {
       isa = PBXGroup;
       children = (
/* Begin PBXSourcesBuildPhase section */
     23CB15321D66DA9300EDDDE1 /* Sources */ = {
       isa = PBXSourcesBuildPhase;
       buildActionMask = 2147483647;
       files = (
         23CB15331D66DA9300EDDDE1 /* GoParserTest.cpp in Sources */,
         23CB15341D66DA9300EDDDE1 /* CPlusPlusLanguageTest.cpp in Sources */,
         AFAFD80A1E57E1B90017A14F /* ModuleCacheTest.cpp in Sources */,
         23CB15351D66DA9300EDDDE1 /* UriParserTest.cpp in Sources */,
+        8C3BD9A01EF5D1FF0016C343 /* JSONTest.cpp in Sources */,
         23CB15361D66DA9300EDDDE1 /* FileSpecTest.cpp in Sources */,
         23E2E5251D90373D006F38BB /* ArchSpecTest.cpp in Sources */,
         AF248A4D1DA71C77000B814D /* TestArm64InstEmulation.cpp in Sources */,
         23CB15371D66DA9300EDDDE1 /* PythonTestSuite.cpp in Sources */,
         23E2E5321D903832006F38BB /* BreakpointIDTest.cpp in Sources */,
         23CB15381D66DA9300EDDDE1 /* PythonExceptionStateTests.cpp in Sources */,
         23CB15391D66DA9300EDDDE1 /* DataExtractorTest.cpp in Sources */,
         23CB153A1D66DA9300EDDDE1 /* GDBRemoteClientBaseTest.cpp in Sources */,

source/Utility/JSON.cpp

 #include <inttypes.h> // for PRIu64, PRId64
 #include <limits.h>
 #include <stddef.h> // for size_t
 #include <utility>  // for pair
 
 using namespace lldb_private;
 
 std::string JSONString::json_string_quote_metachars(const std::string &s) {
-  if (s.find('"') == std::string::npos)
+  if (s.find_first_of("\\\n\"") == std::string::npos)
     return s;
 
   std::string output;
   const size_t s_size = s.size();
   const char *s_chars = s.c_str();
   for (size_t i = 0; i < s_size; i++) {
     unsigned char ch = *(s_chars + i);
-    if (ch == '"') {
+    if (ch == '"' || ch == '\\' || ch == '\n') {
       output.push_back('\\');
+      if (ch == '\n') ch = 'n';
     }
     output.push_back(ch);
   }
   return output;
 }
 
 JSONString::JSONString() : JSONValue(JSONValue::Kind::String), m_data() {}
 

unittests/Utility/CMakeLists.txt

 add_subdirectory(Mocks)
 
 add_lldb_unittest(UtilityTests
   ConstStringTest.cpp
   LogTest.cpp
   NameMatchesTest.cpp
   StatusTest.cpp
   StringExtractorTest.cpp
   TaskPoolTest.cpp
   TildeExpressionResolverTest.cpp
   TimeoutTest.cpp
+  JSONTest.cpp
   UriParserTest.cpp
   VASprintfTest.cpp
 
   LINK_LIBS
       lldbUtility
       lldbUtilityMocks
   LINK_COMPONENTS
     Support
   )

unittests/Utility/JSONTest.cpp

+#include "gtest/gtest.h"
+
+#include "lldb/Core/StreamString.h"
+#include "lldb/Utility/JSON.h"
+
+using namespace lldb_private;
+
+TEST(JSONTest, Dictionary) {
+  JSONObject o;
+  o.SetObject("key", std::make_shared<JSONString>("value"));
+
+  StreamString stream;
+  o.Write(stream);
+
+  ASSERT_EQ(stream.GetString(), R"({"key":"value"})");
+}
+
+TEST(JSONTest, Newlines) {
+  JSONObject o;
+  o.SetObject("key", std::make_shared<JSONString>("hello\nworld"));
+
+  StreamString stream;
+  o.Write(stream);
+
+  ASSERT_EQ(stream.GetString(), R"({"key":"hello\nworld"})");
+}