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

[Simon Marchi <simark@simark.ca>]

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.

> +
>  \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