From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id CIyiFF5nemggkBcAWB0awg (envelope-from ) for ; Fri, 18 Jul 2025 11:25:18 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=W85FD2rc; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 503B31E11C; Fri, 18 Jul 2025 11:25:18 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-10.1 required=5.0 tests=ARC_SIGNED,ARC_VALID, BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED, RCVD_IN_VALIDITY_RPBL,RCVD_IN_VALIDITY_SAFE autolearn=ham autolearn_force=no version=4.0.1 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 924C41E089 for ; Fri, 18 Jul 2025 11:25:17 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 3D74C385EC3F for ; Fri, 18 Jul 2025 15:25:17 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3D74C385EC3F Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=W85FD2rc Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id 9DE33385F022 for ; Fri, 18 Jul 2025 15:24:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9DE33385F022 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 9DE33385F022 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1752852278; cv=none; b=NjA+QK8XtU/c1dSd6qpEHxmOehRfoq+Pw1mdzYjMISsLI0TzrjRoP3yfYiYzQKu5N/vLBR3QCQNn1MGvHOob7PwLmes0sHVk754J3hriNU2NdKmQwb4rrvmG5NotbdrNick4AKb3OQ+Urpt1fmDSqg0RiHimohDybvQ0CHi72jg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1752852278; c=relaxed/simple; bh=ZRpoAC9crEwWKfVJDGYkNkbbU4O7eeBMt/TSnDwA4Y4=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=lxlJ3InIuQvuS8QHaGVaMD7sTgU+SIZUGqFxWgnPS5NLbQsuHfnXn0icBLW6iFUH4xJ+hjsS5ZaewPVEVZQAhdamdKib82qNsg+ABhTXi+KfTPCzLqewu30+DukGNmqPHwqkVelAHEwfo49A8hdJ/AxEpJiq/jI39bXuLhS3dMM= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1752852242; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=3SBqwBdVfx3BaSDEdOlkjdMWrvm65rur3N98N07w7sI=; b=W85FD2rczYa++6TPeprRGvkh8yMJH0nBArhF7Oxs53yMwQTruE8qUsEMtj4bmNj0NSOayT kmR+UGEjpA7nQFNGNdxfnhTADyX58IwMVk2wzegwGMHRkrs1FLx1msUcjyIHimTz0hQJtk CJEvnY56P35GM1yuaa3+kAS1+fU5OEE= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-633-f0s0maUdPl-ZMFWy9PPIEQ-1; Fri, 18 Jul 2025 11:24:01 -0400 X-MC-Unique: f0s0maUdPl-ZMFWy9PPIEQ-1 X-Mimecast-MFC-AGG-ID: f0s0maUdPl-ZMFWy9PPIEQ_1752852240 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-45611579300so16994965e9.0 for ; Fri, 18 Jul 2025 08:24:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1752852240; x=1753457040; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=3SBqwBdVfx3BaSDEdOlkjdMWrvm65rur3N98N07w7sI=; b=EA3NNxhPRZJ5Xy3HUA/OMySMox7l0lMHItY3zDko4aD29knArnJY2TAT6mfSX+UG8q svSe5u29Rb/syHT9KA482pwRsaRTs6gblVRB50pGJZcwMJyEbNczV5RKia/k0AjHWogs wU7yyZ4i/QTrrckR+XSAdUKgfAgJBSHfLE4Vl1o1PwCsgnLdxrtspv9hwXzSONILI6U+ lxX27ovjrQ7NwOG5W9SyhRVxEsTtLVh5bZXl4i2GgzHxkiA1x9G56VLfyjsnVBaw/TPK X2WaeglSHuCAipGhuFAe7epborhU6yHncpZZDN6+TdtB/x6mZAQ93fmuSO7y5XsfoAii HeiQ== X-Forwarded-Encrypted: i=1; AJvYcCUlgd5WoTBNLGV24HBlztmicVpoM/afg+IkmaBsKNAnuwzKbIsPb0tbf0QF1WFMEEPGF3sZd2Zvyq0RCg==@sourceware.org X-Gm-Message-State: AOJu0YxG0ITN9IyqR69qbUZWxKI8evGXAOBLWCwwO1bHifadeDuCKgp4 S3dIVeAB9Dh5Z+QgjYJFj1tJyHSMxGLj6+GcmxtY4O/IS+0GA149l9qtuv4+zHPalveQ7Uxq6xD rfSRxjQ/7a3IXLBUFZyMX6JuQRlV7ggvOBFCmEQUn0FjrGSbdx2E9+7H+D/X7WhA= X-Gm-Gg: ASbGncuKjtRkJjI8O65xjdfxRfVjoai6kDl7WexoKCp95Lhr5Q2UTrcU1W3F9G8wl78 A/k2h8Bp5OboTLYKm1bMqJDvhULbRR9yaq5fyk3UAXZV8wIXLzQdKCgYz/Afz+5EgagnpK2ihMb g6I3Z0ZT1GCVX5v6Z1bCQmtNKOn4V4zrhFZ9E7AQM0J2WwodT4BQ2OukiE2JgeWhpzuHCBmVrO0 7Mi4EuZhw995h7T2p3/VZbRYBkCsXPwQHIu6fpm30QyQOH2LQ2MWNVxc5xrva9Qq+37Ot7UCujt +F5NL/0xTJi/wW39igZDEWIemJLLPwzp8lOWC7P2BVyiTZ5d3zuAHu5ODTQ03/qz X-Received: by 2002:a05:600c:5289:b0:456:2771:e654 with SMTP id 5b1f17b1804b1-4562e37726dmr97255125e9.24.1752852240372; Fri, 18 Jul 2025 08:24:00 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFOrRQlqku3VkWl0UqR4+bAmnT3H+k/r22cRDfAo5INj0QqVVm8yMAvvik4fBMPSjH9GPbkaw== X-Received: by 2002:a05:600c:5289:b0:456:2771:e654 with SMTP id 5b1f17b1804b1-4562e37726dmr97254755e9.24.1752852239738; Fri, 18 Jul 2025 08:23:59 -0700 (PDT) Received: from localhost (75.226.159.143.dyn.plus.net. [143.159.226.75]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3b61ca4d77bsm2164604f8f.70.2025.07.18.08.23.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 18 Jul 2025 08:23:59 -0700 (PDT) From: Andrew Burgess To: Guinevere Larsen , Pawel Kupczak , gdb-patches@sourceware.org Subject: Re: [PATCH 1/3] gdb, amd64: extend the amd64 prologue analyzer to skip register pushes In-Reply-To: <91a1e38e-2978-4e81-a168-762ed55d3811@redhat.com> References: <20250701104759.52595-1-pawel.kupczak@intel.com> <20250701104759.52595-2-pawel.kupczak@intel.com> <91a1e38e-2978-4e81-a168-762ed55d3811@redhat.com> Date: Fri, 18 Jul 2025 16:23:58 +0100 Message-ID: <8734atldyp.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: eGIbhN3V7rgdXlIFY_Tt-xrkqqAN1DYu3mNv2hV7zGM_1752852240 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org Guinevere Larsen writes: > Hello! Thanks for working on this! > > I am not an authority on i386, but I did notice a few things that gave > me pause so I'd like to ask them to make sure I understand the changes. > > Skipping the commit message: > On 7/1/25 7:47 AM, Pawel Kupczak wrote: >> --- >> gdb/amd64-tdep.c | 53 +++++++++++- >> .../amd64-extended-prologue-analysis.c | 49 +++++++++++ >> .../amd64-extended-prologue-analysis.exp | 86 +++++++++++++++++++ >> 3 files changed, 187 insertions(+), 1 deletion(-) >> mode change 100644 => 100755 gdb/amd64-tdep.c >> create mode 100644 gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.c >> create mode 100644 gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.exp >> >> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c >> old mode 100644 >> new mode 100755 >> index 82dd1e07cf3..863b29a8c27 >> --- a/gdb/amd64-tdep.c >> +++ b/gdb/amd64-tdep.c >> @@ -2553,6 +2553,56 @@ amd64_analyze_frame_setup (gdbarch *gdbarch, CORE_ADDR pc, >> return pc; >> } >> >> +/* Check whether PC points at code pushing registers onto the stack. If so, >> + update CACHE and return pc after those pushes or CURRENT_PC, whichever is >> + smaller. Otherwise, return PC passed to this function. */ >> + >> +static CORE_ADDR >> +amd64_analyze_register_saves (CORE_ADDR pc, CORE_ADDR current_pc, >> + amd64_frame_cache *cache) >> +{ >> + gdb_byte op; >> + int offset = 0; >> + >> + /* There are at most 16 registers that would be pushed in the prologue. */ >> + for (int i = 0; i < 16 && pc < current_pc; i++) >> + { >> + int reg = 0; >> + int pc_offset = 0; >> + >> + if (target_read_code (pc, &op, 1) == -1) >> + return pc; >> + >> + /* %r8 - %r15 prefix. */ >> + if (op == 0x41) > > Looking over on the disassembler for record-full, I see that all 0x4- > are considered prefixes, and (in 64 bit targets) they all have this effect. > > Is this something that could affect this prologue analyzer? ie, that > some prefix like 0x40 is used to mean "push a register larger than 7", > which would cause us to not skip that instruction? > >> + { >> + reg += 8; >> + pc_offset = 1; >> + >> + if (target_read_code (pc + 1, &op, 1) == -1) >> + return pc; >> + } >> + >> + /* push %rax|%rcx|%rdx|%rbx|%rsp|%rbp|%rsi|%rdi >> + >> + or with 0x41 prefix: >> + push %r8|%r9|%r10|%r11|%r12|%r13|%r14|%r15. */ >> + if (op < 0x50 || op > 0x57) >> + break; > > This code also ignores the "push lz" and "push lb" instructions. Could > they be used as prologue instructions? (again, this comes from the > recording disassembler) > > If so, I would think this is worth explicity saying it in the comment at > the top of the function, otherwise it could confuse us in the future. > >> + >> + reg += op - 0x50; >> + offset -= 8; >> + >> + int regnum = amd64_arch_reg_to_regnum (reg); >> + cache->saved_regs[regnum] = offset; >> + cache->sp_offset += 8; >> + >> + pc += 1 + pc_offset; >> + } >> + >> + return pc; >> +} >> + >> /* Do a limited analysis of the prologue at PC and update CACHE >> accordingly. Bail out early if CURRENT_PC is reached. Return the >> address where the analysis stopped. >> @@ -2594,7 +2644,8 @@ amd64_analyze_prologue (gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR current_pc, >> if (current_pc <= pc) >> return current_pc; >> >> - return amd64_analyze_frame_setup (gdbarch, pc, current_pc, cache); >> + pc = amd64_analyze_frame_setup (gdbarch, pc, current_pc, cache); >> + return amd64_analyze_register_saves (pc, current_pc, cache); >> } >> >> /* Work around false termination of prologue - GCC PR debug/48827. >> diff --git a/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.c b/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.c >> new file mode 100644 >> index 00000000000..7d777d23236 >> --- /dev/null >> +++ b/gdb/testsuite/gdb.arch/amd64-extended-prologue-analysis.c >> @@ -0,0 +1,49 @@ >> +/* This testcase is part of GDB, the GNU debugger. >> + >> + Copyright 2025 Free Software Foundation, Inc. >> + >> + This program is free software; you can redistribute it and/or modify >> + it under the terms of the GNU General Public License as published by >> + the Free Software Foundation; either version 3 of the License, or >> + (at your option) any later version. >> + >> + This program is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + GNU General Public License for more details. >> + >> + You should have received a copy of the GNU General Public License >> + along with this program. If not, see . */ >> + >> +int __attribute__ ((noinline)) >> +bar (int x) >> +{ >> + return x + x; >> +} >> + >> +/* This function should generate a prologue in shape of: >> + push %rbp >> + .cfi_def_cfa_offset 16 >> + .cfi_offset %rbp, -16 >> + mov %rsp, %rbp >> + .cfi_def_cfa_register %rbp >> + push %reg1 >> + push %reg2 >> + .cfi_offset %reg2, 32 >> + .cfi_offset %reg1, 24 >> + >> + So to be able to unwind a register, GDB needs to skip prologue past >> + register pushes (to access .cfi directives). */ > > In general, if possible, I think it's preferred that we actually use > assembly instead of relying on compiler behavior. You should be able to > use asm statements to create a fake function, and either the dwarf > assembler to actually label it as such... Maybe even using global labels > to do that more easily (not sure if GDB would use the prologue skip in > that case, though). I think there's definitely scope for both types of test in cases like this. On the one hand, we are interested in debugging actual code, so if a future compiler does something unexpected, then it would be nice to see a test break. But as Gwen says, having tests that use fixed assembler sequences does mean that GDB is guaranteed to always support a particular series of instructions. The good thing in this case is that the test is 100% amd64 specific, so it should be easy enough to just grab the disassembly from the current test binary, and build a test around that. You might even be able to fold the two tests into one, something like: standard_testfile .c .S then you can compile $srcfile and $srcfile2 in turn and run the exact same test proc against the compiled binary. Though this isn't a hard requirement, if its easier to do it some other way, then that's fine. Thanks, Andrew