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

[Bruno Larsen <blarsen@redhat.com>]

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