[lldb/Utility] Add GetDescription(Stream&) to StructureData::*

[mib]

Summary

This patch improves the StructuredData classes to provide a
GetDescription(lldb_private::Stream&) affordance.

This is very convenient compared to the Dump method because this try to
pretty print the structure instead of just serializing it into a JSON.

This patch also updates some parts of lldb (i.e. extended crash info) to
use this new affordance instead of StructuredData::Dump.

Signed-off-by: Med Ismail Bennani <[e-mail]>

[JDevlieghere]

The change looks good but this needs unit tests.

[mib]

Address @JDevlieghere comment:

• Add unittest

[mib]

Add test file

[JDevlieghere]

The change looks good but I'd like to see a few more unit tests to cover edge cases (empty array, dict, etc),

lldb/unittests/Utility/StructuredDataTest.cpp

40–43

Maybe use a string literal to make it clear what the formatted text looks like?

[mib]

Address @JDevlieghere comments:

• Fix indentation bug on nested structures
• Add tests
• Use string literals for expected string in tests

[mib]

lldb/include/lldb/Utility/StructuredData.h

584

@JDevlieghere Not sure if it would be worth to inline this

[JDevlieghere]

LGMT with the missing periods fixed.

lldb/source/Utility/StructuredData.cpp

208

Comments should include punctuation.

[shafik]

lldb/unittests/Utility/StructuredDataTest.cpp

41

When doing a build I am seeing:

warning: comparison of integers of different signs: 'const int' and 'const unsigned long' [-Wsign-compare]

[mib]

lldb/unittests/Utility/StructuredDataTest.cpp

41

I'll fix that! Thanks @shafik :) !

lldb/include/lldb/Core/StructuredDataImpl.h

Status GetDescription(Stream &stream) const {
     lldb::StructuredDataPluginSP plugin_sp = m_plugin_wp.lock();
 
     // If there's no plugin, call underlying data's dump method:
     if (!plugin_sp) {
       if (!m_data_sp) {
         error.SetErrorString("No data to describe.");
         return error;
       }
-      m_data_sp->Dump(stream, true);
+      m_data_sp->GetDescription(stream);
       return error;
     }
     // Get the data's description.
     return plugin_sp->GetDescription(m_data_sp, stream);
   }
 
   StructuredData::ObjectSP GetObjectSP() { return m_data_sp; }
 

lldb/include/lldb/Utility/StructuredData.h

public:
 
     virtual void Serialize(llvm::json::OStream &s) const = 0;
 
     void Dump(lldb_private::Stream &s, bool pretty_print = true) const {
       llvm::json::OStream jso(s.AsRawOstream(), pretty_print ? 2 : 0);
       Serialize(jso);
     }
 
+    virtual void GetDescription(lldb_private::Stream &s) const {
+      s.IndentMore();
+      Dump(s, false);
+      s.IndentLess();
+    }
+
   private:
     lldb::StructuredDataType m_type;
   };
 
   class Array : public Object {
   public:
     Array() : Object(lldb::eStructuredDataTypeArray) {}
 
public:
     }
 
     void Push(const ObjectSP &item) { m_items.push_back(item); }
 
     void AddItem(const ObjectSP &item) { m_items.push_back(item); }
 
     void Serialize(llvm::json::OStream &s) const override;
 
