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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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(-)

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<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 : "");
-  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<char> 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<char *> (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<char> *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<char> 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<char *> (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<char> 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);
 }
 

-- 
2.17.1