[PP?] [PATCH v3] GDB PR tdep/8282: MIPS: Wire in `set disassembler-options'
Maciej W. Rozycki
macro@mips.com
Wed Jun 20 15:40:00 GMT 2018
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
More information about the Binutils
mailing list