+    void GetDescription(lldb_private::Stream &s) const override;
+
   protected:
     typedef std::vector<ObjectSP> collection;
     collection m_items;
   };
 
   class Integer : public Object {
   public:
     Integer(uint64_t i = 0)
         : Object(lldb::eStructuredDataTypeInteger), m_value(i) {}
 
     ~Integer() override = default;
 
     void SetValue(uint64_t value) { m_value = value; }
 
     uint64_t GetValue() { return m_value; }
 
     void Serialize(llvm::json::OStream &s) const override;
 
+    void GetDescription(lldb_private::Stream &s) const override;
+
   protected:
     uint64_t m_value;
   };
 
   class Float : public Object {
   public:
     Float(double d = 0.0)
         : Object(lldb::eStructuredDataTypeFloat), m_value(d) {}
 
     ~Float() override = default;
 
     void SetValue(double value) { m_value = value; }
 
     double GetValue() { return m_value; }
 
     void Serialize(llvm::json::OStream &s) const override;
 
+    void GetDescription(lldb_private::Stream &s) const override;
+
   protected:
     double m_value;
   };
 
   class Boolean : public Object {
   public:
     Boolean(bool b = false)
         : Object(lldb::eStructuredDataTypeBoolean), m_value(b) {}
 
     ~Boolean() override = default;
 
     void SetValue(bool value) { m_value = value; }
 
     bool GetValue() { return m_value; }
 
     void Serialize(llvm::json::OStream &s) const override;
 
+    void GetDescription(lldb_private::Stream &s) const override;
+
   protected:
     bool m_value;
   };
 
   class String : public Object {
   public:
     String() : Object(lldb::eStructuredDataTypeString) {}
     explicit String(llvm::StringRef S)
         : Object(lldb::eStructuredDataTypeString), m_value(S) {}
 
     void SetValue(llvm::StringRef S) { m_value = std::string(S); }
 
     llvm::StringRef GetValue() { return m_value; }
 
     void Serialize(llvm::json::OStream &s) const override;
 
+    void GetDescription(lldb_private::Stream &s) const override;
+
   protected:
     std::string m_value;
   };
 
   class Dictionary : public Object {
   public:
     Dictionary() : Object(lldb::eStructuredDataTypeDictionary) {}
 
public:
     }
 
     void AddBooleanItem(llvm::StringRef key, bool value) {
       AddItem(key, std::make_shared<Boolean>(value));
     }
 
     void Serialize(llvm::json::OStream &s) const override;
 
+    void GetDescription(lldb_private::Stream &s) const override;
+
   protected:
     typedef std::map<ConstString, ObjectSP> collection;
     collection m_dict;
   };
 
   class Null : public Object {
   public:
     Null() : Object(lldb::eStructuredDataTypeNull) {}
 
     ~Null() override = default;
 
     bool IsValid() const override { return false; }
 
     void Serialize(llvm::json::OStream &s) const override;
+
+    void GetDescription(lldb_private::Stream &s) const override;
   };
 
   class Generic : public Object {
   public:
     explicit Generic(void *object = nullptr)
         : Object(lldb::eStructuredDataTypeGeneric), m_object(object) {}
 
     void SetValue(void *value) { m_object = value; }
 
     void *GetValue() const { return m_object; }
 
     bool IsValid() const override { return m_object != nullptr; }
 
     void Serialize(llvm::json::OStream &s) const override;
 
+    void GetDescription(lldb_private::Stream &s) const override;
+
   private:
     void *m_object;
   };
 
   static ObjectSP ParseJSON(const std::string &json_text);
   static ObjectSP ParseJSONFromFile(const FileSpec &file, Status &error);
+  static bool IsRecordType(const ObjectSP object_sp);
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_STRUCTUREDDATA_H

lldb/source/Commands/CommandObjectProcess.cpp

if (m_options.m_verbose) {
       if (!expected_crash_info) {
         result.AppendError(llvm::toString(expected_crash_info.takeError()));
         return result.Succeeded();
       }
 
       StructuredData::DictionarySP crash_info_sp = *expected_crash_info;
 
       if (crash_info_sp) {
+        strm.EOL();
         strm.PutCString("Extended Crash Information:\n");
-        crash_info_sp->Dump(strm);
+        crash_info_sp->GetDescription(strm);
       }
     }
 
     return result.Succeeded();
   }
 
 private:
   CommandOptions m_options;

lldb/source/Utility/StructuredData.cpp

StructuredData::ParseJSONFromFile(const FileSpec &input_spec, Status &error) {
   llvm::Expected<json::Value> value =
       json::parse(buffer_or_error.get()->getBuffer().str());
   if (value)
     return ParseJSONValue(*value);
   error.SetErrorString(toString(value.takeError()));
   return StructuredData::ObjectSP();
 }
 
