From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x442.google.com (mail-wr1-x442.google.com [IPv6:2a00:1450:4864:20::442]) by sourceware.org (Postfix) with ESMTPS id 402AD3954C7C for ; Thu, 18 Jun 2020 18:25:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 402AD3954C7C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x442.google.com with SMTP id q11so7108460wrp.3 for ; Thu, 18 Jun 2020 11:25:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=has3kkG1SeuQQFfKY1oOxYqM63uxsdz3ItssRlGbgBY=; b=TDflMz1CzxVCvBT0NpjHxz/1DtNjlKw9XRUcM2MLM+/YK3phYYUY4ITqPniJ8P5u2y msglJClQpOk5cOj1YhI6gBFJ2cnnGQgLUWTzVP/h0s0jO8KruvavA0inp1sUKJuMtl6Y QoQ2C0SUqGzj+B18LM0yGnrc4OnjrDXLH36CMG0K4aWx7a1C+2l/pOHhDsE97Ue1evmp RtsFc0YPY/6OqrCL6xU24yoYqJ34myeBorFYbcGH+vchCbgxiOR9IXjMml5d//oB8tK5 LG2rLnCv5WcYj6/nWd8hOd2SSWgGiqL90dTsluMFh3ZONLSiJsWCCTp6yvaxxROA6Wb6 IzmA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=has3kkG1SeuQQFfKY1oOxYqM63uxsdz3ItssRlGbgBY=; b=OaUzw+z8KKS4QeSu92uhyJp36oisQ/5jlaFdk9Bac77pg2xisCHpyTS5R1cdQVeKuw AkO7zILI7q8mumXi/m8xljURfzSuH8mX6y1qJv0KgwU71FjbxRfR2LPYTzCY+fous1O6 V+B5eesCRjAoc6/BzBZ+8SJalRnlgao4v9NjT9uOzEQl4hUbgYi2sDnJhKwPK7tOjUU4 I/9MZMiPNMq9BcDNniPkC4K6TSOtj+bYX5zsGmiQkcFEXTUwjhCLKvxSYdJiL3n0MC8C f5dkT4OeQ1P+VvebRFjvA7C/bKY6/EBB2JFTchUpqCwCGUlmX2w4oaUR3Ufxde61RH5/ oRiA== X-Gm-Message-State: AOAM533hU+D3W9kjYUbi9x4bT9bzcZQQhZqaHYMKwOOMW4Eo29DOb57p OdcwPcF74LdL+OgleYOk2lJeOtrX0bk= X-Google-Smtp-Source: ABdhPJx0x5RuxfD8JuHvqHe9ayZjo+KoLRyN0dW8DAYZXHdkW1PWy0Cc6zeeUmpC75jPtW3zHE7Byg== X-Received: by 2002:adf:93e3:: with SMTP id 90mr5891685wrp.298.1592504704199; Thu, 18 Jun 2020 11:25:04 -0700 (PDT) Received: from localhost (host86-128-12-16.range86-128.btcentralplus.com. [86.128.12.16]) by smtp.gmail.com with ESMTPSA id k64sm4464793wmf.34.2020.06.18.11.25.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 Jun 2020 11:25:03 -0700 (PDT) Date: Thu, 18 Jun 2020 19:25:02 +0100 From: Andrew Burgess To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Find tailcall frames before inline frames Message-ID: <20200618182502.GD2737@embecosm.com> References: <20200220155820.22809-1-tromey@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200220155820.22809-1-tromey@adacore.com> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 19:21:01 up 10 days, 8:27, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 18 Jun 2020 18:25:07 -0000 * Tom Tromey [2020-02-20 08:58:20 -0700]: > A customer reported a failure to unwind in a certain core dump. A > lengthy investigation showed that the problem came from the > interaction between the tailcall and inline frame sniffers. > > Normally, the regular DWARF unwinder may discover a chain of tail > calls ending in the current frame. In this case, it sets a member on > the dwarf2_frame_cache object, so that a subsequent call into the > tailcall sniffer will create the tailcall frames. > > However, in this scenario, what happened is that the DWARF unwinder > did find tailcall frames -- but then the PC of the first such frame > was recognized and claimed by the inline frame sniffer. I'm trying to understand the setup you have here in the hope I might be able to craft a test case for this - given that I'm not convinced the new placement of the tail call sniffer is safe. Was the setup something like: ,-- f3 tail calls to f4. | | f1 --> f2 --> f3 --> f4 --> f5 --> f6 |_______________| All inlined in f1 Was there anything else special about this case? I feel like there must have been, but I don't really understand the problem description. Thanks, Andrew > > This then caused unwinding to go astray further up the stack. > > This patch fixes the problem by arranging for the tailcall sniffer to > be called before the inline sniffer. This way, if a DWARF frame has > tailcall information, the tailcalls will always be processed first. > This is safe to do, because the tailcall sniffer can only claim a > frame if the previous frame did in fact find this information. (So, > for example, if no DWARF frame is ever found, then this sniffer will > never trigger.) > > This patch also partially reverts: > > commit 1ec56e88aa9b052ab10b806d82fbdbc8d153d977 > Author: Pedro Alves > Date: Fri Nov 22 13:17:46 2013 +0000 > > Eliminate dwarf2_frame_cache recursion, don't unwind from the dwarf2 sniffer (move dwarf2_tailcall_sniffer_first elsewhere). > > That patch moved the call to dwarf2_tailcall_sniffer_first out of > dwarf2_frame_cache, and into dwarf2_frame_prev_register. However, in > this situation, this is too late -- by the time > dwarf2_frame_prev_register is called, the frame in question is already > recognized by the inline frame sniffer. > > Rather than fully revert that patch, though, this just arranges to > call dwarf2_tailcall_sniffer_first from dwarf2_frame_cache -- which is > called shortly after the DWARF frame sniffer succeeds, via > compute_frame_id. > > I don't know how to write a test case for this. > > gdb/ChangeLog > 2020-02-20 Tom Tromey > > * dwarf2/frame.c (struct dwarf2_frame_cache) > entry_cfa_sp_offset_p>: Remove members. > (dwarf2_frame_cache): Call dwarf2_tailcall_sniffer_first. > (dwarf2_frame_prev_register): Don't call > dwarf2_tailcall_sniffer_first. > (dwarf2_append_unwinders): Don't append tailcall unwinder. > * frame-unwind.c (add_unwinder): New fuction. > (frame_unwind_init): Use it. Add tailcall unwinder. > --- > gdb/ChangeLog | 12 ++++++++++++ > gdb/dwarf2/frame.c | 34 ++++++++-------------------------- > gdb/frame-unwind.c | 33 +++++++++++++++++++++++++++------ > 3 files changed, 47 insertions(+), 32 deletions(-) > > diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c > index b240a25e2d8..74488f9a8aa 100644 > --- a/gdb/dwarf2/frame.c > +++ b/gdb/dwarf2/frame.c > @@ -959,22 +959,12 @@ struct dwarf2_frame_cache > /* The .text offset. */ > CORE_ADDR text_offset; > > - /* True if we already checked whether this frame is the bottom frame > - of a virtual tail call frame chain. */ > - int checked_tailcall_bottom; > - > /* If not NULL then this frame is the bottom frame of a TAILCALL_FRAME > sequence. If NULL then it is a normal case with no TAILCALL_FRAME > involved. Non-bottom frames of a virtual tail call frames chain use > dwarf2_tailcall_frame_unwind unwinder so this field does not apply for > them. */ > void *tailcall_cache; > - > - /* The number of bytes to subtract from TAILCALL_FRAME frames frame > - base to get the SP, to simulate the return address pushed on the > - stack. */ > - LONGEST entry_cfa_sp_offset; > - int entry_cfa_sp_offset_p; > }; > > static struct dwarf2_frame_cache * > @@ -1037,6 +1027,8 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache) > in an address that's within the range of FDE locations. This > is due to the possibility of the function occupying non-contiguous > ranges. */ > + LONGEST entry_cfa_sp_offset; > + int entry_cfa_sp_offset_p = 0; > if (get_frame_func_if_available (this_frame, &entry_pc) > && fde->initial_location <= entry_pc > && entry_pc < fde->initial_location + fde->address_range) > @@ -1049,8 +1041,8 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache) > && (dwarf_reg_to_regnum (gdbarch, fs.regs.cfa_reg) > == gdbarch_sp_regnum (gdbarch))) > { > - cache->entry_cfa_sp_offset = fs.regs.cfa_offset; > - cache->entry_cfa_sp_offset_p = 1; > + entry_cfa_sp_offset = fs.regs.cfa_offset; > + entry_cfa_sp_offset_p = 1; > } > } > else > @@ -1195,6 +1187,10 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"), > && fs.regs.reg[fs.retaddr_column].how == DWARF2_FRAME_REG_UNDEFINED) > cache->undefined_retaddr = 1; > > + dwarf2_tailcall_sniffer_first (this_frame, &cache->tailcall_cache, > + (entry_cfa_sp_offset_p > + ? &entry_cfa_sp_offset : NULL)); > + > return cache; > } > > @@ -1239,16 +1235,6 @@ dwarf2_frame_prev_register (struct frame_info *this_frame, void **this_cache, > CORE_ADDR addr; > int realnum; > > - /* Check whether THIS_FRAME is the bottom frame of a virtual tail > - call frame chain. */ > - if (!cache->checked_tailcall_bottom) > - { > - cache->checked_tailcall_bottom = 1; > - dwarf2_tailcall_sniffer_first (this_frame, &cache->tailcall_cache, > - (cache->entry_cfa_sp_offset_p > - ? &cache->entry_cfa_sp_offset : NULL)); > - } > - > /* Non-bottom frames of a virtual tail call frames chain use > dwarf2_tailcall_frame_unwind unwinder so this code does not apply for > them. If dwarf2_tailcall_prev_register_first does not have specific value > @@ -1410,10 +1396,6 @@ static const struct frame_unwind dwarf2_signal_frame_unwind = > void > dwarf2_append_unwinders (struct gdbarch *gdbarch) > { > - /* TAILCALL_FRAME must be first to find the record by > - dwarf2_tailcall_sniffer_first. */ > - frame_unwind_append_unwinder (gdbarch, &dwarf2_tailcall_frame_unwind); > - > frame_unwind_append_unwinder (gdbarch, &dwarf2_frame_unwind); > frame_unwind_append_unwinder (gdbarch, &dwarf2_signal_frame_unwind); > } > diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c > index 35f2e82c57d..3334c472d02 100644 > --- a/gdb/frame-unwind.c > +++ b/gdb/frame-unwind.c > @@ -27,6 +27,7 @@ > #include "gdb_obstack.h" > #include "target.h" > #include "gdbarch.h" > +#include "dwarf2/frame-tailcall.h" > > static struct gdbarch_data *frame_unwind_data; > > @@ -43,6 +44,18 @@ struct frame_unwind_table > struct frame_unwind_table_entry **osabi_head; > }; > > +/* A helper function to add an unwinder to a list. LINK says where to > + install the new unwinder. The new link is returned. */ > + > +static struct frame_unwind_table_entry ** > +add_unwinder (struct obstack *obstack, const struct frame_unwind *unwinder, > + struct frame_unwind_table_entry **link) > +{ > + *link = OBSTACK_ZALLOC (obstack, struct frame_unwind_table_entry); > + (*link)->unwinder = unwinder; > + return &(*link)->next; > +} > + > static void * > frame_unwind_init (struct obstack *obstack) > { > @@ -51,13 +64,21 @@ frame_unwind_init (struct obstack *obstack) > > /* Start the table out with a few default sniffers. OSABI code > can't override this. */ > - table->list = OBSTACK_ZALLOC (obstack, struct frame_unwind_table_entry); > - table->list->unwinder = &dummy_frame_unwind; > - table->list->next = OBSTACK_ZALLOC (obstack, > - struct frame_unwind_table_entry); > - table->list->next->unwinder = &inline_frame_unwind; > + struct frame_unwind_table_entry **link = &table->list; > + > + link = add_unwinder (obstack, &dummy_frame_unwind, link); > + /* The DWARF tailcall sniffer must come before the inline sniffer. > + Otherwise, we can end up in a situation where a DWARF frame finds > + tailcall information, but then the inline sniffer claims a frame > + before the tailcall sniffer, resulting in confusion. This is > + safe to do always because the tailcall sniffer can only ever be > + activated if the newer frame was created using the DWARF > + unwinder, and it also found tailcall information. */ > + link = add_unwinder (obstack, &dwarf2_tailcall_frame_unwind, link); > + link = add_unwinder (obstack, &inline_frame_unwind, link); > + > /* The insertion point for OSABI sniffers. */ > - table->osabi_head = &table->list->next->next; > + table->osabi_head = link; > return table; > } > > -- > 2.21.1 >