[PP?,v3] GDB PR tdep/8282: MIPS: Wire in `set disassembler-options'

  Hi Maciej,

I just have one comment on the memory management on the GDB side.

On 2018-06-14 06:08 PM, Maciej W. Rozycki wrote:
> Existing affected target backends have been adjusted accordingly.  The 
> only other backend (besides MIPS, now modernized) setting disassembler 
> options in its own specific way turned out to be the i386 one.  As its 
> `i386_print_insn' handler is always passed a NULL `disassembler_options' 
> value, assert that and clear the pointer after use, so that code in 
> `gdb_buffered_insn_length' doesn't choke on `free' being called on a 
> static data pointer.

Having a field sometimes own the string and sometimes don't is a recipe for
confusion.  It would be better to make get_all_disassembler_options return a
gdb::unique_xmalloc_ptr instead [1], and propagate to whatever object/scope really
owns the string.  See the patch at the bottom implementing this idea (applied on
top of this one).

[1] Or an std::string. but that does not play well with remove_whitespace_and_extra_commas.
    Can we avoid calling remove_whitespace_and_extra_commas by not putting the comma if it
    is not necessary?

> Index: binutils/gdb/disasm.c
> ===================================================================
> --- binutils.orig/gdb/disasm.c	2018-06-13 16:00:05.000000000 +0100
> +++ binutils/gdb/disasm.c	2018-06-14 21:45:24.771254073 +0100
> @@ -722,6 +722,35 @@ fprintf_disasm (void *stream, const char
>    return 0;
>  }
>  
> +/* Combine implicit and user disassembler options and return them
> +   in a newly-allocated buffer.  Return NULL if the string would be
> +   empty.  */
> +
> +static const char *
> +get_all_disassembler_options (struct gdbarch *gdbarch)
> +{
> +  const char *implicit_options;
> +  const char *options;
> +  char *all_options;
> +  size_t len;
> +
> +  implicit_options = gdbarch_disassembler_options_implicit (gdbarch);
> +  options = get_disassembler_options (gdbarch);
> +  len = ((implicit_options != NULL ? strlen (implicit_options) : 0) + 1
> +	 + (options != NULL ? strlen (options) : 0) + 1);
> +  all_options = (char *) xmalloc (len);
> +  sprintf (all_options, "%s,%s",
> +	   implicit_options != NULL ? implicit_options : "",
> +	   options != NULL ? options : "");

It would be better to use xstrprintf instead of computing the required length
by hand.


From a5239e3ffb381334275a26b7d0b162c4aef5745b Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sun, 17 Jun 2018 22:07:41 -0400
Subject: [PATCH] Suggested changes

---
 gdb/disasm.c    | 68 ++++++++++++++++++++++---------------------------
 gdb/disasm.h    |  7 +++--
 gdb/i386-tdep.c | 10 +-------
 3 files changed, 36 insertions(+), 49 deletions(-)
  

Hi Simon,

> > Existing affected target backends have been adjusted accordingly.  The 
> > only other backend (besides MIPS, now modernized) setting disassembler 
> > options in its own specific way turned out to be the i386 one.  As its 
> > `i386_print_insn' handler is always passed a NULL `disassembler_options' 
> > value, assert that and clear the pointer after use, so that code in 
> > `gdb_buffered_insn_length' doesn't choke on `free' being called on a 
> > static data pointer.
> 
> Having a field sometimes own the string and sometimes don't is a recipe for
> confusion.

 Indeed, however I decided not to complicate the thing further under an 
assumption (perhaps an unjustified one) that the i386 backend will 
eventually get converted to support `set disassembler-options ...' as 
well.  At which point the hack would go away naturally.

>  It would be better to make get_all_disassembler_options return a
> gdb::unique_xmalloc_ptr instead [1], and propagate to whatever object/scope really
> owns the string.  See the patch at the bottom implementing this idea (applied on
> top of this one).

 If you however think it's worth an extra effort to support the interim 
situation with the i386 backend, then I can't object.  Thanks for this 
update.

> [1] Or an std::string. but that does not play well with remove_whitespace_and_extra_commas.
>     Can we avoid calling remove_whitespace_and_extra_commas by not putting the comma if it
>     is not necessary?

 Yeah, why not.

> > Index: binutils/gdb/disasm.c
> > ===================================================================
> > --- binutils.orig/gdb/disasm.c	2018-06-13 16:00:05.000000000 +0100
> > +++ binutils/gdb/disasm.c	2018-06-14 21:45:24.771254073 +0100
> > @@ -722,6 +722,35 @@ fprintf_disasm (void *stream, const char
> >    return 0;
> >  }
> >  
> > +/* Combine implicit and user disassembler options and return them
> > +   in a newly-allocated buffer.  Return NULL if the string would be
> > +   empty.  */
> > +
> > +static const char *
> > +get_all_disassembler_options (struct gdbarch *gdbarch)
> > +{
> > +  const char *implicit_options;
> > +  const char *options;
> > +  char *all_options;
> > +  size_t len;
> > +
> > +  implicit_options = gdbarch_disassembler_options_implicit (gdbarch);
> > +  options = get_disassembler_options (gdbarch);
> > +  len = ((implicit_options != NULL ? strlen (implicit_options) : 0) + 1
> > +	 + (options != NULL ? strlen (options) : 0) + 1);
> > +  all_options = (char *) xmalloc (len);
> > +  sprintf (all_options, "%s,%s",
> > +	   implicit_options != NULL ? implicit_options : "",
> > +	   options != NULL ? options : "");
> 
> It would be better to use xstrprintf instead of computing the required length
> by hand.

 Good point, I didn't know of this one (or for that matter, the underlying 
`asprintf'/`vasprintf' functions, which are a GNU extension).  Thanks.

 I'll fold your change into my proposal and rewrite the necessary bits to 
use `std::string'.  Thank you for your review.

  Maciej