From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from barracuda.ebox.ca (barracuda.ebox.ca [96.127.255.19]) by sourceware.org (Postfix) with ESMTPS id 0275D3858D3C for ; Wed, 16 Feb 2022 04:53:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0275D3858D3C X-ASG-Debug-ID: 1644987195-0c856e06ab46d220001-fS2M51 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id QtOpwwqdRW2U3rK0 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 15 Feb 2022 23:53:15 -0500 (EST) X-Barracuda-Envelope-From: simon.marchi@polymtl.ca X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from simark.localdomain (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) by smtp.ebox.ca (Postfix) with ESMTP id ABA14441B21; Tue, 15 Feb 2022 23:53:15 -0500 (EST) From: Simon Marchi X-Barracuda-RBL-IP: 192.222.157.6 X-Barracuda-Effective-Source-IP: 192-222-157-6.qc.cable.ebox.net[192.222.157.6] X-Barracuda-Apparent-Source-IP: 192.222.157.6 To: gdb-patches@sourceware.org Subject: [PATCH] gdb: remove unneeded value_address call in value_cast_structs Date: Tue, 15 Feb 2022 23:53:14 -0500 X-ASG-Orig-Subj: [PATCH] gdb: remove unneeded value_address call in value_cast_structs Message-Id: <20220216045314.813723-1-simon.marchi@polymtl.ca> X-Mailer: git-send-email 2.35.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Barracuda-Connect: smtp.ebox.ca[96.127.255.82] X-Barracuda-Start-Time: 1644987195 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at ebox.ca X-Barracuda-Scan-Msg-Size: 4383 X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.00 X-Barracuda-Spam-Status: No, SCORE=0.00 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests= X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.96058 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- X-Spam-Status: No, score=-3613.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_QUARANTINE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_SOFTFAIL, TXREP, T_SCC_BODY_TEXT_LINE 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: Wed, 16 Feb 2022 04:53:35 -0000 In the context of the AMD ROCm port of GDB, in order to better support multiple address spaces, we are introducing a new type `gdb::address` that contains a CORE_ADDR plus an address space specifier. We add the appropriate operator overloads to allow common operations, such as adding an address and an offset (yielding an address) and subtracting two addresses (yielding an offset). This tripped on the code changed by this patch, as the types didn't work out. That lead me to believe that the code removed here might be wrong. That code is used when casting a base class pointer to a derived class pointer. For example, with: struct Base1 { int b1; }; struct Base2 { int b2; }; struct Derived : public Base1, Base2 { int d; }; int main() { Derived d; return 0; } and this command: (gdb) p (Derived *)(Base2 *)&d When casting from a `Base2 *` to a `Derived *`, GDB must subtract from the Base2 pointer value the offset between the start of a `Derived` object and the start of the Base2 object embedded in it. In the code, `addr2` is the value of the Base2 pointer, and `v` is a value of type `Base2`, with its embedded offset field set to the offset of a `Base2` object within a `Derived` object. So, subtracting `value_embedded_offset (v)` from addr2 makes sense. But subtracting `value_address (v)` doesn't seem to make sense. Value `v` is of lval kind `not_lval`, so `value_address (v)` returns 0, so there's no actual harm, it's just useless. And since value `v` is derived from this call just above: v = search_struct_field (t2->name (), value_zero (t1, not_lval), t1, 1); which looks up a field from a not_lval value, `v` will always be not_lval. And therefore `value_address (v)` will always return 0. And therefore it can be removed. I did some archeological research to understand what's happening here. You can ignore this if not interested. In today's DWARF, the offset of a base classe is described using a constant value for the DW_AT_data_member_location attribute in the DW_TAG_inheritance DIE (at least for all compilers I know, there's no reason not to do it). But in DWARF 2 [1], DW_AT_data_member_location could only be a location description yielding the address of the base object, given the address of the derived object as input. So what this code did was to pretend there was a derived object at address 0 and calculate the location of the base object. That technically yields an address, but since we started with the derived object at address 0, the resulting address is equal to the offset we are looking for. This is confirmed by these this message: https://sourceware.org/pipermail/gdb-patches/2002-August/019240.html Right --- when casting from a base class to a derived class, we use search_struct_field to find the offset the base class has in the derived class by creating an imaginary instance of the derived class at address zero, finding the base class in that, and then using the base class subobject's address as the offset of the base class in the derived class. Although the trick is explained in this post, it actually comes from an old HP merge commit, here, without more explanation: https://gitlab.com/gnutools/binutils-gdb/-/commit/4ef1f4677390c085543fe80eec41b0fe5d58ddca?page=4#4fad73ebb501f19c2b94682df5910fd5c6553d9c_258_332 I tried browsing that version of the code, but there are still gaps in my comprehension. I am not sure where, when calling search_struct_field, the address of the resulting value would be adjusted. [1] https://dwarfstd.org/doc/dwarf-2.0.0.pdf Change-Id: I118c4ffbbff94d54d9fbee3bbb191d9cea4a7481 --- gdb/valops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gdb/valops.c b/gdb/valops.c index 3a595125752f..5d9f6ebf004a 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -276,7 +276,7 @@ value_cast_structs (struct type *type, struct value *v2) /* Downcasting is possible (t1 is superclass of v2). */ CORE_ADDR addr2 = value_address (v2); - addr2 -= value_address (v) + value_embedded_offset (v); + addr2 -= value_embedded_offset (v); return value_at (type, addr2); } } base-commit: bc85f56bfda7162688b9bea8dd942ec473bdb21b -- 2.35.1