[patch] Fix leak of bp_jit_event breakpoints

[patch] Fix leak of bp_jit_event breakpoints

[Paul Pluzhnikov]

Greetings,

On manual inspection of 'maint info break', I noticed that many instances
of __jit_debug_register_code breakpoint are leaked. For example:

gdb -q --args gdb.base/jit-main gdb.base/jit-solib.so 1
Reading symbols from /usr/local/google/jit-main...done.
(gdb) run
../../../src/gdb/testsuite/gdb.base/jit-main.c:131: libname = gdb.base/jit-solib.so, count = 1

Program exited normally.
(gdb) maint info b
Num     Type           Disp Enb Address            What
-1      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-2      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-4      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-5      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-6      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-7      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-14     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1
-15     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1
-16     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1
-17     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1
-18     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1
-19     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1
(gdb) run
../../../src/gdb/testsuite/gdb.base/jit-main.c:131: libname = gdb.base/jit-solib.so, count = 1

Program exited normally.
(gdb) maint info b
Num     Type           Disp Enb Address            What
-1      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-2      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-4      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-5      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-6      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-7      jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-20     jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-22     jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-23     jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-24     jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-25     jit events     keep y   0x00000000004009c4 <__jit_debug_register_code> inf 1
        breakpoint already hit 2 times
-32     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1
-33     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1
-34     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1
-35     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1
-36     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1
-37     longjmp master keep n   0x00007ffff780a8b0 <__libc_siglongjmp> inf 1


That leak is in fact also causing jit.exp test case to fail with gdbserver
(native), which passes after this patch is applied.

Here is a proposed cleanup/fix.


Thanks,

--
Paul Pluzhnikov

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

	* jit.c (registering_code): Delete.
	(clear_int): Delete.
	(jit_register_code): Adjust.
	(jit_breakpoint_re_set_internal): New function; move code here ...
	(jit_inferior_init): ... from here.
	(jit_breakpoint_re_set): Adjust.
	* breakpoint.c (breakpoint_re_set_one): Delete bp_jit_event
	breakpoint instead of leaking it.


Index: jit.c
===================================================================
RCS file: /cvs/src/src/gdb/jit.c,v
retrieving revision 1.10
diff -u -p -r1.10 jit.c
--- jit.c	9 Jan 2011 03:08:57 -0000	1.10
+++ jit.c	19 Jan 2011 19:36:04 -0000
@@ -41,15 +41,6 @@ static const char *const jit_descriptor_
 
 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;
-
 /* Non-zero if we want to see trace of jit level stuff.  */
 
 static int jit_debug = 0;
@@ -61,14 +52,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;
@@ -278,17 +261,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,43 +298,51 @@ 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 handler.
+   Return 0 on success.  */
 
