Re: [patch] Fix leak of bp_jit_event breakpoints

[Paul Pluzhnikov <ppluzhnikov at google dot com>]

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: gdb-jit-leak-20110127.txt
Description: Text document