public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Simon Marchi <simark@simark.ca>
To: Andrew Burgess <aburgess@redhat.com>, gdb-patches@sourceware.org
Subject: Re: [PATCH 2/2] gdb: add support for disassembler styling using libopcodes
Date: Tue, 19 Jul 2022 22:09:26 -0400	[thread overview]
Message-ID: <60eaedb1-8caf-1a52-0875-e3c5f5c8d8d4@simark.ca> (raw)
In-Reply-To: <eb9059907131fd9acb37ead1b4fc8aa9ff7a1e18.1655462053.git.aburgess@redhat.com>



On 2022-06-17 06:36, Andrew Burgess via Gdb-patches wrote:
> This commit extends GDB to make use of libopcodes styling support
> where available, currently this is just i386 based architectures, and
> RISC-V.
> 
> For architectures that don't support styling using libopcodes GDB will
> fall back to using the Python Pygments package, when the package is
> available.
> 
> The new libopcodes based styling has the disassembler identify parts
> of the disassembled instruction, e.g. registers, immediates,
> mnemonics, etc, and can style these components differently.
> Additionally, as the styling is now done in GDB we can add settings to
> allow the user to configure which colours are used right from the GDB
> CLI.
> 
> There's some new maintenance commands:
> 
>   maintenance set libopcodes-styling enabled on|off
>   maintenance show libopcodes-styling
> 
> These can be used to manually disable use of libopcodes styling.  This
> is a maintenance command as it's not anticipated that a user should
> need to do this.  But, this could be useful for testing, or, in some
> rare cases, a user might want to override the Python hook used for
> disassembler styling, and then disable libopcode styling so that GDB
> falls back to using Python.  Right now I would consider this second
> use case a rare situation, which is why I think a maintenance command
> is appropriate.
> 
> When libopcodes is being used for styling then the user can make use
> of the following new styles:
> 
>   set/show style disassembler comment
>   set/show style disassembler immediate
>   set/show style disassembler mnemonic
>   set/show style disassembler register
> 
> The disassembler also makes use of the 'address' and 'function'
> styles to style some parts of the disassembler output.  I have also
> added the following aliases though:
> 
>   set/show style disassembler address
>   set/show style disassembler symbol
> 
> these are aliases for:
> 
>   set/show style address
>   set/show style function
> 
> respectively, and exist to make it easier for users to discover
> disassembler related style settings.  The 'address' style is used to
> style numeric addresses in the disassembler output, while the 'symbol'
> or 'function' style is used to style the names of symbols in
> disassembler output.
> 
> As not every architecture supports libopcodes styling, the maintenance
> setting 'libopcodes-styling enabled' has an "auto-off" type behaviour.
> Consider this GDB session:
> 
>   (gdb) show architecture
>   The target architecture is set to "auto" (currently "i386:x86-64").
>   (gdb) maintenance show libopcodes-styling enabled
>   Use of libopcodes styling support is "on".
> 
> the setting defaults to "on" for architectures that support libopcodes
> based styling.
> 
>   (gdb) set architecture sparc
>   The target architecture is set to "sparc".
>   (gdb) maintenance show libopcodes-styling enabled
>   Use of libopcodes styling support is "off" (not supported on architecture "sparc")
> 
> the setting will show as "off" if the user switches to an architecture
> that doesn't support libopcodes styling.  The underlying setting is
> still "on" at this point though, if the user switches back to
> i386:x86-64 then the setting would go back to being "on".
> 
>   (gdb) maintenance set libopcodes-styling enabled off
>   (gdb) maintenance show libopcodes-styling enabled
>   Use of libopcodes styling support is "off".
> 
> now the setting is "off" for everyone, even if the user switches back
> to i386:x86-64 the setting will still show as "off".
> 
>   (gdb) maintenance set libopcodes-styling enabled on
>   Use of libopcodes styling not supported on architecture "sparc".
>   (gdb) maintenance show libopcodes-styling enabled
>   Use of libopcodes styling support is "off".
> 
> attempting to switch the setting "on" for an unsupported architecture
> will give an error, and the setting will remain "off".
> 
>   (gdb) set architecture auto
>   The target architecture is set to "auto" (currently "i386:x86-64").
>   (gdb) maintenance show libopcodes-styling enabled
>   Use of libopcodes styling support is "off".
>   (gdb) maintenance set libopcodes-styling enabled on
>   (gdb) maintenance show libopcodes-styling enabled
>   Use of libopcodes styling support is "on".
> 
> the user will need to switch back to a supported architecture before
> they can one again turn this setting "on".
> ---
>  gdb/NEWS            |  28 +++++++
>  gdb/cli/cli-style.c | 107 +++++++++++++++++++++++---
>  gdb/cli/cli-style.h |  28 +++++--
>  gdb/disasm.c        | 184 ++++++++++++++++++++++++++++++++++++++++++--
>  gdb/disasm.h        |  16 ++++
>  gdb/doc/gdb.texinfo | 100 +++++++++++++++++++++++-
>  6 files changed, 437 insertions(+), 26 deletions(-)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index ac9a1aacd34..34a491edd92 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -27,6 +27,12 @@
>    emit to indicate where a breakpoint should be placed to break in a function
>    past its prologue.
>  
> +* Disassembler styling using libopcodes.  GDB now supports
> +  disassembler styling using libopcodes.  This is only available for
> +  some targets (currently x86 and RISC-V).  For unsupported targets
> +  Python Pygments is still used.  For supported targets, libopcodes
> +  styling is used by default.
> +
>  * New commands
>  
>  maintenance set ignore-prologue-end-flag on|off
> @@ -36,6 +42,28 @@ maintenance show ignore-prologue-end-flag
>    used to force GDB to use prologue analyzers if the line-table is constructed
>    from erroneous debug information.
>  
> +maintenance set libopcodes-styling on|off
> +maintenance show libopcodes-styling
> +  These can be used to force off libopcodes based styling, the Python
> +  Pygments styling will then be used instead.
> +
> +set style disassembler comment
> +show style disassembler comment
> +set style disassembler immediate
> +show style disassembler immediate
> +set style disassembler mnemonic
> +show style disassembler mnemonic
> +set style disassembler register
> +show style disassembler register
> +set style disassembler address
> +show style disassembler address
> +set style disassembler symbol
> +show style disassembler symbol
> +  For targets that support libopcodes based styling, these settings
> +  control how various aspects of the disassembler output are styled.
> +  The 'disassembler address' and 'disassembler symbol' styles are
> +  aliases for the 'address' and 'function' styles respectively.
> +
>  * Changed commands
>  
>  maintenance info line-table
> diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c
> index 3fd85f4aa86..2ff1b5815e3 100644
> --- a/gdb/cli/cli-style.c
> +++ b/gdb/cli/cli-style.c
> @@ -110,6 +110,23 @@ cli_style_option version_style ("version", ui_file_style::MAGENTA,
>  
>  /* See cli-style.h.  */
>  
> +cli_style_option disasm_mnemonic_style ("mnemonic", ui_file_style::GREEN);
> +
> +/* See cli-style.h.  */
> +
> +cli_style_option disasm_register_style ("register", ui_file_style::RED);
> +
> +/* See cli-style.h.  */
> +
> +cli_style_option disasm_immediate_style ("immediate", ui_file_style::BLUE);
> +
> +/* See cli-style.h.  */
> +
> +cli_style_option disasm_comment_style ("comment", ui_file_style::WHITE,
> +				       ui_file_style::DIM);
> +
> +/* See cli-style.h.  */
> +
>  cli_style_option::cli_style_option (const char *name,
>  				    ui_file_style::basic_color fg,
>  				    ui_file_style::intensity intensity)
> @@ -224,15 +241,16 @@ cli_style_option::do_show_intensity (struct ui_file *file, int from_tty,
>  
>  /* See cli-style.h.  */
>  
> -void
> +set_show_commands
>  cli_style_option::add_setshow_commands (enum command_class theclass,
>  					const char *prefix_doc,
>  					struct cmd_list_element **set_list,
>  					struct cmd_list_element **show_list,
>  					bool skip_intensity)
>  {
> -  add_setshow_prefix_cmd (m_name, theclass, prefix_doc, prefix_doc,
> -			  &m_set_list, &m_show_list, set_list, show_list);
> +  set_show_commands prefix_cmds
> +    = add_setshow_prefix_cmd (m_name, theclass, prefix_doc, prefix_doc,
> +			      &m_set_list, &m_show_list, set_list, show_list);
>  
>    set_show_commands commands;
>  
> @@ -274,6 +292,8 @@ cli_style_option::add_setshow_commands (enum command_class theclass,
>        commands.set->set_context (this);
>        commands.show->set_context (this);
>      }
> +
> +  return prefix_cmds;
>  }
>  
>  static cmd_list_element *style_set_list;
> @@ -387,11 +407,13 @@ Configure filename colors and display intensity."),
>  					&style_set_list, &style_show_list,
>  					false);
>  
> -  function_name_style.add_setshow_commands (no_class, _("\
> +  set_show_commands function_prefix_cmds
> +    = function_name_style.add_setshow_commands (no_class, _("\
>  Function name display styling.\n\
>  Configure function name colors and display intensity"),
> -					    &style_set_list, &style_show_list,
> -					    false);
> +						&style_set_list,
> +						&style_show_list,
> +						false);
>  
>    variable_name_style.add_setshow_commands (no_class, _("\
>  Variable name display styling.\n\
> @@ -399,11 +421,12 @@ Configure variable name colors and display intensity"),
>  					    &style_set_list, &style_show_list,
>  					    false);
>  
> -  address_style.add_setshow_commands (no_class, _("\
> +  set_show_commands address_prefix_cmds
> +    = address_style.add_setshow_commands (no_class, _("\
>  Address display styling.\n\
>  Configure address colors and display intensity"),
> -				      &style_set_list, &style_show_list,
> -				      false);
> +					  &style_set_list, &style_show_list,
> +					  false);
>  
>    title_style.add_setshow_commands (no_class, _("\
>  Title display styling.\n\
> @@ -451,4 +474,70 @@ Version string display styling.\n\
>  Configure colors used to display the GDB version string."),
>  				      &style_set_list, &style_show_list,
>  				      false);
> +
> +  disasm_mnemonic_style.add_setshow_commands (no_class, _("\
> +Disassembler mnemonic display styling.\n\
> +Configure the colors and display intensity for instruction mnemonics\n\
> +in the disassembler output.  The \"disassembler mnemonic\" style is\n\
> +used to displaying instruction mnemonics as well as any assembler\n\
> +directives, e.g. \".byte\", \".word\", etc.\n\
> +\n\
> +This style will only be used for targets that support libopcodes based\n\
> +disassembler styling.  When Python Pygments based styling is used\n\
> +then this style has no effect."),
> +					      &style_disasm_set_list,
> +					      &style_disasm_show_list,
> +					      false);
> +
> +  disasm_register_style.add_setshow_commands (no_class, _("\
> +Disassembler register display styling.\n\
> +Configure the colors and display intensity for registers in the\n\
> +disassembler output.\n\
> +\n\
> +This style will only be used for targets that support libopcodes based\n\
> +disassembler styling.  When Python Pygments based styling is used\n\
> +then this style has no effect."),
> +					      &style_disasm_set_list,
> +					      &style_disasm_show_list,
> +					      false);
> +
> +  disasm_immediate_style.add_setshow_commands (no_class, _("\
> +Disassembler immediate display styling.\n\
> +Configure the colors and display intensity for immediates in the\n\
> +disassembler output.  The \"disassembler immediate\" style is used for\n\
> +any number that is not an address, this includes constants in arithmetic\n\
> +instructions, as well as address offsets in memory access instructions.\n\
> +\n\
> +This style will only be used for targets that support libopcodes based\n\
> +disassembler styling.  When Python Pygments based styling is used\n\
> +then this style has no effect."),
> +					       &style_disasm_set_list,
> +					       &style_disasm_show_list,
> +					       false);
> +
> +  disasm_comment_style.add_setshow_commands (no_class, _("\
> +Disassembler comment display styling.\n\
> +Configure the colors and display intensity for comments in the\n\
> +disassembler output.  The \"disassembler comment\" style is used for\n\
> +the comment character, and everything after the comment character up to\n\
> +the end of the line.  The comment style overrides any other styling,\n\
> +e.g. a register name in a comment will use the comment styling.\n\
> +\n\
> +This style will only be used for targets that support libopcodes based\n\
> +disassembler styling.  When Python Pygments based styling is used\n\
> +then this style has no effect."),
> +					     &style_disasm_set_list,
> +					     &style_disasm_show_list,
> +					     false);
> +
> +  /* Setup 'disassembler address' style and 'disassembler symbol' style,
> +     these are aliases for 'address' and 'function' styles respectively.  */
> +  add_alias_cmd ("address", address_prefix_cmds.set, no_class, 0,
> +		 &style_disasm_set_list);
> +  add_alias_cmd ("address", address_prefix_cmds.show, no_class, 0,
> +		 &style_disasm_show_list);
> +  add_alias_cmd ("symbol", function_prefix_cmds.set, no_class, 0,
> +		 &style_disasm_set_list);
> +  add_alias_cmd ("symbol", function_prefix_cmds.show, no_class, 0,
> +		 &style_disasm_show_list);
>  }
> diff --git a/gdb/cli/cli-style.h b/gdb/cli/cli-style.h
> index f69df47098c..4090cf01333 100644
> --- a/gdb/cli/cli-style.h
> +++ b/gdb/cli/cli-style.h
> @@ -43,12 +43,13 @@ class cli_style_option
>    /* Return the style name.  */
>    const char *name () { return m_name; };
>  
> -  /* Call once to register this CLI style with the CLI engine.  */
> -  void add_setshow_commands (enum command_class theclass,
> -			     const char *prefix_doc,
> -			     struct cmd_list_element **set_list,
> -			     struct cmd_list_element **show_list,
> -			     bool skip_intensity);
> +  /* Call once to register this CLI style with the CLI engine.  Returns
> +     the set/show prefix commands for the style.  */
> +  set_show_commands add_setshow_commands (enum command_class theclass,
> +					  const char *prefix_doc,
> +					  struct cmd_list_element **set_list,
> +					  struct cmd_list_element **show_list,
> +					  bool skip_intensity);
>  
>    /* Return the 'set style NAME' command list, that can be used
>       to build a lambda DO_SET to call add_setshow_commands.  */
> @@ -116,6 +117,21 @@ extern cli_style_option title_style;
>  /* The metadata style.  */
>  extern cli_style_option metadata_style;
>  
> +/* The disassembler style for mnemonics or assembler directives
> +   (e.g. '.byte', etc).  */
> +extern cli_style_option disasm_mnemonic_style;
> +
> +/* The disassembler style for register names.  */
> +extern cli_style_option disasm_register_style;
> +
> +/* The disassembler style for numerical values that are not addresses, this
> +   includes immediate operands (e.g. in, an add instruction), but also
> +   address offsets (e.g. in a load instruction).  */
> +extern cli_style_option disasm_immediate_style;
> +
> +/* The disassembler style for comments.  */
> +extern cli_style_option disasm_comment_style;
> +
>  /* The border style of a TUI window that does not have the focus.  */
>  extern cli_style_option tui_border_style;
>  
> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index 6c64c14feee..cf27a32fbe7 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -41,6 +41,63 @@
>     which is set by the "set disassembler_options" command.  */
>  static std::string prospective_options;
>  
> +/* When this is true we will try to use libopcodes to provide styling to
> +   the disassembler output.  */
> +
> +static bool use_libopcodes_styling = true;
> +
> +/* To support the set_use_libopcodes_styling function we have a second
> +   variable which is connected to the actual set/show option.  */
> +
> +static bool use_libopcodes_styling_option = use_libopcodes_styling;
> +
> +/* The "maint show libopcodes-styling enabled" command.  */
> +
> +static void
> +show_use_libopcodes_styling  (struct ui_file *file, int from_tty,
> +			      struct cmd_list_element *c,
> +			      const char *value)
> +{
> +  gdb_non_printing_memory_disassembler dis (target_gdbarch ());
> +  bool supported = dis.disasm_info ()->created_styled_output;
> +
> +  if (supported || !use_libopcodes_styling)
> +    gdb_printf (file, _("Use of libopcodes styling support is \"%s\".\n"),
> +		value);
> +  else
> +    {
> +      /* Use of libopcodes styling is not supported, and the user has this
> +	 turned on!  */
> +      gdb_printf (file, _("Use of libopcodes styling support is \"off\""
> +			  " (not supported on architecture \"%s\")\n"),
> +		  gdbarch_bfd_arch_info (target_gdbarch ())->printable_name);
> +    }
> +}
> +
> +/* The "maint set libopcodes-styling enabled" command.  */
> +
> +static void
> +set_use_libopcodes_styling (const char *args, int from_tty,
> +			    struct cmd_list_element *c)
> +{
> +  gdb_non_printing_memory_disassembler dis (target_gdbarch ());
> +  bool supported = dis.disasm_info ()->created_styled_output;
> +
> +  /* If the current architecture doesn't support libopcodes styling then we
> +     give an error here, but leave the underlying setting enabled.  This
> +     means that if the user switches to an architecture that does support
> +     libopcodes styling the setting will be enabled.  */
> +
> +  if (use_libopcodes_styling_option && !supported)
> +    {
> +      use_libopcodes_styling_option = use_libopcodes_styling;
> +      error (_("Use of libopcodes styling not supported on architecture \"%s\"."),
> +	     gdbarch_bfd_arch_info (target_gdbarch ())->printable_name);
> +    }
> +  else
> +    use_libopcodes_styling = use_libopcodes_styling_option;
> +}
> +
>  /* This structure is used to store line number information for the
>     deprecated /m option.
>     We need a different sort of line table from the normal one cuz we can't
> @@ -160,7 +217,26 @@ gdb_disassembler::dis_asm_print_address (bfd_vma addr,
>    gdb_disassembler *self
>      = static_cast<gdb_disassembler *>(info->application_data);
>  
> -  print_address (self->arch (), addr, self->stream ());
> +  if (self->in_comment_p ())
> +    {
> +      /* Calling 'print_address' might add styling to the output (based on
> +	 the properties of the stream we're writing too).  This is usually
> +	 fine, but if we are in an assembler comment then we'd prefer to
> +	 have the comment style, rather than the default address style.
> +
> +	 Print the address into a temporary buffer which doesn't support
> +	 styling, then reprint this unstyled address with the default text
> +	 style.
> +
> +	 As we are inside a comment right now, the standard print routine
> +	 will ensure that the comment is printed to the user with a
> +	 suitable comment style.  */
> +      string_file tmp;
> +      print_address (self->arch (), addr, &tmp);
> +      self->fprintf_styled_func (self, dis_style_text, "%s", tmp.c_str ());
> +    }
> +  else
> +    print_address (self->arch (), addr, self->stream ());
>  }
>  
>  /* See disasm.h.  */
> @@ -202,11 +278,54 @@ gdb_printing_disassembler::fprintf_styled_func (void *dis_info,
>  						const char *format, ...)
>  {
>    ui_file *stream = stream_from_gdb_disassemble_info (dis_info);
> +  gdb_printing_disassembler *dis = (gdb_printing_disassembler *) dis_info;
>  
>    va_list args;
>    va_start (args, format);
> -  gdb_vprintf (stream, format, args);
> +  std::string content = string_vprintf (format, args);
>    va_end (args);
> +
> +  /* Once in a comment then everything should be styled as a comment.  */
> +  if (style == dis_style_comment_start)
> +    dis->set_in_comment (true);
> +  if (dis->in_comment_p ())
> +    style = dis_style_comment_start;
> +
> +  /* Now print the content with the correct style.  */
> +  const char *txt = content.c_str ();
> +  switch (style)
> +    {
> +    case dis_style_mnemonic:
> +    case dis_style_assembler_directive:
> +      fputs_styled (txt, disasm_mnemonic_style.style (), stream);
> +      break;
> +
> +    case dis_style_register:
> +      fputs_styled (txt, disasm_register_style.style (), stream);
> +      break;
> +
> +    case dis_style_immediate:
> +    case dis_style_address_offset:
> +      fputs_styled (txt, disasm_immediate_style.style (), stream);
> +      break;
> +
> +    case dis_style_address:
> +      fputs_styled (txt, address_style.style (), stream);
> +      break;
> +
> +    case dis_style_symbol:
> +      fputs_styled (txt, function_name_style.style (), stream);
> +      break;
> +
> +    case dis_style_comment_start:
> +      fputs_styled (txt, disasm_comment_style.style (), stream);
> +      break;
> +
> +    case dis_style_text:
> +      gdb_puts (txt, stream);
> +      break;
> +    }
> +
>    /* Something non -ve.  */
>    return 0;
>  }
> @@ -818,7 +937,20 @@ gdb_disassembler::gdb_disassembler (struct gdbarch *gdbarch,
>  				    read_memory_ftype func)
>    : gdb_printing_disassembler (gdbarch, &m_buffer, func,
>  			       dis_asm_memory_error, dis_asm_print_address),
> -    m_buffer (!use_ext_lang_colorization_p && disassembler_styling
> +    /* The use of m_di.created_styled_output here is a bit of a cheat, but
> +       it works fine for now.  Currently, for all targets that support
> +       libopcodes styling, this field is set during the call to
> +       disassemble_init_for_target, which was called as part of the
> +       initialization of gdb_printing_disassembler.  And so, we are able to
> +       spot if a target supports libopcodes styling, and create m_buffer in
> +       the correct styling mode.
> +
> +       If there's ever a target that only sets created_styled_output during
> +       the actual disassemble phase, then the logic here will have to
> +       change.  */
> +    m_buffer ((!use_ext_lang_colorization_p
> +	       || (use_libopcodes_styling && m_di.created_styled_output))
> +	      && disassembler_styling
>  	      && file->can_emit_style_escape ()),
>      m_dest (file)
>  { /* Nothing.  */ }
> @@ -903,14 +1035,17 @@ gdb_disassembler::print_insn (CORE_ADDR memaddr,
>  {
>    m_err_memaddr.reset ();
>    m_buffer.clear ();
> +  this->set_in_comment (false);
>  
>    int length = gdb_print_insn_1 (arch (), memaddr, &m_di);
>  
> -  /* If we have successfully disassembled an instruction, styling is on, we
> -     think that the extension language might be able to perform styling for
> -     us, and the destination can support styling, then lets call into the
> -     extension languages in order to style this output.  */
> +  /* If we have successfully disassembled an instruction, disassembler
> +     styling using the extension language is on, and libopcodes hasn't
> +     already styled the output for us, and, if the destination can support
> +     styling, then lets call into the extension languages in order to style
> +     this output.  */
>    if (length > 0 && disassembler_styling
> +      && (!m_di.created_styled_output || !use_libopcodes_styling)
>        && use_ext_lang_colorization_p
>        && m_dest->can_emit_style_escape ())
>      {
> @@ -1311,4 +1446,39 @@ Show the disassembler options."), NULL,
>  					 show_disassembler_options_sfunc,
>  					 &setlist, &showlist);
>    set_cmd_completer (set_show_disas_opts.set, disassembler_options_completer);
> +
> +
> +  /* All the 'maint set|show libopcodes-styling' sub-commands.  */
> +  static struct cmd_list_element *maint_set_libopcodes_styling_cmdlist;
> +  static struct cmd_list_element *maint_show_libopcodes_styling_cmdlist;
> +
> +  /* Adds 'maint set|show libopcodes-styling'.  */
> +  add_setshow_prefix_cmd ("libopcodes-styling", class_maintenance,
> +			  _("Set libopcodes-styling specific variables."),
> +			  _("Show libopcodes-styling specific variables."),
> +			  &maint_set_libopcodes_styling_cmdlist,
> +			  &maint_show_libopcodes_styling_cmdlist,
> +			  &maintenance_set_cmdlist,
> +			  &maintenance_show_cmdlist);
> +
> +  /* Adds 'maint set|show gnu-source-highlight enabled'.  */
> +  add_setshow_boolean_cmd ("enabled", class_maintenance,
> +			   &use_libopcodes_styling_option, _("\
> +Set whether the libopcodes styling support should be used."), _("\
> +Show whether the libopcodes styling support should be used."),_("\
> +When enabled, GDB will try to make use of the builtin libopcodes styling\n\
> +support, to style the disassembler output.  Not every architecture has\n\
> +styling support within libopcodes, so enabling this is not a guarantee\n\
> +that libopcodes styling will be available.\n\
> +\n\
> +When this option is disabled, GDB will make use of the Python Pygments\n\
> +package (if available) to style the disassembler output.\n\
> +\n\
> +All disassembler styling can be disabled with:\n\
> +\n\
> +  set style disassembler enabled off"),
> +			   set_use_libopcodes_styling,
> +			   show_use_libopcodes_styling,
> +			   &maint_set_libopcodes_styling_cmdlist,
> +			   &maint_show_libopcodes_styling_cmdlist);
>  }
> diff --git a/gdb/disasm.h b/gdb/disasm.h
> index 54176eb095a..2921d537e0a 100644
> --- a/gdb/disasm.h
> +++ b/gdb/disasm.h
> @@ -138,6 +138,16 @@ struct gdb_printing_disassembler : public gdb_disassemble_info
>  				  const char *format, ...)
>      ATTRIBUTE_PRINTF(3,4);
>  
> +  /* Return true if the disassembler is considered inside a comment, false
> +     otherwise.  */
> +  bool in_comment_p () const
> +  { return m_in_comment; }
> +
> +  /* Set whether the disassembler should be considered as within comment
> +     text or not.  */
> +  void set_in_comment (bool c)
> +  { m_in_comment = c; }

My GDB built with UBSan fails gdb.python/py-disasm.exp with:

    (gdb) PASS: gdb.python/py-disasm.exp: global_disassembler=GlobalPreInfoDisassembler: python add_global_disassembler(GlobalPreInfoDisassembler)
    disassemble main
    Dump of assembler code for function main:
       0x0000555555555119 <+0>:     push   %rbp
       0x000055555555511a <+1>:     mov    %rsp,%rbp
       0x000055555555511d <+4>:     nop
    /home/simark/src/binutils-gdb/gdb/disasm.h:144:12: runtime error: load of value 118, which is not a valid value for type 'bool'

I tracked it to m_in_comment not being initialized, and some code
calling in_comment_p without anything setting m_in_comment.  The
backtrace is:

    #0  __sanitizer::Die () at /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_termination.cpp:50
    #1  0x00007f9a2220e8ad in __ubsan::__ubsan_handle_load_invalid_value_abort (Data=<optimized out>, Val=<optimized out>) at /usr/src/debug/gcc/libsanitizer/ubsan/ubsan_handlers.cpp:551
    #2  0x000055d15b4fcb51 in gdb_printing_disassembler::in_comment_p (this=0x7ffde7aadc20) at /home/simark/src/binutils-gdb/gdb/disasm.h:144
    #3  0x000055d15b4e9366 in gdb_printing_disassembler::fprintf_styled_func (dis_info=0x7ffde7aadc20, style=dis_style_mnemonic, format=0x55d15f8c4760 "%.*s") at /home/simark/src/binutils-gdb/gdb/disasm.c:291
    #4  0x000055d15d6366d7 in i386_dis_printf (ins=0x7ffde7aad580, style=dis_style_mnemonic, fmt=0x55d15f8c4dc0 "%s%*s") at /home/simark/src/binutils-gdb/opcodes/i386-dis.c:9302
    #5  0x000055d15d63b83e in print_insn (pc=93824992235806, info=0x7ffde7aadc28, intel_syntax=-1) at /home/simark/src/binutils-gdb/opcodes/i386-dis.c:9818
    #6  0x000055d15d63c9aa in print_insn_i386 (pc=93824992235806, info=0x7ffde7aadc28) at /home/simark/src/binutils-gdb/opcodes/i386-dis.c:9924
    #7  0x000055d15ab99628 in default_print_insn (memaddr=93824992235806, info=0x7ffde7aadc28) at /home/simark/src/binutils-gdb/gdb/arch-utils.c:1041
    #8  0x000055d15bde7f1b in i386_print_insn (pc=93824992235806, info=0x7ffde7aadc28) at /home/simark/src/binutils-gdb/gdb/i386-tdep.c:4073
    #9  0x000055d15abba930 in gdbarch_print_insn (gdbarch=0x621000192910, vma=93824992235806, info=0x7ffde7aadc28) at /home/simark/src/binutils-gdb/gdb/gdbarch.c:3348
    #10 0x000055d15c73ba57 in disasmpy_builtin_disassemble (self=0x7f9a14df3ab0, args=0x7f9a14d281c0, kw=0x0) at /home/simark/src/binutils-gdb/gdb/python/py-disasm.c:294
    #11 0x00007f9a23155511 in ?? () from /usr/lib/libpython3.10.so.1.0
    #12 0x00007f9a2314edeb in _PyObject_MakeTpCall () from /usr/lib/libpython3.10.so.1.0
    #13 0x00007f9a2314a19a in _PyEval_EvalFrameDefault () from /usr/lib/libpython3.10.so.1.0
    #14 0x00007f9a231559c9 in _PyFunction_Vectorcall () from /usr/lib/libpython3.10.so.1.0

I presume we just need to initialize it to false.  It makes the test
case pass here at least.

Simon

  parent reply	other threads:[~2022-07-20  2:09 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-17 10:36 [PATCH 0/2] Use libopcodes disassembler styling with GDB Andrew Burgess
2022-06-17 10:36 ` [PATCH 1/2] gdb: have gdb_disassemble_info carry 'this' in its stream pointer Andrew Burgess
2022-06-17 10:36 ` [PATCH 2/2] gdb: add support for disassembler styling using libopcodes Andrew Burgess
2022-06-17 11:16   ` Eli Zaretskii
2022-06-30 12:22     ` Andrew Burgess
2022-06-30 13:46       ` Eli Zaretskii
2022-07-04 10:15         ` Andrew Burgess
2022-07-20  2:09   ` Simon Marchi [this message]
2022-07-20 13:14     ` [PATCH 0/2] Fix some undefined haviour bugs in Python disassembler API Andrew Burgess
2022-07-20 13:14       ` [PATCH 1/2] gdb: fix use of uninitialised gdb_printing_disassembler::m_in_comment Andrew Burgess
2022-07-20 14:21         ` Simon Marchi
2022-07-20 13:14       ` [PATCH 2/2] gdb/python: fix invalid use disassemble_info::stream Andrew Burgess
2022-07-25 18:27       ` [PATCH 0/2] Fix some undefined haviour bugs in Python disassembler API Andrew Burgess
2022-07-04 10:17 ` [PATCH 0/2] Use libopcodes disassembler styling with GDB Andrew Burgess
2022-07-11 14:18   ` Andrew Burgess

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=60eaedb1-8caf-1a52-0875-e3c5f5c8d8d4@simark.ca \
    --to=simark@simark.ca \
    --cc=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).