-static void
-jit_inferior_init (struct gdbarch *gdbarch)
+static int
+jit_breakpoint_re_set_internal (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;
-  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;
 
   /* 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;
+    return 1;
   reg_addr = SYMBOL_VALUE_ADDRESS (reg_symbol);
   if (reg_addr == 0)
-    return;
+    return 2;
 
   if (jit_debug)
-    fprintf_unfiltered (gdb_stdlog, "jit_inferior_init, reg_addr = %s\n",
+    fprintf_unfiltered (gdb_stdlog,
+			"jit_breakpoint_re_set_internal, reg_addr = %s\n",
 			paddress (gdbarch, reg_addr));
 
+  /* Put a breakpoint in the registration symbol.  */
+  create_jit_event_breakpoint (gdbarch, reg_addr);
+
+  return 0;
+}
+
+/* Register any already created translations.  */
+
+static void
+jit_inferior_init (struct gdbarch *gdbarch)
+{
+  struct minimal_symbol *desc_symbol;
+  struct jit_descriptor descriptor;
+  struct jit_code_entry cur_entry;
+  CORE_ADDR cur_entry_addr;
+
+  if (jit_debug)
+    fprintf_unfiltered (gdb_stdlog, "jit_inferior_init\n");
+
+  if (jit_breakpoint_re_set_internal (gdbarch) != 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);
@@ -382,9 +365,6 @@ jit_inferior_init (struct gdbarch *gdbar
   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 +396,7 @@ jit_inferior_created_hook (void)
 void
 jit_breakpoint_re_set (void)
 {
-  jit_inferior_init (target_gdbarch);
+  jit_breakpoint_re_set_internal (target_gdbarch);
 }
 
 /* Wrapper to match the observer function pointer prototype.  */
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.529
diff -u -p -r1.529 breakpoint.c
--- breakpoint.c	11 Jan 2011 19:39:35 -0000	1.529
+++ breakpoint.c	19 Jan 2011 19:36:04 -0000
@@ -10588,11 +10588,12 @@ breakpoint_re_set_one (void *bint)
       printf_filtered (_("Deleting unknown breakpoint type %d\n"), b->type);
       /* fall through */
       /* Delete overlay event and longjmp master breakpoints; they will be
-	 reset later by breakpoint_re_set.  */
+	 reset later by breakpoint_re_set.  Likewise for jit_event.  */
     case bp_overlay_event:
     case bp_longjmp_master:
     case bp_std_terminate_master:
     case bp_exception_master:
+    case bp_jit_event:
       delete_breakpoint (b);
       break;
 
@@ -10619,7 +10620,6 @@ breakpoint_re_set_one (void *bint)
     case bp_longjmp_resume:
     case bp_exception:
     case bp_exception_resume:
-    case bp_jit_event:
       break;
     }
 

Re: [patch] Fix leak of bp_jit_event breakpoints

[Pedro Alves]

On Wednesday 19 January 2011 13:43:15, Paul Pluzhnikov wrote:
> @@ -10588,11 +10588,12 @@ breakpoint_re_set_one (void *bint)
>        printf_filtered (_("Deleting unknown breakpoint type %d\n"),
> b->type); /* fall through */
>        /* Delete overlay event and longjmp master breakpoints; they will be
> -        reset later by breakpoint_re_set.  */
> +        reset later by breakpoint_re_set.  Likewise for jit_event.  */
>      case bp_overlay_event:
>      case bp_longjmp_master:
>      case bp_std_terminate_master:
>      case bp_exception_master:
> +    case bp_jit_event:
>        delete_breakpoint (b);
>        break;
>  
> @@ -10619,7 +10620,6 @@ breakpoint_re_set_one (void *bint)
>      case bp_longjmp_resume:
>      case bp_exception:
>      case bp_exception_resume:
> -    case bp_jit_event:
>        break;
>      }

This part doesn't look quite right.  In non-stop (breakpoint always-inserted)
mode, this looks it will create a race window where you delete the jit event
breakpoint whenever a random thread loads/unloads a DSO, meaning
you could miss a jit registration done by some other thread still running.

-- 
Pedro Alves

Re: [patch] Fix leak of bp_jit_event breakpoints

[Paul Pluzhnikov]

On Wed, Jan 19, 2011 at 1:09 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Wednesday 19 January 2011 13:43:15, Paul Pluzhnikov wrote:
>> @@ -10588,11 +10588,12 @@ breakpoint_re_set_one (void *bint)
>>        printf_filtered (_("Deleting unknown breakpoint type %d\n"),
>> b->type); /* fall through */
>>        /* Delete overlay event and longjmp master breakpoints; they will be
>> -        reset later by breakpoint_re_set.  */
>> +        reset later by breakpoint_re_set.  Likewise for jit_event.  */
>>      case bp_overlay_event:
>>      case bp_longjmp_master:
>>      case bp_std_terminate_master:
>>      case bp_exception_master:
>> +    case bp_jit_event:
>>        delete_breakpoint (b);
>>        break;
>>
>> @@ -10619,7 +10620,6 @@ breakpoint_re_set_one (void *bint)
>>      case bp_longjmp_resume:
>>      case bp_exception:
>>      case bp_exception_resume:
>> -    case bp_jit_event:
>>        break;
>>      }
>
> This part doesn't look quite right.  In non-stop (breakpoint always-inserted)
> mode, this looks it will create a race window where you delete the jit event
> breakpoint whenever a random thread loads/unloads a DSO, meaning
> you could miss a jit registration done by some other thread still running.

Good point.

We could go with Yao's solution of searching for a matching existing
bp_jit_event at the given address; but I don't like it for two
reasons:
- yet another linear loop
- if a binary is reloaded, and &__jit_debug_register_code changes,
we'll still leak a breakpoint on rerun

Would recording the current jit_event breakpoint in inferior (via
register_inferior_data_with_cleanup ()) be a good solution?

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Fix leak of bp_jit_event breakpoints

[Paul Pluzhnikov]

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

On Wed, Jan 19, 2011 at 9:57 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> Would recording the current jit_event breakpoint in inferior (via
> register_inferior_data_with_cleanup ()) be a good solution?

That appears to work reasonably well.

It fixes both the gdbserver failure in jit.exp, and also as yet undiscovered
problem: the old static/global jit_descriptor_addr couldn't possibly work
for more than one inferior.

