[RFC PATCH] [gdb] Fix std::unique_ptr printing regression (PR 28480)

[RFC PATCH] [gdb] Fix std::unique_ptr printing regression (PR 28480)

[Bruno Larsen]

The commit 87a37e5e078f506fa9905b74e9238593c537fcd5, introduced to fix
another regression, failed to correctly identify the desired field when
printing a unique_ptr. Before this commit, printing a unique_ptr would
result in the following output:

$1 = std::unique_ptr<datum> = {get() = {<No data fields>}}

while the expected output would be in the form of:

$1 = std::unique_ptr<datum> = {get() = 0x0}

Or with a valid memory address. This happened because the searching
algorithm, when looking for internal members of __uniq_ptr, would stop
at the first _M_head_impl, which is of type class Deleter, instead of
returning the second finding, which is the managed pointer.

This commit fixes that by adding all members with the searched name and
letting search_struct_field decide which is the best candidate, instead
of adding only one at struct_field_searcher::update_result.

An unfortunate side effect of this change is that the output for part of
the test case has changed to returning the last baseclass that virtually
inherits the ambiguous members, instead of retuning the first, so the
test case had to be changed a little.

This is an RFC for two reasons: The first is that I'm not sure changing
the search function is the best way to fix a printing issue, but I
couldn't find another way to fix this, so I'm asking for extra input;

The second is that I'm having trouble with the testing. running with
-data-directory gdb/data-directory gives me a very verbose printing that
is always correct, but also counter-productive for debugging. How can I
make the testsuite have the minimal printing?

---
 gdb/testsuite/gdb.cp/ambiguous.cc  |  2 ++
 gdb/testsuite/gdb.cp/ambiguous.exp | 10 ++++++++--
 gdb/valops.c                       |  5 +++++
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.cp/ambiguous.cc b/gdb/testsuite/gdb.cp/ambiguous.cc
index a55686547f2..c32f13554f1 100644
--- a/gdb/testsuite/gdb.cp/ambiguous.cc
+++ b/gdb/testsuite/gdb.cp/ambiguous.cc
@@ -1,3 +1,4 @@
+#include <memory>
 
 class A1 {
 public:
@@ -92,6 +93,7 @@ int main()
   JVA1 jva1;
   JVA2 jva2;
   JVA1V jva1v;
+  std::unique_ptr<int> uptr(nullptr);
   
   int i;
 
diff --git a/gdb/testsuite/gdb.cp/ambiguous.exp b/gdb/testsuite/gdb.cp/ambiguous.exp
index 008898c5818..dff5f2605c8 100644
--- a/gdb/testsuite/gdb.cp/ambiguous.exp
+++ b/gdb/testsuite/gdb.cp/ambiguous.exp
@@ -224,7 +224,7 @@ gdb_test "print jv.x" " = 1"
 # ancestors, and as a non-virtual immediate base. Ambiguity must
 # be reported.
 test_ambiguous "jva1.x" "x" "JVA1" {
-    "'int A1::x' (JVA1 -> KV -> A1)"
+    "'int A1::x' (JVA1 -> LV -> A1)"
     "'int A1::x' (JVA1 -> A1)"
 }
 
@@ -232,7 +232,7 @@ test_ambiguous "jva1.x" "x" "JVA1" {
 # ancestors, and A2 is a non-virtual immediate base. Ambiguity must
 # be reported as A1 and A2 both have a member 'x'.
 test_ambiguous "jva2.x" "x" "JVA2" {
-    "'int A1::x' (JVA2 -> KV -> A1)"
+    "'int A1::x' (JVA2 -> LV -> A1)"
     "'int A2::x' (JVA2 -> A2)"
 }
 
@@ -264,3 +264,9 @@ gdb_test "print (A1)(KV)jva1" " = \{x = 3, y = 4\}"
 # JVA1V is derived from A1; A1 is a virtual base indirectly
 # and also directly; must not report ambiguity when a JVA1V is cast to an A1.
 gdb_test "print (A1)jva1v" " = {x = 1, y = 2}"
+
+#unique_ptr is a weird edge-case that interacts differently with the
+#ambiguity detection, so we should test it directly
+set print pretty
+print uptr
+gdb_test "print uptr" " = {get() = 0x0}"
diff --git a/gdb/valops.c b/gdb/valops.c
index 9787cdbb513..44ebaf84202 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1962,6 +1962,11 @@ struct_field_searcher::update_result (struct value *v, LONGEST boffset)
 	     space.  */
 	  if (m_fields.empty () || m_last_boffset != boffset)
 	    m_fields.push_back ({m_struct_path, v});
+	  else
+	  {
+	    m_fields.pop_back ();
+	    m_fields.push_back ({m_struct_path, v});
+	  }
 	}
     }
 }
