Re: [PATCH] gdb: Add switch to disable DWARF stack unwinders

[Andrew Burgess <andrew.burgess@embecosm.com>]

Simon,

Thanks for your review.  It all looks great, except for one follow up
question I have which is inline below...

* Simon Marchi <simark@simark.ca> [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);
> >  \f
> >  
> > +/* 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;
> >  }
> >  \f
> >  
> > 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 <forward_list>
> >  #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?

> 
> > +
> >  \f
> >  /* 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