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 9AE873858418 for ; Mon, 6 Dec 2021 15:14:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9AE873858418 Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-440-d91-QEPzONi1RlvXEa-TEQ-1; Mon, 06 Dec 2021 10:13:22 -0500 X-MC-Unique: d91-QEPzONi1RlvXEa-TEQ-1 Received: by mail-wm1-f71.google.com with SMTP id ay34-20020a05600c1e2200b00337fd217772so42299wmb.4 for ; Mon, 06 Dec 2021 07:13:22 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=Rc+ODc+tssND9m1qg+j35BtC01DYUfTUyjzCK8oDr8w=; b=sBByce9lfGNmulFFMzNOpxMB15omQ+OVQA2Tmj7MNYBCPtL/ALXdMzFamk9aVMBU8G vHdNkYq10H8nMuTtfH5M8ADbJUKhGcyUszuvPsWrZYnZpXb5i8kYOKXMP/IuWXyyb4MH sAtEuvejoOkEtxt1yZhK7+LLI2S1e60uBjLgtdCRSM1diIyThVXFErsagBun7ZMKdGxK 0/0C16v2o30PKeNexXfGmYjOpPiP3rkLE0Nhh6IcWBM5KLcC5IVCxB2RRkzEClGkGMTD n5FTCERZPiceB8seTHD7K7GIiWOJ+bVPZ3++hGGqLyezHlxYM4xKMi5nYGJa18klA/Zp XSTA== X-Gm-Message-State: AOAM5324QwY/00m/yhqP53PcR9nOz8GHuowTpwyQxD/Ql6YZkqgpfIoz Ba3D/U0+HlmJ908WrV5YvCgTBFxtE+ZJtwCoP08wS3BbEtBlZe6+HTezDAUuJzDIIYnm4fiO1V3 zVlagmRkhGObmerID/jjSig== X-Received: by 2002:a05:600c:1c20:: with SMTP id j32mr38960208wms.1.1638803600807; Mon, 06 Dec 2021 07:13:20 -0800 (PST) X-Google-Smtp-Source: ABdhPJxvqtEOoh+kB8fs9n2sgKOZzyp0B8fxqw6LJHS6qbxSwRaTRaaJV0Y2zOxjQvm2vyvN1dyZrg== X-Received: by 2002:a05:600c:1c20:: with SMTP id j32mr38960170wms.1.1638803600479; Mon, 06 Dec 2021 07:13:20 -0800 (PST) Received: from localhost (host86-134-238-138.range86-134.btcentralplus.com. [86.134.238.138]) by smtp.gmail.com with ESMTPSA id ay21sm14132668wmb.7.2021.12.06.07.13.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Dec 2021 07:13:20 -0800 (PST) Date: Mon, 6 Dec 2021 15:13:19 +0000 From: Andrew Burgess To: Pedro Alves Cc: Djordje Todorovic , Nikola Tesic , "gdb-patches@sourceware.org" , "asowda@cisco.com" Subject: Re: gdb: Implement the init_reg dwarf2_frame_ops for amd64 Message-ID: <20211206151319.GD123597@redhat.com> References: <32b9671e-7d7e-46bf-a703-62d9c100c6d5@syrmia.com> <87pmr95u4j.fsf@redhat.com> <20211202155655.GK2662946@redhat.com> <58f03bd2-57a6-3e4a-1544-44cfbed33727@palves.net> MIME-Version: 1.0 In-Reply-To: <58f03bd2-57a6-3e4a-1544-44cfbed33727@palves.net> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 15:06:41 up 5:08, 1 X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_NUMSUBJECT, RCVD_IN_BARRACUDACENTRAL, 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: Mon, 06 Dec 2021 15:14:44 -0000 * Pedro Alves [2021-12-03 17:45:04 +0000]: > On 2021-12-02 15:56, Andrew Burgess via Gdb-patches wrote: > > * Djordje Todorovic [2021-11-18 12:42:32 +0000]: > > > >> Hi Andrew, > >> > >> Please find the patch, rebased on top of your improvement. > >> > >> Thanks, > >> Djordje > >> > >> --- > >> gdb/amd64-tdep.c | 37 +++++++++++++ > >> gdb/frame.c | 9 +++- > >> .../gdb.arch/amd64-invalid-stack-middle.exp | 8 +-- > >> gdb/testsuite/gdb.arch/amd64-not-saved-regs.c | 28 ++++++++++ > >> .../gdb.arch/amd64-not-saved-regs.exp | 52 +++++++++++++++++++ > >> gdb/testsuite/gdb.dwarf2/dw2-op-out-param.exp | 4 +- > >> .../gdb.dwarf2/dw2-reg-undefined.exp | 8 +-- > >> gdb/testsuite/gdb.mi/mi-reg-undefined.exp | 4 +- > >> 8 files changed, 136 insertions(+), 14 deletions(-) > >> create mode 100644 gdb/testsuite/gdb.arch/amd64-not-saved-regs.c > >> create mode 100644 gdb/testsuite/gdb.arch/amd64-not-saved-regs.exp > >> ? > >> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c > >> index 7c67359678b..a6b544145fb 100644 > >> --- a/gdb/amd64-tdep.c > >> +++ b/gdb/amd64-tdep.c > >> @@ -25,6 +25,8 @@ > >> #include "arch-utils.h" > >> #include "block.h" > >> #include "dummy-frame.h" > >> +#include "dwarf2.h" > >> +#include "dwarf2/frame.h" > >> #include "frame.h" > >> #include "frame-base.h" > >> #include "frame-unwind.h" > >> @@ -3103,6 +3105,38 @@ amd64_in_indirect_branch_thunk (struct gdbarch *gdbarch, CORE_ADDR pc) > >> AMD64_RIP_REGNUM); > >> } > >> > >> +/* Implement the "init_reg" dwarf2_frame_ops method. */ > >> + > >> +static void > >> +amd64_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum, > >> + struct dwarf2_frame_state_reg *reg, > >> + struct frame_info *this_frame) > >> +{ > >> + switch (regnum) > >> + { > >> + case AMD64_RIP_REGNUM: > >> + reg->how = DWARF2_FRAME_REG_FN; > >> + return; > >> + > >> + case AMD64_RSP_REGNUM: > >> + reg->how = DWARF2_FRAME_REG_CFA; > >> + return; > >> + > >> + /* Caller-saved registers. */ > >> + case AMD64_RAX_REGNUM: > >> + case AMD64_RDI_REGNUM: > >> + case AMD64_RSI_REGNUM: > >> + case AMD64_RDX_REGNUM: > >> + case AMD64_RCX_REGNUM: > >> + case AMD64_R8_REGNUM: > >> + case AMD64_R9_REGNUM: > >> + case AMD64_R10_REGNUM: > >> + case AMD64_R11_REGNUM: > >> + reg->how = DWARF2_FRAME_REG_UNDEFINED; > >> + return; > >> + } > >> +} > > > > I believe that this is the System-V ABI, which is not the only ABI > > used on x86-64. From what I've read I believe that Windows uses a > > slightly different ABI, where $rsi and $rdi are not callee saved. > > > > I think that we might consider moving this function from the general > > amd64-tdep.c to something like amd64-linux-tdep.c, and register this > > in the function amd64_linux_init_abi. > > > > Of course, this will mean that other System-V like targets would miss > > out for now, but maybe the function itself could be renamed to > > something like amd64_system_v_dwarf2_frame_init_reg, and left in > > amd64-tdep.c, then from amd64-linux-tdep.c we can register that > > function. And in future, other system-v like targets can also > > register the function. > > > >> + > >> void > >> amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch, > >> const target_desc *default_tdesc) > >> @@ -3217,6 +3251,9 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch, > >> set_gdbarch_stab_reg_to_regnum (gdbarch, amd64_dwarf_reg_to_regnum); > >> set_gdbarch_dwarf2_reg_to_regnum (gdbarch, amd64_dwarf_reg_to_regnum); > >> > >> + /* Frame handling. */ > >> + dwarf2_frame_set_init_reg (gdbarch, amd64_dwarf2_frame_init_reg); > >> + > >> /* We don't override SDB_REG_RO_REGNUM, since COFF doesn't seem to > >> be in use on any of the supported AMD64 targets. */ > >> > >> diff --git a/gdb/frame.c b/gdb/frame.c > >> index 2a899fc494f..da12ed36e02 100644 > >> --- a/gdb/frame.c > >> +++ b/gdb/frame.c > >> @@ -2315,9 +2315,14 @@ get_prev_frame_always (struct frame_info *this_frame) > >> } > >> catch (const gdb_exception_error &ex) > >> { > >> - if (ex.error == MEMORY_ERROR) > >> + if (ex.error == MEMORY_ERROR || ex.error == OPTIMIZED_OUT_ERROR) > >> { > >> - this_frame->stop_reason = UNWIND_MEMORY_ERROR; > >> + if (ex.error == MEMORY_ERROR) > >> + this_frame->stop_reason = UNWIND_MEMORY_ERROR; > >> + else > >> + /* This is for the OPTIMIZED_OUT_ERROR case. */ > >> + this_frame->stop_reason = UNWIND_UNAVAILABLE; > > Hmm? Nak on this part, it makes no sense to me, but I may be missing something. > Optimized out should never be converted to UNWIND_UNAVAILABLE. I had a misunderstanding about the meaning on unavailable within GDB (see below), but given that, maybe we need another unwind stop reason, UNWIND_OPTIMIZED_OUT? For when the information required to unwind has been optimised away? > > The patch is missing a commit log describing it / providing a rationale, > so I'm kind of lost here. Could you add that, please? > > >> + > >> if (ex.message != NULL) > >> { > >> char *stop_string; > >> diff --git a/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp b/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp > >> index f9e590d83bb..6ae1c9c1322 100644 > >> --- a/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp > >> +++ b/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp > >> @@ -42,11 +42,11 @@ if ![runto breakpt] { > >> return -1 > >> } > >> > >> -gdb_test "bt" "^bt\r\n#0 +breakpt *\\(\\) \[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in func5\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in func4\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in func3\[^\r\n\]*\r\nBacktrace stopped: Cannot access memory at address 0x\[0-9a-f\]+" \ > >> - "first backtrace, with error message" > >> +gdb_test "bt" "^bt\r\n#0 +breakpt *\\(\\) \[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in func5\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in func4\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in func3\[^\r\n\]*\r\nBacktrace stopped: value has been optimized out" \ > >> + "first backtrace, with error message" > > > > I think the new "value has been optimized out" message is better than > > what we had previously ("Cannot access memory at address 0x....") > > which I guess was caused by trying to access through an unavailable > > register. > > > > That said, I remember a long time ago there was a whole big discussion > > about whether registers that were not saved should be described as > > "optimized out" or not. In the end we added the "" message > > to cover this case. > > I don't recall a big discussion about it, but yes, an optimized out > register is presented as , as that's what it means. > > > > > So I wonder if we should instead be ending the backtrace with > > something like: "Backtrace stopped: value is not available"? I took > > that exact text from require_available (value.c) but anything that > > talks about available rather than optimized out might be better. > > I would rather this said "register not saved" or something > around it. Maybe even mention the register name. That's because "unavailable" > has a different meaning from optimized out / not saved -- is for > when e.g., we're looking at a traceframe, and some register/memory we need to > display the value wasn't collected, it's not in the traceframe. Or a > similar thing with a trimmed corefile. Or e.g., the register exists in the machine, > but the kernel is running an older kernel missing a PTRACE op to get at the register. > I.e., it's for when the value actually exists, but we don't have a way to get at it. > > > > > Though I don't know how hard it would be to achieve that result. I > > guess we'd need to start in dwarf2_frame_prev_register where we map > > DWARF2_FRAME_REG_UNDEFINED onto optimized_out - maybe that needs to > > map to an unavailable value instead, but that feels like it might have > > huge knock on consequences ... we don't want optimized out variables > > to suddenly start reporting themselves as unavailable instead... > > Please don't. No, that would be the wrong thing to do given the intended use of unavailable. I knew unavailable was used for traceframes, but thought that it was also used for registers that hadn't been saved between frames. Given the explanation you gave above then using optimized out for unsaved registers is what we expect. Thanks for correcting me on this. Andrew