[lldb][DataFormatter] Format libstdc++ unique_ptr like we do libc++ #146909
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Michael137
commented
Jul 3, 2025
| /// formatting the rest of the object. Currently this is the case when the | ||
| /// pointer is a nullptr. | ||
| bool DumpCxxSmartPtrPointerSummary(Stream &stream, ValueObject &ptr, | ||
| const TypeSummaryOptions &options); |
There was a problem hiding this comment.
The return value is a bit awkward. Happy to consider alternatives.
There was a problem hiding this comment.
Is it actually needed though? In my experiments, a normally constructed empty shared pointer will have the __ctrl_ field as null anyway.
I was able to construct a nullptr shared_ptr with a non-empty control field using the subobject constructor (std::shared_ptr<int> si(new int(47)); std::shared_ptr<int> sie(si, nullptr);), but in that case, maybe we actually should show the weak and shared counts?
There was a problem hiding this comment.
Good point, don't think the early return is necessary. Let me add that subobject constructor test too
There was a problem hiding this comment.
Applied your suggestion (and refactored the weak/strong count logic in the latest commit). Happy to split that up into a separate PR if this is becoming too noisy.
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThe only difference is that with libc++ the summary string contains the derefernced pointer value. With libstdc++ we currently display the pointer itself, which seems redundant. E.g., This patch moves the logic into a common helper that's shared between the libc++ and libstdc++ formatters. Full diff: https://github.com/llvm/llvm-project/pull/146909.diff 5 Files Affected:
diff --git a/lldb/include/lldb/DataFormatters/FormattersHelpers.h b/lldb/include/lldb/DataFormatters/FormattersHelpers.h
index 42699d0a0b1b1..ea77b43e8da39 100644
--- a/lldb/include/lldb/DataFormatters/FormattersHelpers.h
+++ b/lldb/include/lldb/DataFormatters/FormattersHelpers.h
@@ -55,6 +55,12 @@ void AddFilter(TypeCategoryImpl::SharedPointer category_sp,
std::optional<size_t> ExtractIndexFromString(const char *item_name);
+/// Returns \c false if the smart pointer formatters shouldn't continue
+/// formatting the rest of the object. Currently this is the case when the
+/// pointer is a nullptr.
+bool DumpCxxSmartPtrPointerSummary(Stream &stream, ValueObject &ptr,
+ const TypeSummaryOptions &options);
+
Address GetArrayAddressOrPointerValue(ValueObject &valobj);
time_t GetOSXEpoch();
diff --git a/lldb/source/DataFormatters/FormattersHelpers.cpp b/lldb/source/DataFormatters/FormattersHelpers.cpp
index d7b058d91c4a3..022a5d12efae3 100644
--- a/lldb/source/DataFormatters/FormattersHelpers.cpp
+++ b/lldb/source/DataFormatters/FormattersHelpers.cpp
@@ -126,3 +126,25 @@ lldb_private::formatters::GetArrayAddressOrPointerValue(ValueObject &valobj) {
return data_addr.address;
}
+
+bool lldb_private::formatters::DumpCxxSmartPtrPointerSummary(
+ Stream &stream, ValueObject &ptr, const TypeSummaryOptions &options) {
+ if (ptr.GetValueAsUnsigned(0) == 0) {
+ stream.Printf("nullptr");
+ return true;
+ }
+
+ Status error;
+ ValueObjectSP pointee_sp = ptr.Dereference(error);
+ // FIXME: should we be handling this as an error?
+ if (!pointee_sp || !error.Success())
+ return false;
+
+ if (!pointee_sp->DumpPrintableRepresentation(
+ stream, ValueObject::eValueObjectRepresentationStyleSummary,
+ lldb::eFormatInvalid,
+ ValueObject::PrintableRepresentationSpecialCases::eDisable, false))
+ stream.Printf("ptr = 0x%" PRIx64, ptr.GetValueAsUnsigned(0));
+
+ return false;
+}
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index 7143089209dd3..995c943ac4da4 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -166,24 +166,8 @@ bool lldb_private::formatters::LibcxxSmartPointerSummaryProvider(
if (!ptr_sp)
return false;
- if (ptr_sp->GetValueAsUnsigned(0) == 0) {
- stream.Printf("nullptr");
+ if (!DumpCxxSmartPtrPointerSummary(stream, *ptr_sp, options))
return true;
- } else {
- bool print_pointee = false;
- Status error;
- ValueObjectSP pointee_sp = ptr_sp->Dereference(error);
- if (pointee_sp && error.Success()) {
- if (pointee_sp->DumpPrintableRepresentation(
- stream, ValueObject::eValueObjectRepresentationStyleSummary,
- lldb::eFormatInvalid,
- ValueObject::PrintableRepresentationSpecialCases::eDisable,
- false))
- print_pointee = true;
- }
- if (!print_pointee)
- stream.Printf("ptr = 0x%" PRIx64, ptr_sp->GetValueAsUnsigned(0));
- }
if (count_sp)
stream.Printf(" strong=%" PRIu64, 1 + count_sp->GetValueAsUnsigned(0));
@@ -210,24 +194,7 @@ bool lldb_private::formatters::LibcxxUniquePointerSummaryProvider(
if (!ptr_sp)
return false;
- if (ptr_sp->GetValueAsUnsigned(0) == 0) {
- stream.Printf("nullptr");
- return true;
- } else {
- bool print_pointee = false;
- Status error;
- ValueObjectSP pointee_sp = ptr_sp->Dereference(error);
- if (pointee_sp && error.Success()) {
- if (pointee_sp->DumpPrintableRepresentation(
- stream, ValueObject::eValueObjectRepresentationStyleSummary,
- lldb::eFormatInvalid,
- ValueObject::PrintableRepresentationSpecialCases::eDisable,
- false))
- print_pointee = true;
- }
- if (!print_pointee)
- stream.Printf("ptr = 0x%" PRIx64, ptr_sp->GetValueAsUnsigned(0));
- }
+ DumpCxxSmartPtrPointerSummary(stream, *ptr_sp, options);
return true;
}
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
index b47ff579b6a5e..e570a4bb1a886 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
@@ -156,14 +156,8 @@ bool LibStdcppUniquePtrSyntheticFrontEnd::GetSummary(
if (!m_ptr_obj)
return false;
- bool success;
- uint64_t ptr_value = m_ptr_obj->GetValueAsUnsigned(0, &success);
- if (!success)
- return false;
- if (ptr_value == 0)
- stream.Printf("nullptr");
- else
- stream.Printf("0x%" PRIx64, ptr_value);
+ DumpCxxSmartPtrPointerSummary(stream, *m_ptr_obj, options);
+
return true;
}
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/unique_ptr/TestDataFormatterStdUniquePtr.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/unique_ptr/TestDataFormatterStdUniquePtr.py
index 2301e5edfbd90..600a8669c81ba 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/unique_ptr/TestDataFormatterStdUniquePtr.py
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/unique_ptr/TestDataFormatterStdUniquePtr.py
@@ -31,15 +31,15 @@ def test_with_run_command(self):
self.assertTrue(frame.IsValid())
self.expect("frame variable nup", substrs=["nup = nullptr"])
- self.expect("frame variable iup", substrs=["iup = 0x"])
- self.expect("frame variable sup", substrs=["sup = 0x"])
+ self.expect("frame variable iup", substrs=["iup = 123"])
+ self.expect("frame variable sup", substrs=['sup = "foobar"'])
self.expect("frame variable ndp", substrs=["ndp = nullptr"])
self.expect(
- "frame variable idp", substrs=["idp = 0x", "deleter = ", "a = 1", "b = 2"]
+ "frame variable idp", substrs=["idp = 456", "deleter = ", "a = 1", "b = 2"]
)
self.expect(
- "frame variable sdp", substrs=["sdp = 0x", "deleter = ", "a = 3", "b = 4"]
+ "frame variable sdp", substrs=['sdp = "baz"', "deleter = ", "a = 3", "b = 4"]
)
self.assertEqual(
@@ -106,13 +106,13 @@ def test_recursive_unique_ptr(self):
substrs=["stopped", "stop reason = breakpoint"],
)
- self.expect("frame variable f1->fp", substrs=["fp = 0x"])
+ self.expect("frame variable f1->fp", substrs=["fp = Foo @ 0x"])
self.expect(
- "frame variable --ptr-depth=1 f1->fp", substrs=["data = 2", "fp = 0x"]
+ "frame variable --ptr-depth=1 f1->fp", substrs=["data = 2", "fp = Foo @ 0x"]
)
self.expect(
"frame variable --ptr-depth=2 f1->fp",
- substrs=["data = 2", "fp = 0x", "data = 1"],
+ substrs=["data = 2", "fp = Foo @ 0x", "data = 1"],
)
frame = self.frame()
|
|
✅ With the latest revision this PR passed the Python code formatter. |
labath
reviewed
Jul 3, 2025
|
|
||
| Status error; | ||
| ValueObjectSP pointee_sp = ptr.Dereference(error); | ||
| // FIXME: should we be handling this as an error? |
There was a problem hiding this comment.
I think it's reasonable to not print anything in this case.
| /// formatting the rest of the object. Currently this is the case when the | ||
| /// pointer is a nullptr. | ||
| bool DumpCxxSmartPtrPointerSummary(Stream &stream, ValueObject &ptr, | ||
| const TypeSummaryOptions &options); |
There was a problem hiding this comment.
Is it actually needed though? In my experiments, a normally constructed empty shared pointer will have the __ctrl_ field as null anyway.
I was able to construct a nullptr shared_ptr with a non-empty control field using the subobject constructor (std::shared_ptr<int> si(new int(47)); std::shared_ptr<int> sie(si, nullptr);), but in that case, maybe we actually should show the weak and shared counts?
The only difference is that with libc++ the summary string contains the
derefernced pointer value. With libstdc++ we currently display the
pointer itself, which seems redundant. E.g.,
```
(std::unique_ptr<int>) iup = 0x55555556d2b0 {
pointer = 0x000055555556d2b0
}
(std::unique_ptr<std::basic_string<char> >) sup = 0x55555556d2d0 {
pointer = "foobar"
}
```
This patch moves the logic into a common helper that's shared between
the libc++ and libstdc++ formatters.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The only difference is that with libc++ the summary string contains the derefernced pointer value. With libstdc++ we currently display the pointer itself, which seems redundant. E.g.,
This patch moves the logic into a common helper that's shared between the libc++ and libstdc++ formatters.
After this patch we can combine the libc++ and libstdc++ API tests (see #146740).