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

["Maciej W. Rozycki" <macro@mips.com>]

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