Thanks,
-- 
Paul Pluzhnikov


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

	* 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_exit_hook): Adjust.
	(jit_event_handler): Adjust.
	(_initialize_jit): Adjust.

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

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	20 Jan 2011 23:03:20 -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,42 @@ bfd_open_from_target_memory (CORE_ADDR a
                           mem_bfd_iovec_stat);
 }
 
+struct jit_inferior_data {
+  struct breakpoint *breakpoint;
+  CORE_ADDR breakpoint_addr;
+  CORE_ADDR descriptor_addr;
+};
+
+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 +179,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 +290,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 +327,99 @@ 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 && inf_data->breakpoint != NULL)
+    /* Breakpoint already set.  */
+    return 0;
+
+  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;
+    }
+
+  if (jit_debug)
+    fprintf_unfiltered (gdb_stdlog,
+			"jit_breakpoint_re_set_internal, "
+			"breakpoint_addr = %s\n",
+			paddress (gdbarch, inf_data->breakpoint_addr));
+
+  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;
+
+      /* Location has changed since last run.  */
+      delete_breakpoint (inf_data->breakpoint);
+    }
+
+  /* Put a breakpoint in the registration symbol.  */
+  inf_data->breakpoint =
+    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 +451,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.  */
@@ -436,14 +472,16 @@ 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;
+  struct jit_inferior_data *inf_data;
 
   ALL_OBJFILES_SAFE (objf, temp)
     if (objfile_data (objf, jit_objfile_data) != NULL)
       jit_unregister_code (objf);
+
+  /* Force lookup of jit symbol addresses on next run.  */
+  inf_data = inferior_data (inf, jit_inferior_data);
+  inf_data->breakpoint_addr = 0;
+  inf_data->descriptor_addr = 0;
 }
 
 void
@@ -455,7 +493,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 +540,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);
 }

Re: [patch] Fix leak of bp_jit_event breakpoints

[Paul Pluzhnikov]

On Thu, Jan 20, 2011 at 3:27 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> That appears to work reasonably well.
>
> It fixes both the gdbserver failure in jit.exp, and also as yet undiscovered
> problem: the old static/global jit_descriptor_addr couldn't possibly work
> for more than one inferior.

Ping?

--
Paul Pluzhnikov

Re: [patch] Fix leak of bp_jit_event breakpoints

[Pedro Alves]

Hi Paul, sorry for the delay in getting back to this.

On Thursday 27 January 2011 23:27:39, Paul Pluzhnikov wrote:
> 
> +struct jit_inferior_data {

Put the { on it's own line, please.

> +  struct breakpoint *breakpoint;
> +  CORE_ADDR breakpoint_addr;
> +  CORE_ADDR descriptor_addr;
> +};

> +static struct jit_inferior_data *
> +get_jit_inferior_data (void)

Please add a small describing blurb over functions.


> +  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?)

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.  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.  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.

> +
> +      /* Location has changed since last run.  */
> +      delete_breakpoint (inf_data->breakpoint);
> +    }

-- 
Pedro Alves

Re: [patch] Fix leak of bp_jit_event breakpoints

[Paul Pluzhnikov]

[-- 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);
 }

Re: [patch] Fix leak of bp_jit_event breakpoints

[Pedro Alves]

On Thursday 27 January 2011 21:50:35, Paul Pluzhnikov wrote:
> -/* This is the address of the JIT descriptor in the inferior.  */
> -
> -static CORE_ADDR jit_descriptor_addr = 0;

...

> +struct jit_inferior_data
> +{
> +  CORE_ADDR breakpoint_addr;
> +  CORE_ADDR descriptor_addr;
> +};

You've lost the comment above.  It'd be nice to
have this structure and its fields documented..

> > 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.

The other way around isn't.  If e.g., you attach to a process,
and notice that the exec is wrongly set --- and then use "file"
command to fix it.  We'd set a new jit breakpoint before, but
we don't now --- I think we should reset the jit state in that
case, at least like jit_inferior_created_observer is
doing, and that's what I was thinking could be done
from the executable_changed observer.

Other than that, the patch looks okay.

-- 
Pedro Alves

Re: [patch] Fix leak of bp_jit_event breakpoints

[Paul Pluzhnikov]

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

On Thu, Jan 27, 2011 at 2:59 PM, Pedro Alves <pedro@codesourcery.com> wrote:

> On Thursday 27 January 2011 21:50:35, Paul Pluzhnikov wrote:
>> -/* This is the address of the JIT descriptor in the inferior.  */
>> -
>> -static CORE_ADDR jit_descriptor_addr = 0;
>
> ...
>
>> +struct jit_inferior_data
>> +{
>> +  CORE_ADDR breakpoint_addr;
>> +  CORE_ADDR descriptor_addr;
>> +};
>
> You've lost the comment above.  It'd be nice to
> have this structure and its fields documented..

