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

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

[Mihails Strasuns]

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

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

[Strasuns, Mihails]

Note: this should target GDB 10 branch as it fixes a regression from GDB 9.

> -----Original Message-----
> From: Mihails Strasuns <mihails.strasuns@intel.com>
> Sent: Wednesday, October 14, 2020 11:00 AM
> To: gdb-patches@sourceware.org
> Cc: Strasuns, Mihails <mihails.strasuns@intel.com>
> Subject: [PATCH] gdb: get jiter objfile from a bound minsym
> 
> This fixes a regression introduced by the following commit:
>  ...
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

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

[Andrew Burgess]

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

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

[Aktemur, Tankut Baris]

Hi Mihails,

A few nits below.

On Monday, October 19, 2020 11:50 AM, Andrew Burgess wrote:
> * 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.

"a multiple jiters" -> "multiple jiters"
"in a following commits" -> "in a subsequent commit."? Or maybe just end the
sentence after "jiters".

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

Dot-space-space.
"events" -> "event"

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

"and" -> "Add"

Thanks for the fix.  It makes sense to me.

-Baris


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

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

[Strasuns, Mihails]

Pushed to both gdb-10-branch and master after addressing comments from Baris and Andrew.

> -----Original Message-----
> From: Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com>
> Sent: Monday, October 19, 2020 12:40 PM
> To: Strasuns, Mihails <mihails.strasuns@intel.com>
> Cc: gdb-patches@sourceware.org; Andrew Burgess
> <andrew.burgess@embecosm.com>
> Subject: RE: [PATCH] gdb: get jiter objfile from a bound minsym
> 
> Hi Mihails,
> 
> A few nits below.
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