+bool StructuredData::IsRecordType(const ObjectSP object_sp) {
+  return object_sp->GetType() == lldb::eStructuredDataTypeArray ||
+         object_sp->GetType() == lldb::eStructuredDataTypeDictionary;
+}
+
 static StructuredData::ObjectSP ParseJSONValue(json::Value &value) {
   if (json::Object *O = value.getAsObject())
     return ParseJSONObject(O);
 
   if (json::Array *A = value.getAsArray())
     return ParseJSONArray(A);
 
   if (auto s = value.getAsString())

 
 void StructuredData::Null::Serialize(json::OStream &s) const {
   s.value(nullptr);
 }
 
 void StructuredData::Generic::Serialize(json::OStream &s) const {
   s.value(llvm::formatv("{0:X}", m_object));
 }
+
+void StructuredData::Integer::GetDescription(lldb_private::Stream &s) const {
+  s.Printf("%" PRId64, static_cast<int64_t>(m_value));
+}
+
+void StructuredData::Float::GetDescription(lldb_private::Stream &s) const {
+  s.Printf("%f", m_value);
+}
+
+void StructuredData::Boolean::GetDescription(lldb_private::Stream &s) const {
+  s.Printf(m_value ? "True" : "False");
+}
+
+void StructuredData::String::GetDescription(lldb_private::Stream &s) const {
+  s.Printf("%s", m_value.empty() ? "\"\"" : m_value.c_str());
+}
+
+void StructuredData::Array::GetDescription(lldb_private::Stream &s) const {
+  size_t index = 0;
+  size_t indentation_level = s.GetIndentLevel();
+  for (const auto &item_sp : m_items) {
+    // Sanitize.
+    if (!item_sp)
+      continue;
+
+    // Reset original indentation level.
+    s.SetIndentLevel(indentation_level);
+    s.Indent();
+
+    // Print key
+    s.Printf("[%zu]:", index++);
+
+    // Return to new line and increase indentation if value is record type.
+    // Otherwise add spacing.
+    bool should_indent = IsRecordType(item_sp);
+    if (should_indent) {
+      s.EOL();
+      s.IndentMore();
+    } else {
+      s.PutChar(' ');
+    }
+
+    // Print value and new line if now last pair.
+    item_sp->GetDescription(s);
+    if (item_sp != *(--m_items.end()))
+      s.EOL();
+
+    // Reset indentation level if it was incremented previously.
+    if (should_indent)
+      s.IndentLess();
+  }
+}
+
+void StructuredData::Dictionary::GetDescription(lldb_private::Stream &s) const {
+  size_t indentation_level = s.GetIndentLevel();
+  for (const auto &pair : m_dict) {
+    // Sanitize.
+    if (pair.first.IsNull() || pair.first.IsEmpty() || !pair.second)
+      continue;
+
+    // Reset original indentation level.
+    s.SetIndentLevel(indentation_level);
+    s.Indent();
+
+    // Print key.
+    s.Printf("%s:", pair.first.AsCString());
+
+    // Return to new line and increase indentation if value is record type.
+    // Otherwise add spacing.
+    bool should_indent = IsRecordType(pair.second);
+    if (should_indent) {
+      s.EOL();
+      s.IndentMore();
+    } else {
+      s.PutChar(' ');
+    }
+
+    // Print value and new line if now last pair.
+    pair.second->GetDescription(s);
+    if (pair != *(--m_dict.end()))
+      s.EOL();
+
+    // Reset indentation level if it was incremented previously.
+    if (should_indent)
+      s.IndentLess();
+  }
+}
+
+void StructuredData::Null::GetDescription(lldb_private::Stream &s) const {
+  s.Printf("NULL");
+}
+
+void StructuredData::Generic::GetDescription(lldb_private::Stream &s) const {
+  s.Printf("%p", m_object);
+}

lldb/test/API/functionalities/process_crash_info/TestProcessCrashInfo.py

def test_cli(self):
         exe = self.getBuildArtifact("a.out")
         self.expect("file " + exe,
                     patterns=["Current executable set to .*a.out"])
 
         self.expect('process launch',
                     patterns=["Process .* launched: .*a.out"])
 
         self.expect('process status --verbose',
-                    patterns=["\"message\".*pointer being freed was not allocated"])
+                    patterns=["Extended Crash Information",
+                              "crash-info annotations",
+                              "pointer being freed was not allocated"])
 
 
     @skipIfAsan # The test process intentionally hits a memory bug.
     @skipUnlessDarwin
     def test_api(self):
         """Test that lldb can fetch a crashed process' extended crash information
         dictionary from the api properly."""
         self.build()

lldb/unittests/Utility/CMakeLists.txt

add_lldb_unittest(UtilityTests
   LINK_LIBS
       lldbUtility
       lldbUtilityHelpers
       LLVMTestingSupport
   LINK_COMPONENTS
     Support
   )
 
