Skip to content

[LLDB] Fix type formatting empty c-strings #68924

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 13, 2023
Merged

[LLDB] Fix type formatting empty c-strings #68924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eaf314f
[LLDB] Fix type formatting empty c-strings
Oct 12, 2023
c2a603f
Update TestBuiltinFormats.py
walter-erquinigo Oct 12, 2023
82ed20c
Update TestBuiltinFormats.py
walter-erquinigo Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lldb/source/DataFormatters/TypeFormat.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
16 changes: 15 additions & 1 deletion lldb/test/API/functionalities/data-formatter/builtin-formats/TestBuiltinFormats.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -19,6 +19,20 @@ 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"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(), '')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
Expand Down
2 changes: 2 additions & 0 deletions lldb/test/API/functionalities/data-formatter/builtin-formats/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
}