From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 2D21A3858439 for ; Thu, 4 Nov 2021 19:10:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2D21A3858439 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-330-RSu0e_buONOd1FqJCew4wQ-1; Thu, 04 Nov 2021 15:10:30 -0400 X-MC-Unique: RSu0e_buONOd1FqJCew4wQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7F5F6102C857; Thu, 4 Nov 2021 19:10:21 +0000 (UTC) Received: from [10.97.116.55] (ovpn-116-55.gru2.redhat.com [10.97.116.55]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6D36170951; Thu, 4 Nov 2021 19:10:16 +0000 (UTC) Message-ID: <4e363cd6-5491-2570-788c-ae2adde3dfe4@redhat.com> Date: Thu, 4 Nov 2021 16:10:13 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [RFC PATCH] [gdb] Fix std::unique_ptr printing regression (PR 28480) To: Tom Tromey , Bruno Larsen via Gdb-patches References: <20211104151317.19615-1-blarsen@redhat.com> <874k8rkbf9.fsf@tromey.com> From: Bruno Larsen In-Reply-To: <874k8rkbf9.fsf@tromey.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Nov 2021 19:10:33 -0000 On 11/4/21 15:44, Tom Tromey wrote: >>>>>> "Bruno" == Bruno Larsen via Gdb-patches 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 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