[LLDB] Fix type formatting empty c-strings

[walter-erquinigo]

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.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Walter Erquinigo (walter-erquinigo)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/68924.diff

3 Files Affected:

• (modified) lldb/source/DataFormatters/TypeFormat.cpp (+3-3)
• (modified) lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py (+16-1)
• (modified) lldb/test/API/functionalities/data-formatter/builtin-formats/main.cpp (+2)

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
 }

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[clayborg]

Do we want to do this via the API? Something like:

(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.cpp"))
frame = thread.GetFrameAtIndex(0)
v = frame.FindVariable('void_empty_cstring')
self.assertEq(v.GetSummary(), '')
v = frame.FindVariable('empty_cstring')
self.assertEq(v.GetSummary(), '')

[walter-erquinigo]

We could, but I was just following the pattern that is already being used in that test file. I don't think it's worth deviating from that.

[bulbazord]

I'm alright with this, though I'm a little concerned that the return value of ReadCStringFromMemory and error.Success() don't give you the same result. I'm also somewhat sad that we have to check both of those to know if we succeeded, but I know that's not the point of this PR.

[walter-erquinigo]

@bulbazord , ReadCStringFromMemory returns the length of the string, which in the case of empty strings is 0!
So, this code was wrong since the beginning. The only thing to check is error.Success() and not the size.