From patchwork Mon Jun 18 02:58:51 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 27899 Received: (qmail 81360 invoked by alias); 18 Jun 2018 02:59:08 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 81342 invoked by uid 89); 18 Jun 2018 02:59:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 spammy=Wire, H*r:8.14.7 X-Spam-User: qpsmtpd, 2 recipients X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 18 Jun 2018 02:59:06 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id w5I2wq4V029801 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 17 Jun 2018 22:58:57 -0400 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id C345B1E529; Sun, 17 Jun 2018 22:58:51 -0400 (EDT) Subject: Re: [PP?] [PATCH v3] GDB PR tdep/8282: MIPS: Wire in `set disassembler-options' To: "Maciej W. Rozycki" , gdb-patches@sourceware.org, binutils@sourceware.org Cc: Joel Brobecker , Fredrik Noring References: From: Simon Marchi Message-ID: <158bad5a-3a2d-cc7f-4a77-71135a93f0ac@polymtl.ca> Date: Sun, 17 Jun 2018 22:58:51 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 18 Jun 2018 02:58:52 +0000 X-IsSubscribed: yes 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 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(-) diff --git a/gdb/disasm.c b/gdb/disasm.c index 991214598e5f..cc37c01459dd 100644 --- a/gdb/disasm.c +++ b/gdb/disasm.c @@ -726,29 +726,24 @@ fprintf_disasm (void *stream, const char *format, ...) in a newly-allocated buffer. Return NULL if the string would be empty. */ -static const char * +static gdb::unique_xmalloc_ptr 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 : ""); - if (remove_whitespace_and_extra_commas (all_options) == NULL) - { - xfree (all_options); - return NULL; - } - else + const char *implicit_options = gdbarch_disassembler_options_implicit (gdbarch); + if (implicit_options == nullptr) + implicit_options = ""; + + const char *options = get_disassembler_options (gdbarch); + if (options == nullptr) + options = ""; + + gdb::unique_xmalloc_ptr all_options + (xstrprintf ("%s,%s", implicit_options, options)); + + if (remove_whitespace_and_extra_commas (all_options.get ()) != nullptr) return all_options; + else + return nullptr; } gdb_disassembler::gdb_disassembler (struct gdbarch *gdbarch, @@ -775,15 +770,11 @@ gdb_disassembler::gdb_disassembler (struct gdbarch *gdbarch, m_di.endian = gdbarch_byte_order (gdbarch); m_di.endian_code = gdbarch_byte_order_for_code (gdbarch); m_di.application_data = this; - m_di.disassembler_options = get_all_disassembler_options (gdbarch); + m_disassembler_options_holder = get_all_disassembler_options (gdbarch); + m_di.disassembler_options = m_disassembler_options_holder.get (); disassemble_init_for_target (&m_di); } -gdb_disassembler::~gdb_disassembler () -{ - xfree (const_cast (m_di.disassembler_options)); -} - int gdb_disassembler::print_insn (CORE_ADDR memaddr, int *branch_delay_insns) @@ -867,13 +858,15 @@ gdb_buffered_insn_length_fprintf (void *stream, const char *format, ...) return 0; } -/* Initialize a struct disassemble_info for gdb_buffered_insn_length. */ +/* Initialize a struct disassemble_info for gdb_buffered_insn_length. + Upon return, *DISASSEMBLER_OPTIONS_HOLDER owns the string pointed + to by DI.DISASSEMBLER_OPTIONS. */ static void -gdb_buffered_insn_length_init_dis (struct gdbarch *gdbarch, - struct disassemble_info *di, - const gdb_byte *insn, int max_len, - CORE_ADDR addr) +gdb_buffered_insn_length_init_dis + (struct gdbarch *gdbarch, disassemble_info *di, const gdb_byte *insn, + int max_len, CORE_ADDR addr, + gdb::unique_xmalloc_ptr *disassembler_options_holder) { init_disassemble_info (di, NULL, gdb_buffered_insn_length_fprintf); @@ -889,7 +882,8 @@ gdb_buffered_insn_length_init_dis (struct gdbarch *gdbarch, di->endian = gdbarch_byte_order (gdbarch); di->endian_code = gdbarch_byte_order_for_code (gdbarch); - di->disassembler_options = get_all_disassembler_options (gdbarch); + *disassembler_options_holder = get_all_disassembler_options (gdbarch); + di->disassembler_options = disassembler_options_holder->get (); disassemble_init_for_target (di); } @@ -901,14 +895,12 @@ gdb_buffered_insn_length (struct gdbarch *gdbarch, const gdb_byte *insn, int max_len, CORE_ADDR addr) { struct disassemble_info di; - int len; - - gdb_buffered_insn_length_init_dis (gdbarch, &di, insn, max_len, addr); + gdb::unique_xmalloc_ptr disassembler_options_holder; - len = gdbarch_print_insn (gdbarch, addr, &di); + gdb_buffered_insn_length_init_dis (gdbarch, &di, insn, max_len, addr, + &disassembler_options_holder); - xfree (const_cast (di.disassembler_options)); - return len; + return gdbarch_print_insn (gdbarch, addr, &di); } char * diff --git a/gdb/disasm.h b/gdb/disasm.h index 712a216447fc..f70b006f0a79 100644 --- a/gdb/disasm.h +++ b/gdb/disasm.h @@ -47,8 +47,6 @@ public: : gdb_disassembler (gdbarch, file, dis_asm_read_memory) {} - ~gdb_disassembler (); - int print_insn (CORE_ADDR memaddr, int *branch_delay_insns = NULL); /* Return the gdbarch of gdb_disassembler. */ @@ -68,6 +66,11 @@ private: /* Stores data required for disassembling instructions in opcodes. */ struct disassemble_info m_di; + + /* If we own the string in m_di.disassembler_options, we do se + using this field. */ + gdb::unique_xmalloc_ptr m_disassembler_options_holder; + CORE_ADDR m_err_memaddr; static int dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index 803252201c37..b1d502f4827f 100644 --- a/gdb/i386-tdep.c +++ b/gdb/i386-tdep.c @@ -3966,20 +3966,12 @@ i386_sigtramp_p (struct frame_info *this_frame) static int i386_print_insn (bfd_vma pc, struct disassemble_info *info) { - int result; - gdb_assert (disassembly_flavor == att_flavor || disassembly_flavor == intel_flavor); - gdb_assert (info->disassembler_options == NULL); - info->disassembler_options = disassembly_flavor; - result = default_print_insn (pc, info); - - info->disassembler_options = NULL; - - return result; + return default_print_insn (pc, info); }