Re: [patch] Fix leak of bp_jit_event breakpoints

[Pedro Alves <pedro@codesourcery.com>]

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