From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 62245 invoked by alias); 14 Jul 2018 20:35:18 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 62233 invoked by uid 89); 14 Jul 2018 20:35:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=HX-Received:sk:w5-v6mr X-HELO: mail-wr1-f66.google.com Received: from mail-wr1-f66.google.com (HELO mail-wr1-f66.google.com) (209.85.221.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 14 Jul 2018 20:35:15 +0000 Received: by mail-wr1-f66.google.com with SMTP id a3-v6so18967630wrt.2 for ; Sat, 14 Jul 2018 13:35:15 -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:user-agent; bh=T7Zrb3z9uCQJFtUg69+XAgfl9HpiB4ef73El0cL2244=; b=JMyRgIdlgzeP1IC2FVaw4WjokhVoiV+uy/XzsLk8KC/ynIBgFPGc7U92nAyYnCjY0r 6JVsQ9k97IGVQ3XScovDmL+ixWru7woCCpavkP3FKHw6ziQjghNbhnC4tjvKqeY8f532 dYo5RCvsGiVpz5dOlqo0yOkcfcA/wSVReDdj0x5xKGke4xgtMvrCKqPAPItctqqHMJnO l0No8zyaaxcYN/BYFiWk1Jeoo4EXfae/iB2D++R2Muo31pymJkxI9XzzaZmM7Q4bFbRq UivU1HjfKcUPVPRs0I/VvilE1zIJSkSIuM95/sB9I8poGsv+wI0dAzrgSPYEh6l446g9 /Mhw== Return-Path: Received: from localhost (host86-164-85-3.range86-164.btcentralplus.com. [86.164.85.3]) by smtp.gmail.com with ESMTPSA id i1-v6sm12416554wrq.69.2018.07.14.13.35.12 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 14 Jul 2018 13:35:12 -0700 (PDT) Date: Sat, 14 Jul 2018 20:35:00 -0000 From: Andrew Burgess To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: Add switch to disable DWARF stack unwinders Message-ID: <20180714203510.GC2888@embecosm.com> References: <20180713134629.27011-1-andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Fortune: If we all work together, we can totally disrupt the system. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-07/txt/msg00454.txt.bz2 Simon, Thanks for your review. It all looks great, except for one follow up question I have which is inline below... * Simon Marchi [2018-07-13 21:11:38 -0400]: > Hi Andrew, > > On 2018-07-13 09:46 AM, Andrew Burgess wrote: > > diff --git a/gdb/dwarf2-frame-tailcall.c b/gdb/dwarf2-frame-tailcall.c > > index 1d3e1f445bb..f565a2eecc8 100644 > > --- a/gdb/dwarf2-frame-tailcall.c > > +++ b/gdb/dwarf2-frame-tailcall.c > > @@ -318,6 +318,9 @@ tailcall_frame_sniffer (const struct frame_unwind *self, > > int next_levels; > > struct tailcall_cache *cache; > > > > + if (!dwarf2_frame_unwinders_enabled_p) > > + return 0; > > + > > /* Inner tail call element does not make sense for a sentinel frame. */ > > next_frame = get_next_frame (this_frame); > > if (next_frame == NULL) > > diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c > > index 91e16cf024e..fdd41b360e6 100644 > > --- a/gdb/dwarf2-frame.c > > +++ b/gdb/dwarf2-frame.c > > @@ -169,6 +169,9 @@ static CORE_ADDR read_encoded_value (struct comp_unit *unit, gdb_byte encoding, > > CORE_ADDR func_base); > > > > > > +/* See dwarf2-frame.h. */ > > +int dwarf2_frame_unwinders_enabled_p = 0; > > Is there any reason why you initialize this to 0, and set it to 1 in > dwarf2_append_unwinders? It can produce some quite unexpected results, IMO: > > (gdb) maintenance set dwarf unwinders off > (gdb) maintenance show dwarf unwinders > The DWARF stack unwinders are currently off. > (gdb) file test > Reading symbols from test...done. > (gdb) maintenance show dwarf unwinders > The DWARF stack unwinders are currently on. > > Is there something wrong with initializing it to 1 directly and not touching > it after? > > > + > > /* Store the length the expression for the CFA in the `cfa_reg' field, > > which is unused in that case. */ > > #define cfa_exp_len cfa_reg > > @@ -1326,6 +1329,9 @@ static int > > dwarf2_frame_sniffer (const struct frame_unwind *self, > > struct frame_info *this_frame, void **this_cache) > > { > > + if (!dwarf2_frame_unwinders_enabled_p) > > + return 0; > > + > > /* Grab an address that is guarenteed to reside somewhere within the > > function. get_frame_pc(), with a no-return next function, can > > end up returning something past the end of this function's body. > > @@ -1389,6 +1395,9 @@ dwarf2_append_unwinders (struct gdbarch *gdbarch) > > > > frame_unwind_append_unwinder (gdbarch, &dwarf2_frame_unwind); > > frame_unwind_append_unwinder (gdbarch, &dwarf2_signal_frame_unwind); > > + > > + /* Mark dwarf frame unwinders as enabled. */ > > + dwarf2_frame_unwinders_enabled_p = 1; > > } > > > > > > diff --git a/gdb/dwarf2-frame.h b/gdb/dwarf2-frame.h > > index 471281a2c3f..56c90be8ceb 100644 > > --- a/gdb/dwarf2-frame.h > > +++ b/gdb/dwarf2-frame.h > > @@ -210,6 +210,12 @@ struct dwarf2_frame_state > > bool armcc_cfa_offsets_reversed = false; > > }; > > > > +/* When this is true the dwarf frame unwinders can be used if they are > > + registered with the gdbarch. Not all architectures can or do use the > > + dwarf unwinders. Setting this to true on a target that does not > > + otherwise support the dwarf unwinders has no effect. */ > > dwarf -> DWARF > > > +extern int dwarf2_frame_unwinders_enabled_p; > > + > > /* Set the architecture-specific register state initialization > > function for GDBARCH to INIT_REG. */ > > > > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > > index 372f45ee175..b49d4d9fbbc 100644 > > --- a/gdb/dwarf2read.c > > +++ b/gdb/dwarf2read.c > > @@ -90,6 +90,7 @@ > > #include > > #include "rust-lang.h" > > #include "common/pathstuff.h" > > +#include "dwarf2-frame.h" > > > > /* When == 1, print basic high level tracing messages. > > When > 1, be more verbose. > > @@ -1423,6 +1424,19 @@ show_dwarf_max_cache_age (struct ui_file *file, int from_tty, > > "DWARF compilation units is %s.\n"), > > value); > > } > > + > > +/* Handle 'maintenance show dwarf unwinders'. */ > > + > > +static void > > +show_dwarf_unwinders_enabled_p (struct ui_file *file, int from_tty, > > + struct cmd_list_element *c, > > + const char *value) > > +{ > > + fprintf_filtered (file, > > + _("The DWARF stack unwinders are currently %s.\n"), > > + value); > > +} > > I don't think this is necessary, if you leave the "show" callback to NULL, > I think the message is clear enough: > > (gdb) maintenance show dwarf unwinders > Whether the DWARF stack frame unwinders are used is on. I thought that was no longer the approved approach as the resulting string is generated and not internationalised? > > > + > > > > /* local function prototypes */ > > > > @@ -25363,6 +25377,18 @@ conversational style, when possible."), > > &set_dwarf_cmdlist, > > &show_dwarf_cmdlist); > > > > + add_setshow_boolean_cmd ("unwinders", class_obscure, > > + &dwarf2_frame_unwinders_enabled_p , _("\ > > +Set whether the DWARF stack frame unwinders are used."), _("\ > > +Show whether the DWARF stack frame unwinders are used."), _("\ > > +When enabled the DWARF stack frame unwinders can be used for architectures\n\ > > +that support the DWARF unwinders. Enabling the dwarf unwinders for an\n\ > > +architecture that doesn't support them will have no effect."), > > + NULL, > > + show_dwarf_unwinders_enabled_p, > > + &set_dwarf_cmdlist, > > + &show_dwarf_cmdlist); > > + > > I think it would make sense to have the command registration in the same file as > dwarf2_frame_unwinders_enabled_p (dwarf2-frame.c). set_dwarf_cmdlist/show_dwarf_cmdlist > would need to be exported though. I guess their declarations can go in dwarf2read.h, > since they are defined in dwarf2read.c. > > Simon Thanks, Andrew