Done.

>> I think this is covered now -- after "file", if we attach or run,
>> inferior_create_observer will delete the old breakpoint.
>
> The other way around isn't.  If e.g., you attach to a process,
> and notice that the exec is wrongly set --- and then use "file"
> command to fix it.  We'd set a new jit breakpoint before, but
> we don't now --- I think we should reset the jit state in that
> case, at least like jit_inferior_created_observer is
> doing, and that's what I was thinking could be done
> from the executable_changed observer.

Done.

gdb.base/jit.exp passes with both gdb and native gdbserver.

I'll wait a couple more days, then submit attached patch if there are no
further comments.

Thanks,
-- 
Paul Pluzhnikov

2011-01-28  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_reset_inferior_data_and_breakpoints): New function.
	(jit_inferior_created_observer): Adjust.
	(jit_inferior_exit_hook): Adjust.
	(jit_executable_changed_observer): New function.
	(jit_event_handler): Adjust.
	(_initialize_jit): Adjust.

[-- Attachment #2: gdb-jit-leak-20110128.txt --]
[-- Type: text/plain, Size: 11795 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	28 Jan 2011 17:36:10 -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	28 Jan 2011 17:36:10 -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	28 Jan 2011 17:36:10 -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,47 @@ bfd_open_from_target_memory (CORE_ADDR a
                           mem_bfd_iovec_stat);
 }
 
+/* Per-inferior structure recording the addresses in the inferior.  */
+
+struct jit_inferior_data
+{
+  CORE_ADDR breakpoint_addr;  /* &__jit_debug_register_code()  */
+  CORE_ADDR descriptor_addr;  /* &__jit_debug_descriptor  */
+};
+
+/* 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 +184,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 +295,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 +332,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);
+    fprintf_unfiltered (gdb_stdlog, "jit_inferior_init\n");
 
-  /* 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)
+  inf_data = get_jit_inferior_data ();
+  if (jit_breakpoint_re_set_internal (gdbarch, inf_data) != 0)
     return;
 
-  /* 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)
-    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,6 +443,26 @@ jit_inferior_created_hook (void)
 void
 jit_breakpoint_re_set (void)
 {
+  jit_breakpoint_re_set_internal (target_gdbarch,
+				  get_jit_inferior_data ());
+}
+
+/* Reset inferior_data, so sybols will be looked up again, and jit_breakpoint
+   will be reset.  */
+
+static void
+jit_reset_inferior_data_and_breakpoints (void)
+{
+  struct jit_inferior_data *inf_data;
+
+  /* Force jit_inferior_init to re-lookup of jit symbol addresses.  */
+  inf_data = get_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);
 }
 
@@ -424,7 +471,7 @@ jit_breakpoint_re_set (void)
 static void
 jit_inferior_created_observer (struct target_ops *objfile, int from_tty)
 {
-  jit_inferior_init (target_gdbarch);
+  jit_reset_inferior_data_and_breakpoints ();
 }
 
 /* This function cleans up any code entries left over when the
@@ -437,15 +484,17 @@ 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);
 }
 
+static void
+jit_executable_changed_observer (void)
+{
+  jit_reset_inferior_data_and_breakpoints ();
+}
+
 void
 jit_event_handler (struct gdbarch *gdbarch)
 {
@@ -455,7 +504,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.  */
@@ -500,5 +550,8 @@ _initialize_jit (void)
 
   observer_attach_inferior_created (jit_inferior_created_observer);
   observer_attach_inferior_exit (jit_inferior_exit_hook);
+  observer_attach_executable_changed (jit_executable_changed_observer);
   jit_objfile_data = register_objfile_data ();
+  jit_inferior_data =
+    register_inferior_data_with_cleanup (jit_inferior_data_cleanup);
 }

Re: [patch] Fix leak of bp_jit_event breakpoints

[Pedro Alves]

On Friday 28 January 2011 17:47:40, Paul Pluzhnikov wrote:
> gdb.base/jit.exp passes with both gdb and native gdbserver.
> 
> I'll wait a couple more days, then submit attached patch if there are no
> further comments.
> 

Thanks for the patience.  I have no further comments.

-- 
Pedro Alves

Re: [patch] Fix leak of bp_jit_event breakpoints

[Paul Pluzhnikov]

On Fri, Jan 28, 2011 at 10:02 AM, Pedro Alves <pedro@codesourcery.com> wrote:

>> I'll wait a couple more days, then submit attached patch if there are no
>> further comments.
>>
>
> Thanks for the patience.  I have no further comments.

So committed.

Thanks,
-- 
Paul Pluzhnikov