Re: [PATCH] gdb: get jiter objfile from a bound minsym

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Mihails Strasuns via Gdb-patches <gdb-patches@sourceware.org> [2020-10-14 11:00:03 +0200]:

> This fixes a regression introduced by the following commit:
> 
> fe053b9e853 gdb/jit: pass the jiter objfile as an argument to jit_event_handler
> 
> In the refactoring `handle_jit_event` function was changed to pass a matching
> objfile pointer to the `jit_event_handler` explicitly, rather using internal
> storage:
> 
> ```
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5448,8 +5448,9 @@ handle_jit_event (void)
> 
>    frame = get_current_frame ();
>    gdbarch = get_frame_arch (frame);
> +  objfile *jiter = symbol_objfile (get_frame_function (frame));
> 
> -  jit_event_handler (gdbarch);
> +  jit_event_handler (gdbarch, jiter);
> ```
> 
> This was needed to add support for a multiple jiters in a following commits.
> However it has also introduced a regression, because `get_frame_function
> (frame)` here may return `nullptr`, resulting in a crash.
> 
> A more resilient way would be to use an approach mirroring
> `jit_breakpoint_re_set` - to find a minimal symbol matching the breakpoint
> location and use its object file. We know that this breakpoint events comes
> from a breakpoint set by `jit_breakpoint_re_set`, thus using the reverse
> approach should be reliable enough.
> 
> gdb/Changelog:
> 2020-10-14  Mihails Strasuns  <mihails.strasuns@intel.com>
> 
> 	* breakpoint.c (handle_jit_event): and an argument, change how
> 	`jit_event_handler` is called.
> ---
>  gdb/breakpoint.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 414208469f9..878d48c5984 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5434,9 +5434,8 @@ bpstat_stop_status (const address_space *aspace,
>  }
>  
>  static void
> -handle_jit_event (void)
> +handle_jit_event (CORE_ADDR address)
>  {
> -  struct frame_info *frame;
>    struct gdbarch *gdbarch;
>  
>    if (debug_infrun)
> @@ -5446,11 +5445,9 @@ handle_jit_event (void)
>       breakpoint_re_set.  */
>    target_terminal::ours_for_output ();
>  
> -  frame = get_current_frame ();
> -  gdbarch = get_frame_arch (frame);
> -  objfile *jiter = symbol_objfile (get_frame_function (frame));
> -
> -  jit_event_handler (gdbarch, jiter);
> +  gdbarch = get_frame_arch (get_current_frame ());
> +  bound_minimal_symbol jit_bp_sym = lookup_minimal_symbol_by_pc (address);
> +  jit_event_handler (gdbarch, jit_bp_sym.objfile);

It might be nice to add:

  gdb_assert (jit_bp_sym.objfile != nullptr);

before the call to jit_event_handler.  Even better (IMO) would be to
add a short comment explaining why it is that the assertion should be
true, though this isn't essential as the assertion can easily be
tracked back to this commit which includes an explanation.

I'm not very (or at all) familiar with this area of GDB, but the
change seems reasonable so I'd be happy to see this merged with the
assertion added.

Thanks,
Andrew

>  
>    target_terminal::inferior ();
>  }
> @@ -5651,7 +5648,7 @@ bpstat_run_callbacks (bpstat bs_head)
>        switch (b->type)
>  	{
>  	case bp_jit_event:
> -	  handle_jit_event ();
> +	  handle_jit_event (bs->bp_location_at->address);
>  	  break;
>  	case bp_gnu_ifunc_resolver:
>  	  gnu_ifunc_resolver_stop (b);
> -- 
> 2.17.1
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>