-- 
2.27.0

Re: [RFC PATCH] [gdb] Fix std::unique_ptr printing regression (PR 28480)

[Tom Tromey]

>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

Thank you for the patch.

Bruno> Or with a valid memory address. This happened because the searching
Bruno> algorithm, when looking for internal members of __uniq_ptr, would stop
Bruno> at the first _M_head_impl, which is of type class Deleter, instead of
Bruno> returning the second finding, which is the managed pointer.

I don't really understand this explanation.
However, see my comments below about the test case.

Bruno> The second is that I'm having trouble with the testing. running with
Bruno> -data-directory gdb/data-directory gives me a very verbose printing that
Bruno> is always correct, but also counter-productive for debugging. How can I
Bruno> make the testsuite have the minimal printing?

Bruno> +  std::unique_ptr<int> uptr(nullptr);

Using a standard library type in the test suite is usually not what you
want, because it means that the gdb feature being tested then depends on
the precise implementation of this type.

Instead, because you're changing gdb to handle some C++ scenario, I
think it would be better to write out exactly what is being tested --
basically, the relevant parts of unique_ptr.  I think it's fine to add
these to the existing test.

However, it sounds like you also are trying to test the pretty-printer
here?  This part wasn't clear to me, but IIRC std::unique_ptr has a
pretty-printer in libstdc++.  In this case, you'd also need to write a
pretty-printer, in which case you'd probably just want a whole new test
in gdb.python.

Integration testing for the unique_ptr pretty-printer would also be
good, but those tests ought to be in libstdc++.

I don't have a comment on the particular change here.  I suspect it
ought to be done some other way, but I don't really understand it.  I
think a more self-contained test would help with this as well.

thanks,
Tom

Re: [RFC PATCH] [gdb] Fix std::unique_ptr printing regression (PR 28480)

[Bruno Larsen]

On 11/4/21 15:44, Tom Tromey wrote:
>>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Thank you for the patch.
> 

Thanks for reviewing!

> Bruno> Or with a valid memory address. This happened because the searching
> Bruno> algorithm, when looking for internal members of __uniq_ptr, would stop
> Bruno> at the first _M_head_impl, which is of type class Deleter, instead of
> Bruno> returning the second finding, which is the managed pointer.
> 
> I don't really understand this explanation.
> However, see my comments below about the test case.

Further conversation on bugzilla revealed that I didn't either. Here's a better one: the ambiguity detection was failing on this case, because unique_ptr was using [[no_unique_address]], and the original patch depends on different boffsets to determine if a query is ambiguous.

> 
> Bruno> The second is that I'm having trouble with the testing. running with
> Bruno> -data-directory gdb/data-directory gives me a very verbose printing that
> Bruno> is always correct, but also counter-productive for debugging. How can I
> Bruno> make the testsuite have the minimal printing?
> 
> Bruno> +  std::unique_ptr<int> uptr(nullptr);
> 
> Using a standard library type in the test suite is usually not what you
> want, because it means that the gdb feature being tested then depends on
> the precise implementation of this type.
> 
> Instead, because you're changing gdb to handle some C++ scenario, I
> think it would be better to write out exactly what is being tested --
> basically, the relevant parts of unique_ptr.  I think it's fine to add
> these to the existing test.

Jwakely came up with a better, self contained test, which is in the bugzilla report.

> 
> However, it sounds like you also are trying to test the pretty-printer
> here?  This part wasn't clear to me, but IIRC std::unique_ptr has a
> pretty-printer in libstdc++.  In this case, you'd also need to write a
> pretty-printer, in which case you'd probably just want a whole new test
> in gdb.python.> 
> Integration testing for the unique_ptr pretty-printer would also be
> good, but those tests ought to be in libstdc++.
> 
> I don't have a comment on the particular change here.  I suspect it
> ought to be done some other way, but I don't really understand it.  I
> think a more self-contained test would help with this as well.

Right. The specific case I was testing did come from libstdc++'s pretty printer, but it ended up showing an ambiguous case not covered by the original patch, and this change would mask the problem, instead of fixing it. A proper patch will be comming soon

> 
> thanks,
> Tom
> 


-- 
Cheers!
Bruno Larsen