[Lldb-commits] [lldb] [LLDB] Fix type formatting empty c-strings (PR #68924)

[Walter Erquinigo via lldb-commits]

https://github.com/walter-erquinigo updated https://github.com/llvm/llvm-project/pull/68924

>From eaf314f813202fd71b884f3cd2e87db6cfd97e96 Mon Sep 17 00:00:00 2001
From: walter erquinigo <walter at modular.com>
Date: Thu, 12 Oct 2023 15:58:19 -0400
Subject: [PATCH 1/3] [LLDB] Fix type formatting empty c-strings

The type formatter code is effectively considering empty strings as read errors, which is wrong. The fix is very simple. We should rely on the error object and stop checking the size. I also added a test.
---
 lldb/source/DataFormatters/TypeFormat.cpp       |  6 +++---
 .../builtin-formats/TestBuiltinFormats.py       | 17 ++++++++++++++++-
 .../data-formatter/builtin-formats/main.cpp     |  2 ++
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/lldb/source/DataFormatters/TypeFormat.cpp b/lldb/source/DataFormatters/TypeFormat.cpp
index 5ee89fc0d5eb319..126240aeca65e43 100644
--- a/lldb/source/DataFormatters/TypeFormat.cpp
+++ b/lldb/source/DataFormatters/TypeFormat.cpp
@@ -81,9 +81,9 @@ bool TypeFormatImpl_Format::FormatObject(ValueObject *valobj,
               WritableDataBufferSP buffer_sp(
                   new DataBufferHeap(max_len + 1, 0));
               Address address(valobj->GetPointerValue());
-              if (target_sp->ReadCStringFromMemory(
-                      address, (char *)buffer_sp->GetBytes(), max_len, error) &&
-                  error.Success())
+              target_sp->ReadCStringFromMemory(
+                  address, (char *)buffer_sp->GetBytes(), max_len, error);
+              if (error.Success())
                 data.SetData(buffer_sp);
             }
           }
diff --git a/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py b/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py
index aa768c158b5b5ff..5c49141c176303d 100644
--- a/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py
+++ b/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py
@@ -3,9 +3,9 @@
 """
 
 import lldb
+from lldbsuite.test import lldbutil
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
-from lldbsuite.test import lldbutil
 
 
 class TestCase(TestBase):
@@ -19,6 +19,21 @@ def getFormatted(self, format, expr):
         self.assertTrue(result.Succeeded(), result.GetError())
         return result.GetOutput()
 
+    @no_debug_info_test
+    @skipIfWindows
+    def testAllPlatforms(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(
+            self, "// break here", lldb.SBFileSpec("main.cpp")
+        )
+        # We can dump correctly non char* c-strings with explicit formatting.
+        self.assertIn(' = ""', self.getFormatted("c-string", "void_empty_cstring"));
+        self.assertIn(' = ""', self.getFormatted("c-string", "empty_cstring"));
+
+
+    # TODO: Move as many asserts as possible within this function to `testAllPlatforms`.
+    # Currently `arm` is being skipped even though many asserts would effectively
+    # pass.
     @no_debug_info_test
     @skipIfWindows
     # uint128_t not available on arm.
diff --git a/lldb/test/API/functionalities/data-formatter/builtin-formats/main.cpp b/lldb/test/API/functionalities/data-formatter/builtin-formats/main.cpp
index 58b8116dfa1ec9b..573c111306c14de 100644
--- a/lldb/test/API/functionalities/data-formatter/builtin-formats/main.cpp
+++ b/lldb/test/API/functionalities/data-formatter/builtin-formats/main.cpp
@@ -1,8 +1,10 @@
 #include <cstdint>
 
 const char cstring[15] = " \033\a\b\f\n\r\t\vaA09\0";
+const char *empty_cstring = "";
 
 int main() {
   int use = *cstring;
+  void *void_empty_cstring = (void *)empty_cstring;
   return use; // break here
 }

>From c2a603f7a47077c75e1a4844510d0cfda0b78332 Mon Sep 17 00:00:00 2001
From: Walter Erquinigo <a20012251 at gmail.com>
Date: Thu, 12 Oct 2023 16:13:26 -0400
Subject: [PATCH 2/3] Update TestBuiltinFormats.py

---
 .../data-formatter/builtin-formats/TestBuiltinFormats.py      | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py b/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py
index 5c49141c176303d..aa8f63e8a179f3f 100644
--- a/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py
+++ b/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py
@@ -27,8 +27,8 @@ def testAllPlatforms(self):
             self, "// break here", lldb.SBFileSpec("main.cpp")
         )
         # We can dump correctly non char* c-strings with explicit formatting.
-        self.assertIn(' = ""', self.getFormatted("c-string", "void_empty_cstring"));
-        self.assertIn(' = ""', self.getFormatted("c-string", "empty_cstring"));
+        self.assertIn(' = ""', self.getFormatted("c-string", "void_empty_cstring"))
+        self.assertIn(' = ""', self.getFormatted("c-string", "empty_cstring"))
 
 
     # TODO: Move as many asserts as possible within this function to `testAllPlatforms`.

>From 82ed20cb57c08c1448d79ae949f58b8738323fe1 Mon Sep 17 00:00:00 2001
From: Walter Erquinigo <a20012251 at gmail.com>
Date: Thu, 12 Oct 2023 16:27:55 -0400
Subject: [PATCH 3/3] Update TestBuiltinFormats.py

---
 .../data-formatter/builtin-formats/TestBuiltinFormats.py         | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py b/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py
index aa8f63e8a179f3f..4e0f14d039a743f 100644
--- a/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py
+++ b/lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py
@@ -30,7 +30,6 @@ def testAllPlatforms(self):
         self.assertIn(' = ""', self.getFormatted("c-string", "void_empty_cstring"))
         self.assertIn(' = ""', self.getFormatted("c-string", "empty_cstring"))
 
-
     # TODO: Move as many asserts as possible within this function to `testAllPlatforms`.
     # Currently `arm` is being skipped even though many asserts would effectively
     # pass.