From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ms9.webland.ch (ms9.webland.ch [92.43.217.109]) by sourceware.org (Postfix) with ESMTPS id 45F61386C5BE for ; Wed, 22 Jun 2022 12:58:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 45F61386C5BE Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=indel.ch Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=ms9.webland.ch Received: from macserver.private ([77.58.17.252]) by ms9.webland.ch (12.3.0 build 2 x64) with ASMTP id 01202206221458432080; Wed, 22 Jun 2022 14:58:43 +0200 Received: from localhost (localhost [127.0.0.1]) by macserver.private (Postfix) with ESMTP id 05AFE4C9F3E3; Wed, 22 Jun 2022 14:58:42 +0200 (CEST) X-Virus-Scanned: amavisd-new at indel.ch Received: from macserver.private ([127.0.0.1]) by localhost (macserver.private [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0aVb+gU7eJ+0; Wed, 22 Jun 2022 14:58:41 +0200 (CEST) Received: from [192.168.1.99] (unknown [192.168.1.99]) by macserver.private (Postfix) with ESMTP id 99BFB4C9F3DC; Wed, 22 Jun 2022 14:58:41 +0200 (CEST) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.21\)) Subject: Re: [PATCH] Fix number of children of varobj with stub debug info From: Christian Walther In-Reply-To: Date: Wed, 22 Jun 2022 14:58:41 +0200 Cc: gdb-patches@sourceware.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <20220621081851.4139622-1-walther@indel.ch> To: Bruno Larsen X-Mailer: Apple Mail (2.3445.104.21) X-CTCH: RefID="str=0001.0A782F1A.62B31203.0038,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0"; Spam="Unknown"; VOD="Unknown" X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Wed, 22 Jun 2022 12:58:48 -0000 Thanks for the quick reaction and for your comments! > The mention to "print object on" looks very weird to me. The fact that > without it GDB works fine makes me think that the different code path > should be fixed. >=20 > Looking at the comments, c-varobj.c:111 says that: >=20 > /* The 'get_target_type' function calls check_typedef on > result, so we can immediately check type code. No > need to call check_typedef here. */ >=20 > So I'd say that going specifically against this comment is not a good = idea. The dependency on "print object on" comes from the "if" on = c-varobj.c:575: when "print object" is off, = adjust_value_for_child_access() is called without passing it the value, = it doesn't enter the "if" on c-varobj.c:116, and the return value from = get_target_type() is returned, which as the mentioned comment says = already had check_typedef() called on it, so all is well. That comment however is irrelevant if the "if" on line 116 is taken, = because in that case the return value from get_target_type() is = discarded (it was only used to convert from the pointer value to the = pointed-to value) and replaced by the type extracted from the value, = which has not had check_typedef() called on it (until I added it = explicitly). So, I don't think I am "going specifically against this = comment". (In fact, I think that comment is in a strange place. It would make more = sense if it was between lines 88 and 89. It was introduced in 2024f65, = when it was at least a little closer.) > My guess for a better solution would be checking if the type we = already had > (either from line 75 or line 105) matches the enclosing type, in which = case > we would keep the previous variable, or check which is more complete. I'm not sure I understand what you mean by this. The type we get on line = 75, in the case where the bug occurs, is the pointer type and therefore = definitely doesn't match the enclosing type. The type we get on line 105 = does match (in the sense of "describes the same type", not "=3D=3D") the = enclosing type in my example, because class C does not have any = super-/subclasses and declared type and runtime type are the same for = variable c, but that need not necessarily be the case. When it is not, = then couldn't the bug still occur unless we add a call to = check_typedef()? Are we guaranteed that the enclosing type is not a stub = (has already had check_typedef() called) in that case? Even if we were, = isn't it still simpler and less error-prone to unconditionally add a = call to check_typedef() on the output of value_actual_type(), as I = proposed, given that as far as I can tell it is a harmless no-op when it = doesn't have to do anything? > Also, for a version 2, it would be nice if you could provide a test = case. > We have a mechanism for generating custom DWARF information for = compiled > programs, called dwarf assembler(sorry if you knew this already). This = way > we can ensure GDB never regresses. I had thought about that, but had no idea where to even start to build a = test case that doesn't require Clang 13 and runs on any architecture. = Your mention of a "dwarf assembler" (I did not know about that) gives me = a clue, that sounds useful. I have now searched for that term and found = it in a couple of places, but it still all looks like gobbledygook to = me. I fear I won't have time to learn enough about DejaGnu or whatever = it is to get such a thing to work. I can't spend more than another day = or two on this. I'm sorry but I fear I'll have to offer the patch = without a test case, take it or leave it. I don't mind if you decline, = we build GDB ourselves with a couple of patches anyway and I can just = apply the patch there to solve my problem. > You must add a whitespace before the ( for function calls. Whoops, I thought I had checked the code style, but that slipped = through. Thanks, will fix in v2. In summary, I am unsure about how to proceed. Unless you can give me = more guidance on what a better solution would be, I will just resubmit = the same patch with the style issue fixed. -Christian