-add_unittest_inputs(UtilityTests
+set(test_inputs
   StructuredData-basic.json
+  StructuredData-nested.json
+  StructuredData-full.json
   )
+
+add_unittest_inputs(UtilityTests "${test_inputs}")

lldb/unittests/Utility/Inputs/StructuredData-full.json

+{
+  "Array": [
+    3.14,
+    {
+      "key": "val"
+    }
+  ],
+  "Dictionary": {
+    "FalseBool": false
+  },
+  "Integer": 1,
+  "Null": null,
+  "String": "value",
+  "TrueBool": true
+}

lldb/unittests/Utility/Inputs/StructuredData-nested.json

+{
+  "my_dict": [
+    {
+      "three": 3,
+      "two": 2
+    },
+    {
+      "four": {
+        "val": 4
+      }
+    },
+    1
+  ]
+}

lldb/unittests/Utility/StructuredDataTest.cpp

TEST(StructuredDataTest, StringDump) {
   for (auto P : TestCases) {
     StreamString S;
     const bool pretty_print = false;
     StructuredData::String(P.first).Dump(S, pretty_print);
     EXPECT_EQ(P.second, S.GetString());
   }
 }
 
+TEST(StructuredDataTest, GetDescriptionEmpty) {
+  Status status;
+  auto object_sp = StructuredData::ParseJSON("{}");
+  ASSERT_NE(nullptr, object_sp);
+
+  StreamString S;
+  object_sp->GetDescription(S);
+  EXPECT_EQ(0, S.GetSize());
+}
+
+TEST(StructuredDataTest, GetDescriptionBasic) {
+  Status status;
+  std::string input = GetInputFilePath("StructuredData-basic.json");
+  auto object_sp = StructuredData::ParseJSONFromFile(FileSpec(input), status);
+  ASSERT_NE(nullptr, object_sp);
+
+  const std::string expected = "[0]: 1\n"
+                               "[1]: 2\n"
+                               "[2]: 3";
+
+  StreamString S;
+  object_sp->GetDescription(S);
+  EXPECT_EQ(expected, S.GetString());
+}
+
+TEST(StructuredDataTest, GetDescriptionNested) {
+  Status status;
+  std::string input = GetInputFilePath("StructuredData-nested.json");
+  auto object_sp = StructuredData::ParseJSONFromFile(FileSpec(input), status);
+  ASSERT_NE(nullptr, object_sp);
+
+  const std::string expected = "my_dict:\n"
+                               "  [0]:\n"
+                               "    three: 3\n"
+                               "    two: 2\n"
+                               "  [1]:\n"
+                               "    four:\n"
+                               "      val: 4\n"
+                               "  [2]: 1";
+
+  StreamString S;
+  object_sp->GetDescription(S);
+  EXPECT_EQ(expected, S.GetString());
+}
+
+TEST(StructuredDataTest, GetDescriptionFull) {
+  Status status;
+  std::string input = GetInputFilePath("StructuredData-full.json");
+  auto object_sp = StructuredData::ParseJSONFromFile(FileSpec(input), status);
+  ASSERT_NE(nullptr, object_sp);
+
+  const std::string expected = "Array:\n"
+                               "  [0]: 3.140000\n"
+                               "  [1]:\n"
+                               "    key: val\n"
+                               "Dictionary:\n"
+                               "  FalseBool: False\n"
+                               "Integer: 1\n"
+                               "Null: NULL\n"
+                               "String: value\n"
+                               "TrueBool: True";
+
+  StreamString S;
+  object_sp->GetDescription(S);
+  EXPECT_EQ(expected, S.GetString());
+}
+
 TEST(StructuredDataTest, ParseJSONFromFile) {
   Status status;
   auto object_sp = StructuredData::ParseJSONFromFile(
       FileSpec("non-existing-file.json"), status);
   EXPECT_EQ(nullptr, object_sp);
 
   std::string input = GetInputFilePath("StructuredData-basic.json");
   object_sp = StructuredData::ParseJSONFromFile(FileSpec(input), status);
   ASSERT_NE(nullptr, object_sp);
 
   StreamString S;
   object_sp->Dump(S, false);
   EXPECT_EQ("[1,2,3]", S.GetString());
 }