Re: [patch] Fix leak of bp_jit_event breakpoints

[Paul Pluzhnikov <ppluzhnikov@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3124 bytes --]

On Thu, Jan 27, 2011 at 4:44 AM, Pedro Alves <pedro@codesourcery.com> wrote:

Thanks for your comments.

>> +struct jit_inferior_data {
>
> Put the { on it's own line, please.

Done.

>> +static struct jit_inferior_data *
>> +get_jit_inferior_data (void)
>
> Please add a small describing blurb over functions.

Done.

>> +  if (inf_data->breakpoint != NULL)
>> +    {
>> +      if (inf_data->breakpoint_addr == inf_data->breakpoint->loc->address)
>> +       /* Same location as on last run.  Existing breakpoint is good.  */
>> +       return 0;
>
> I'm a little warry about this optimization.  For example,
> we should probably also compare other things, like
> gdbarch and location's pspace|aspace.  Is it
> a significant difference if we always delete the breakpoint
> (here or perhaps on inferior exit?)

I have not measured this, and optimization pass will be needed later anyway
for the quadratic slowdown (reported by Vyacheslav earlier).  Let's worry
about correctness first.

> There's at least one problem to solve: on "exec",
> update_breakpoints_after_exec deletes bp_jit_event
> breakpoints, effectively making it so that your
> inf_data->breakpoint pointer becomes stale.

I am in a twisty maze of passages, all alike ;-(

> There may
> be other paths that delete the breakpoint behind jit.c's
> back.  One solution would be to get rid of the breakpoint
> pointer in jit.c, and add a remove_jit_event_breakpoints
> function, modelled on remove_solib_event_breakpoints.

Done.

I am calling remove_jit_event_breakpoints from inferior_create_observer
(removing breakpoints doesn't work from inferior_exit_hook :-(

> But
> if you want to come up with other solutions, I'd be happy
> to consider them.  I'm thinking that we should delete the
> jit breakpoint (and perhaps more) whenever the executable
> changes (say, the "file" command), which is kind of
> a similar case of an "exec", so maybe we should install
> an executable_changed observer as well.  Not sure that
> covers all we need.

I think this is covered now -- after "file", if we attach or run,
inferior_create_observer will delete the old breakpoint.

Thanks,
-- 
Paul Pluzhnikov

2011-01-27  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* breakpoint.h (remove_jit_event_breakpoints): New prototype.
	* breakpoint.c (remove_jit_event_breakpoints): New function.
       	* jit.c (jit_descriptor_addr): Delete.
       	(registering_code): Delete.
       	(clear_int): Delete.
       	(jit_inferior_data): New variable.
       	(struct jit_inferior_data): New type.
       	(get_jit_inferior_data): New function.
       	(jit_inferior_data_cleanup): New function.
       	(jit_read_descriptor): Adjust.
       	(jit_register_code): Adjust.
       	(jit_breakpoint_re_set_internal): New function; move code here ...
       	(jit_inferior_init): ... from here.
       	(jit_breakpoint_re_set): Adjust.
       	(jit_inferior_created_observer): Adjust.
       	(jit_inferior_exit_hook): Adjust.
       	(jit_event_handler): Adjust.
       	(_initialize_jit): Adjust.

[-- Attachment #2: gdb-jit-leak-20110127.txt --]
[-- Type: text/plain, Size: 11221 bytes --]

Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.132
diff -u -p -p -u -r1.132 breakpoint.h
--- breakpoint.h	11 Jan 2011 19:23:02 -0000	1.132
+++ breakpoint.h	27 Jan 2011 21:48:13 -0000
@@ -1077,6 +1077,8 @@ extern struct breakpoint *create_solib_e
 extern struct breakpoint *create_thread_event_breakpoint (struct gdbarch *,
 							  CORE_ADDR);
 
+extern void remove_jit_event_breakpoints (void);
+
 extern void remove_solib_event_breakpoints (void);
 
 extern void remove_thread_event_breakpoints (void);
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.529
diff -u -p -p -u -r1.529 breakpoint.c
--- breakpoint.c	11 Jan 2011 19:39:35 -0000	1.529
+++ breakpoint.c	27 Jan 2011 21:48:13 -0000
@@ -5911,6 +5911,19 @@ create_jit_event_breakpoint (struct gdba
   return b;
 }
 
+/* Remove JIT code registration and unregistration breakpoint(s).  */
+
+void
+remove_jit_event_breakpoints (void)
+{
+  struct breakpoint *b, *b_tmp;
+
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
+    if (b->type == bp_jit_event
+	&& b->loc->pspace == current_program_space)
+      delete_breakpoint (b);
+}
+
 void
 remove_solib_event_breakpoints (void)
 {
Index: jit.c
===================================================================
RCS file: /cvs/src/src/gdb/jit.c,v
retrieving revision 1.10
diff -u -p -p -u -r1.10 jit.c
--- jit.c	9 Jan 2011 03:08:57 -0000	1.10
+++ jit.c	27 Jan 2011 21:48:13 -0000
@@ -24,6 +24,7 @@
 #include "command.h"
 #include "gdbcmd.h"
 #include "gdbcore.h"
+#include "inferior.h"
 #include "observer.h"
 #include "objfiles.h"
 #include "symfile.h"
@@ -37,18 +38,7 @@ static const char *const jit_break_name 
 
 static const char *const jit_descriptor_name = "__jit_debug_descriptor";
 
-/* This is the address of the JIT descriptor in the inferior.  */
-
-static CORE_ADDR jit_descriptor_addr = 0;
-
-/* This is a boolean indicating whether we're currently registering code.  This
-   is used to avoid re-entering the registration code.  We want to check for
-   new JITed every time a new object file is loaded, but we want to avoid
-   checking for new code while we're registering object files for JITed code.
-   Therefore, we flip this variable to 1 before registering new object files,
-   and set it to 0 before returning.  */
-
-static int registering_code = 0;
+static const struct inferior_data *jit_inferior_data = NULL;
 
 /* Non-zero if we want to see trace of jit level stuff.  */
 
@@ -61,14 +51,6 @@ show_jit_debug (struct ui_file *file, in
   fprintf_filtered (file, _("JIT debugging is %s.\n"), value);
 }
 
-/* Helper cleanup function to clear an integer flag like the one above.  */
-
-static void
-clear_int (void *int_addr)
-{
-  *((int *) int_addr) = 0;
-}
-
 struct target_buffer
 {
   CORE_ADDR base;
@@ -146,12 +128,45 @@ bfd_open_from_target_memory (CORE_ADDR a
                           mem_bfd_iovec_stat);
 }
 
+struct jit_inferior_data
+{
+  CORE_ADDR breakpoint_addr;
+  CORE_ADDR descriptor_addr;
+};
+
+/* Return jit_inferior_data for current inferior.  Allocate if not already
+   present.  */
+
+static struct jit_inferior_data *
+get_jit_inferior_data (void)
+{
+  struct inferior *inf;
+  struct jit_inferior_data *inf_data;
+
+  inf = current_inferior ();
+  inf_data = inferior_data (inf, jit_inferior_data);
+  if (inf_data == NULL)
+    {
+      inf_data = XZALLOC (struct jit_inferior_data);
+      set_inferior_data (inf, jit_inferior_data, inf_data);
+    }
+
+  return inf_data;
+}
+
+static void
+jit_inferior_data_cleanup (struct inferior *inf, void *arg)
+{
+  xfree (arg);
+}
+
 /* Helper function for reading the global JIT descriptor from remote
    memory.  */
 
 static void
 jit_read_descriptor (struct gdbarch *gdbarch,
-		     struct jit_descriptor *descriptor)
+		     struct jit_descriptor *descriptor,
+		     CORE_ADDR descriptor_addr)
 {
   int err;
   struct type *ptr_type;
@@ -167,7 +182,7 @@ jit_read_descriptor (struct gdbarch *gdb
   desc_buf = alloca (desc_size);
 
   /* Read the descriptor.  */
-  err = target_read_memory (jit_descriptor_addr, desc_buf, desc_size);
+  err = target_read_memory (descriptor_addr, desc_buf, desc_size);
   if (err)
     error (_("Unable to read JIT descriptor from remote memory!"));
 
@@ -278,17 +293,9 @@ JITed symbol file is not an object file,
         ++i;
       }
 
-  /* Raise this flag while we register code so we won't trigger any
-     re-registration.  */
-  registering_code = 1;
-  my_cleanups = make_cleanup (clear_int, &registering_code);
-
   /* This call takes ownership of sai.  */
   objfile = symbol_file_add_from_bfd (nbfd, 0, sai, OBJF_SHARED);
 
-  /* Clear the registering_code flag.  */
-  do_cleanups (my_cleanups);
-
   /* Remember a mapping from entry_addr to objfile.  */
   entry_addr_ptr = xmalloc (sizeof (CORE_ADDR));
   *entry_addr_ptr = entry_addr;
@@ -323,68 +330,86 @@ jit_find_objf_with_entry_addr (CORE_ADDR
   return NULL;
 }
 
-/* (Re-)Initialize the jit breakpoint handler, and register any already
-   created translations.  */
+/* (Re-)Initialize the jit breakpoint if necessary.
+   Return 0 on success.  */
+
+static int
+jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
+				struct jit_inferior_data *inf_data)
+{
+  if (inf_data->breakpoint_addr == 0)
+    {
+      struct minimal_symbol *reg_symbol;
+
+      /* Lookup the registration symbol.  If it is missing, then we assume
+	 we are not attached to a JIT.  */
+      reg_symbol = lookup_minimal_symbol (jit_break_name, NULL, NULL);
+      if (reg_symbol == NULL)
+	return 1;
+      inf_data->breakpoint_addr = SYMBOL_VALUE_ADDRESS (reg_symbol);
+      if (inf_data->breakpoint_addr == 0)
+	return 2;
+    }
+  else
+    return 0;
+
+  if (jit_debug)
+    fprintf_unfiltered (gdb_stdlog,
+			"jit_breakpoint_re_set_internal, "
+			"breakpoint_addr = %s\n",
+			paddress (gdbarch, inf_data->breakpoint_addr));
+
+  /* Put a breakpoint in the registration symbol.  */
+  create_jit_event_breakpoint (gdbarch, inf_data->breakpoint_addr);
+
+  return 0;
+}
+
+/* Register any already created translations.  */
 
 static void
 jit_inferior_init (struct gdbarch *gdbarch)
 {
-  struct minimal_symbol *reg_symbol;
-  struct minimal_symbol *desc_symbol;
-  CORE_ADDR reg_addr;
   struct jit_descriptor descriptor;
   struct jit_code_entry cur_entry;
+  struct jit_inferior_data *inf_data;
   CORE_ADDR cur_entry_addr;
 
   if (jit_debug)
-    fprintf_unfiltered (gdb_stdlog,
-			"jit_inferior_init, registering_code = %d\n",
-			registering_code);
-
-  /* When we register code, GDB resets its breakpoints in case symbols have
-     changed.  That in turn calls this handler, which makes us look for new
-     code again.  To avoid being re-entered, we check this flag.  */
-  if (registering_code)
-    return;
+    fprintf_unfiltered (gdb_stdlog, "jit_inferior_init\n");
 
-  /* Lookup the registration symbol.  If it is missing, then we assume we are
-     not attached to a JIT.  */
-  reg_symbol = lookup_minimal_symbol (jit_break_name, NULL, NULL);
-  if (reg_symbol == NULL)
-    return;
-  reg_addr = SYMBOL_VALUE_ADDRESS (reg_symbol);
-  if (reg_addr == 0)
+  inf_data = get_jit_inferior_data ();
+  if (jit_breakpoint_re_set_internal (gdbarch, inf_data) != 0)
     return;
 
-  if (jit_debug)
-    fprintf_unfiltered (gdb_stdlog, "jit_inferior_init, reg_addr = %s\n",
-			paddress (gdbarch, reg_addr));
+  if (inf_data->descriptor_addr == 0)
+    {
+      struct minimal_symbol *desc_symbol;
 
-  /* Lookup the descriptor symbol and cache the addr.  If it is missing, we
-     assume we are not attached to a JIT and return early.  */
-  desc_symbol = lookup_minimal_symbol (jit_descriptor_name, NULL, NULL);
-  if (desc_symbol == NULL)
-    return;
-  jit_descriptor_addr = SYMBOL_VALUE_ADDRESS (desc_symbol);
-  if (jit_descriptor_addr == 0)
-    return;
+      /* Lookup the descriptor symbol and cache the addr.  If it is
+	 missing, we assume we are not attached to a JIT and return early.  */
+      desc_symbol = lookup_minimal_symbol (jit_descriptor_name, NULL, NULL);
+      if (desc_symbol == NULL)
+	return;
+
+      inf_data->descriptor_addr = SYMBOL_VALUE_ADDRESS (desc_symbol);
+      if (inf_data->descriptor_addr == 0)
+	return;
+    }
 
   if (jit_debug)
     fprintf_unfiltered (gdb_stdlog,
-			"jit_inferior_init, jit_descriptor_addr = %s\n",
-			paddress (gdbarch, jit_descriptor_addr));
+			"jit_inferior_init, descriptor_addr = %s\n",
+			paddress (gdbarch, inf_data->descriptor_addr));
 
   /* Read the descriptor so we can check the version number and load
      any already JITed functions.  */
-  jit_read_descriptor (gdbarch, &descriptor);
+  jit_read_descriptor (gdbarch, &descriptor, inf_data->descriptor_addr);
 
   /* Check that the version number agrees with that we support.  */
   if (descriptor.version != 1)
     error (_("Unsupported JIT protocol version in descriptor!"));
 
-  /* Put a breakpoint in the registration symbol.  */
-  create_jit_event_breakpoint (gdbarch, reg_addr);
-
   /* If we've attached to a running program, we need to check the descriptor
      to register any functions that were already generated.  */
   for (cur_entry_addr = descriptor.first_entry;
@@ -416,7 +441,8 @@ jit_inferior_created_hook (void)
 void
 jit_breakpoint_re_set (void)
 {
-  jit_inferior_init (target_gdbarch);
+  jit_breakpoint_re_set_internal (target_gdbarch,
+				  get_jit_inferior_data ());
 }
 
 /* Wrapper to match the observer function pointer prototype.  */
@@ -424,6 +450,16 @@ jit_breakpoint_re_set (void)
 static void
 jit_inferior_created_observer (struct target_ops *objfile, int from_tty)
 {
+  struct jit_inferior_data *inf_data;
+
+  /* Force jit_inferior_init to re-lookup of jit symbol addresses.  */
+  inf_data = inferior_data (current_inferior (), jit_inferior_data);
+  inf_data->breakpoint_addr = 0;
+  inf_data->descriptor_addr = 0;
+
+  /* Remove any existing JIT breakpoint(s).  */
+  remove_jit_event_breakpoints ();
+
   jit_inferior_init (target_gdbarch);
 }
 
@@ -437,10 +473,6 @@ jit_inferior_exit_hook (struct inferior 
   struct objfile *objf;
   struct objfile *temp;
 
-  /* We need to reset the descriptor addr so that next time we load up the
-     inferior we look for it again.  */
-  jit_descriptor_addr = 0;
-
   ALL_OBJFILES_SAFE (objf, temp)
     if (objfile_data (objf, jit_objfile_data) != NULL)
       jit_unregister_code (objf);
@@ -455,7 +487,8 @@ jit_event_handler (struct gdbarch *gdbar
   struct objfile *objf;
 
   /* Read the descriptor from remote memory.  */
-  jit_read_descriptor (gdbarch, &descriptor);
+  jit_read_descriptor (gdbarch, &descriptor,
+		       get_jit_inferior_data ()->descriptor_addr);
   entry_addr = descriptor.relevant_entry;
 
   /* Do the corresponding action.  */
@@ -501,4 +534,6 @@ _initialize_jit (void)
   observer_attach_inferior_created (jit_inferior_created_observer);
   observer_attach_inferior_exit (jit_inferior_exit_hook);
   jit_objfile_data = register_objfile_data ();
+  jit_inferior_data =
+    register_inferior_data_with_cleanup (jit_inferior_data_cleanup);
 }