[2/3] gdb/jit: enable tracking multiple jitter objfiles

Message ID 886ee7b960b7da9ee20ae3c4b889d2b9c3246b33.1590397723.git.tankut.baris.aktemur@intel.com
State New
Headers show
Series
  • Handling multiple JITers
Related show

Commit Message

Keep track of multiple JITer objfiles and their JIT breakpoints.

This patch is viewed better with 'git diff -w'.

Regression-tested on X86_64 Linux.

gdb/ChangeLog:
2020-05-25  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* jit.c (jit_breakpoint_re_set_internal): Iterate over all objfiles
	in the program space to look for JIT symbols.
	(jit_inferior_init): Iterate over all objfiles with JIT breakpoints.
	(jit_event_handler): Ditto.

gdb/testsuite/ChangeLog:
2020-05-25  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* gdb.base/jit-reader-simple.exp: Add a scenario for a binary that
	loads two JITers.
---
 gdb/jit.c                                    | 171 +++++++++----------
 gdb/testsuite/gdb.base/jit-reader-simple.exp |  43 ++++-
 2 files changed, 125 insertions(+), 89 deletions(-)

-- 
2.17.1

Comments

Simon Marchi June 14, 2020, 5:09 p.m. | #1
On 2020-05-25 5:38 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> diff --git a/gdb/jit.c b/gdb/jit.c

> index fdb1248ed5b..c689ac3f392 100644

> --- a/gdb/jit.c

> +++ b/gdb/jit.c

> @@ -966,58 +966,53 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,


The comment above jit_breakpoint_re_set_internal says:

  Return 0 if the jit breakpoint has been successfully initialized.

Can you adjust it to the new reality?  Something like "Return 0 if one
or more JIT breakpoints were successfully initialized".

I think it would also be good (as a separate cleanup) to make this function
return a bool, returning true when one or more JIT breakpoints were inserted.

> @@ -1357,38 +1352,38 @@ jit_event_handler (struct gdbarch *gdbarch)

>  

>    jit_program_space_data *ps_data = get_jit_program_space_data ();

>  

> -  /* Fetch the saved objfile.  */

> -  objfile *objf = nullptr;

> -  if (!ps_data->objfile_and_bps.empty ())

> -    objf = (ps_data->objfile_and_bps.begin ())->first;

> -

> -  /* Read the descriptor from remote memory.  */

> -  if (!jit_read_descriptor (gdbarch, &descriptor, objf))

> -    return;

> -  entry_addr = descriptor.relevant_entry;

> -

> -  /* Do the corresponding action.  */

> -  switch (descriptor.action_flag)

> +  for (auto &objf_and_bp : ps_data->objfile_and_bps)

>      {

> -    case JIT_NOACTION:

> -      break;

> -    case JIT_REGISTER:

> -      jit_read_code_entry (gdbarch, entry_addr, &code_entry);

> -      jit_register_code (gdbarch, entry_addr, &code_entry);

> -      break;

> -    case JIT_UNREGISTER:

> -      objf = jit_find_objf_with_entry_addr (entry_addr);

> -      if (objf == NULL)

> -	printf_unfiltered (_("Unable to find JITed code "

> -			     "entry at address: %s\n"),

> -			   paddress (gdbarch, entry_addr));

> -      else

> -	objf->unlink ();

> +      objfile *objf = objf_and_bp.first;

>  

> -      break;

> -    default:

> -      error (_("Unknown action_flag value in JIT descriptor!"));

> -      break;

> +      /* Read the descriptor from remote memory.  */

> +      if (!jit_read_descriptor (gdbarch, &descriptor, objf))

> +	continue;

> +      entry_addr = descriptor.relevant_entry;

> +

> +      /* Do the corresponding action.  */

> +      switch (descriptor.action_flag)

> +	{

> +	case JIT_NOACTION:

> +	  break;

> +	case JIT_REGISTER:

> +	  jit_read_code_entry (gdbarch, entry_addr, &code_entry);

> +	  jit_register_code (gdbarch, entry_addr, &code_entry);

> +	  break;

> +	case JIT_UNREGISTER:

> +	  objf = jit_find_objf_with_entry_addr (entry_addr);

> +	  if (objf == NULL)

> +	    printf_unfiltered (_("Unable to find JITed code "

> +				 "entry at address: %s\n"),

> +			       paddress (gdbarch, entry_addr));

> +	  else

> +	    objf->unlink ();

> +

> +	  break;

> +	default:

> +	  error (_("Unknown action_flag value in JIT descriptor!"));

> +	  break;

> +	}


I don't think that iterating on all JIT-providing objfiles is the correct thing
to do here.  After all, we have hit the __jit_debug_register_code breakpoint of
exactly one of them, so we should only process the jit descriptor of that one.
At the same moment, the other JIT-providing objfiles could have prepared anything
in their descriptor (or there could be junk data), but they have not asked us
to process it.

I haven't tried, but I bet that you could build a test to trigger a bug here:

1. Have two jit-providing objfiles register one jit objfile each
2. Have the two jit-providing objfiles prepare their descriptor with JIT_UNREGISTER
   and the address of their jit objfile
3. Have one of them call __jit_debug_register_code.

We would forget about both jit objfiles, when in reality we should forget only about
one.

I think we'll need to know which jit event breakpoint was hit, so we can know which
jit-providing objfile asked us to process its descriptor.

Simon
Pedro Franco de Carvalho via Gdb-patches June 16, 2020, 9:48 a.m. | #2
On Sunday, June 14, 2020 7:09 PM, Simon Marchi wrote:
> On 2020-05-25 5:38 a.m., Tankut Baris Aktemur via Gdb-patches wrote:

> > diff --git a/gdb/jit.c b/gdb/jit.c

> > index fdb1248ed5b..c689ac3f392 100644

> > --- a/gdb/jit.c

> > +++ b/gdb/jit.c

> > @@ -966,58 +966,53 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,

> 

> The comment above jit_breakpoint_re_set_internal says:

> 

>   Return 0 if the jit breakpoint has been successfully initialized.

> 

> Can you adjust it to the new reality?  Something like "Return 0 if one

> or more JIT breakpoints were successfully initialized".

> 

> I think it would also be good (as a separate cleanup) to make this function

> return a bool, returning true when one or more JIT breakpoints were inserted.


Updated the comment in v2.  Also added a separate patch to make the return type 'bool'.

> > @@ -1357,38 +1352,38 @@ jit_event_handler (struct gdbarch *gdbarch)

> >

> >    jit_program_space_data *ps_data = get_jit_program_space_data ();

> >

> > -  /* Fetch the saved objfile.  */

> > -  objfile *objf = nullptr;

> > -  if (!ps_data->objfile_and_bps.empty ())

> > -    objf = (ps_data->objfile_and_bps.begin ())->first;

> > -

> > -  /* Read the descriptor from remote memory.  */

> > -  if (!jit_read_descriptor (gdbarch, &descriptor, objf))

> > -    return;

> > -  entry_addr = descriptor.relevant_entry;

> > -

> > -  /* Do the corresponding action.  */

> > -  switch (descriptor.action_flag)

> > +  for (auto &objf_and_bp : ps_data->objfile_and_bps)

> >      {

> > -    case JIT_NOACTION:

> > -      break;

> > -    case JIT_REGISTER:

> > -      jit_read_code_entry (gdbarch, entry_addr, &code_entry);

> > -      jit_register_code (gdbarch, entry_addr, &code_entry);

> > -      break;

> > -    case JIT_UNREGISTER:

> > -      objf = jit_find_objf_with_entry_addr (entry_addr);

> > -      if (objf == NULL)

> > -	printf_unfiltered (_("Unable to find JITed code "

> > -			     "entry at address: %s\n"),

> > -			   paddress (gdbarch, entry_addr));

> > -      else

> > -	objf->unlink ();

> > +      objfile *objf = objf_and_bp.first;

> >

> > -      break;

> > -    default:

> > -      error (_("Unknown action_flag value in JIT descriptor!"));

> > -      break;

> > +      /* Read the descriptor from remote memory.  */

> > +      if (!jit_read_descriptor (gdbarch, &descriptor, objf))

> > +	continue;

> > +      entry_addr = descriptor.relevant_entry;

> > +

> > +      /* Do the corresponding action.  */

> > +      switch (descriptor.action_flag)

> > +	{

> > +	case JIT_NOACTION:

> > +	  break;

> > +	case JIT_REGISTER:

> > +	  jit_read_code_entry (gdbarch, entry_addr, &code_entry);

> > +	  jit_register_code (gdbarch, entry_addr, &code_entry);

> > +	  break;

> > +	case JIT_UNREGISTER:

> > +	  objf = jit_find_objf_with_entry_addr (entry_addr);

> > +	  if (objf == NULL)

> > +	    printf_unfiltered (_("Unable to find JITed code "

> > +				 "entry at address: %s\n"),

> > +			       paddress (gdbarch, entry_addr));

> > +	  else

> > +	    objf->unlink ();

> > +

> > +	  break;

> > +	default:

> > +	  error (_("Unknown action_flag value in JIT descriptor!"));

> > +	  break;

> > +	}

> 

> I don't think that iterating on all JIT-providing objfiles is the correct thing

> to do here.  After all, we have hit the __jit_debug_register_code breakpoint of

> exactly one of them, so we should only process the jit descriptor of that one.

> At the same moment, the other JIT-providing objfiles could have prepared anything

> in their descriptor (or there could be junk data), but they have not asked us

> to process it.

> 

> I haven't tried, but I bet that you could build a test to trigger a bug here:

> 

> 1. Have two jit-providing objfiles register one jit objfile each

> 2. Have the two jit-providing objfiles prepare their descriptor with JIT_UNREGISTER

>    and the address of their jit objfile

> 3. Have one of them call __jit_debug_register_code.

> 

> We would forget about both jit objfiles, when in reality we should forget only about

> one.


You're right.

> I think we'll need to know which jit event breakpoint was hit, so we can know which

> jit-providing objfile asked us to process its descriptor.

> 

> Simon


I added a separate patch in v2 to pass the JITer objfile as an argument to jit_event_handler.

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

Patch

diff --git a/gdb/jit.c b/gdb/jit.c
index fdb1248ed5b..c689ac3f392 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -966,58 +966,53 @@  jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
   struct jit_objfile_data *objf_data;
   CORE_ADDR addr;
 
-  objfile *the_objfile = nullptr;
-  if (!ps_data->objfile_and_bps.empty ())
-    the_objfile = ps_data->objfile_and_bps.begin ()->first;
-
-  if (the_objfile == nullptr)
+  for (objfile *the_objfile : current_program_space->objfiles ())
     {
       /* Lookup the registration symbol.  If it is missing, then we
 	 assume we are not attached to a JIT.  */
-      reg_symbol = lookup_bound_minimal_symbol (jit_break_name);
+      reg_symbol = lookup_minimal_symbol (jit_break_name, nullptr,
+					  the_objfile);
       if (reg_symbol.minsym == NULL
 	  || BMSYMBOL_VALUE_ADDRESS (reg_symbol) == 0)
-	return 1;
+	continue;
 
       desc_symbol = lookup_minimal_symbol (jit_descriptor_name, NULL,
 					   reg_symbol.objfile);
       if (desc_symbol.minsym == NULL
 	  || BMSYMBOL_VALUE_ADDRESS (desc_symbol) == 0)
-	return 1;
+	continue;
 
       objf_data = get_jit_objfile_data (reg_symbol.objfile);
       objf_data->register_code = reg_symbol.minsym;
       objf_data->descriptor = desc_symbol.minsym;
 
       the_objfile = reg_symbol.objfile;
-    }
-  else
-    objf_data = get_jit_objfile_data (the_objfile);
 
-  addr = MSYMBOL_VALUE_ADDRESS (the_objfile, objf_data->register_code);
+      addr = MSYMBOL_VALUE_ADDRESS (the_objfile, objf_data->register_code);
 
-  if (jit_debug)
-    fprintf_unfiltered (gdb_stdlog,
-			"jit_breakpoint_re_set_internal, "
-			"breakpoint_addr = %s\n",
-			paddress (gdbarch, addr));
+      if (jit_debug)
+	fprintf_unfiltered (gdb_stdlog,
+			    "jit_breakpoint_re_set_internal, "
+			    "breakpoint_addr = %s\n",
+			    paddress (gdbarch, addr));
 
-  /* Note that this will insert a fresh value if THE_OBJFILE does not
-     exist as a key.  */
-  jit_objfile_bp &info = ps_data->objfile_and_bps[the_objfile];
+      /* Note that this will insert a fresh value if THE_OBJFILE does not
+	 exist as a key.  */
+      jit_objfile_bp &info = ps_data->objfile_and_bps[the_objfile];
 
-  if (info.cached_code_address == addr)
-    return 0;
+      if (info.cached_code_address == addr)
+	continue;
 
-  /* Delete the old breakpoint.  */
-  if (info.jit_breakpoint != nullptr)
-    delete_breakpoint (info.jit_breakpoint);
+      /* Delete the old breakpoint.  */
+      if (info.jit_breakpoint != nullptr)
+	delete_breakpoint (info.jit_breakpoint);
 
-  /* Put a breakpoint in the registration symbol.  */
-  info.cached_code_address = addr;
-  info.jit_breakpoint = create_jit_event_breakpoint (gdbarch, addr);
+      /* Put a breakpoint in the registration symbol.  */
+      info.cached_code_address = addr;
+      info.jit_breakpoint = create_jit_event_breakpoint (gdbarch, addr);
+    }
 
-  return 0;
+  return (ps_data->objfile_and_bps.empty ()) ? 1 : 0;
 }
 
 /* The private data passed around in the frame unwind callback
@@ -1268,40 +1263,40 @@  jit_inferior_init (struct gdbarch *gdbarch)
   if (jit_breakpoint_re_set_internal (gdbarch, ps_data) != 0)
     return;
 
-  /* Fetch the saved objfile.  */
-  objfile *objf = nullptr;
-  if (!ps_data->objfile_and_bps.empty ())
-    objf = (ps_data->objfile_and_bps.begin ())->first;
+  for (auto &objf_and_bp : ps_data->objfile_and_bps)
+    {
+      objfile *objf = objf_and_bp.first;
 
-  /* Read the descriptor so we can check the version number and load
-     any already JITed functions.  */
-  if (!jit_read_descriptor (gdbarch, &descriptor, objf))
-    return;
+      /* Read the descriptor so we can check the version number and load
+	 any already JITed functions.  */
+      if (!jit_read_descriptor (gdbarch, &descriptor, objf))
+	continue;
 
-  /* Check that the version number agrees with that we support.  */
-  if (descriptor.version != 1)
-    {
-      printf_unfiltered (_("Unsupported JIT protocol version %ld "
-			   "in descriptor (expected 1)\n"),
-			 (long) descriptor.version);
-      return;
-    }
+      /* Check that the version number agrees with that we support.  */
+      if (descriptor.version != 1)
+	{
+	  printf_unfiltered (_("Unsupported JIT protocol version %ld "
+			       "in descriptor (expected 1)\n"),
+			     (long) descriptor.version);
+	  continue;
+	}
 
-  /* 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;
-       cur_entry_addr != 0;
-       cur_entry_addr = cur_entry.next_entry)
-    {
-      jit_read_code_entry (gdbarch, cur_entry_addr, &cur_entry);
+      /* 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;
+	   cur_entry_addr != 0;
+	   cur_entry_addr = cur_entry.next_entry)
+	{
+	  jit_read_code_entry (gdbarch, cur_entry_addr, &cur_entry);
 
-      /* This hook may be called many times during setup, so make sure
-	 we don't add the same symbol file twice.  */
-      if (jit_find_objf_with_entry_addr (cur_entry_addr) != NULL)
-	continue;
+	  /* This hook may be called many times during setup, so make sure
+	     we don't add the same symbol file twice.  */
+	  if (jit_find_objf_with_entry_addr (cur_entry_addr) != NULL)
+	    continue;
 
-      jit_register_code (gdbarch, cur_entry_addr, &cur_entry);
+	  jit_register_code (gdbarch, cur_entry_addr, &cur_entry);
+	}
     }
 }
 
@@ -1357,38 +1352,38 @@  jit_event_handler (struct gdbarch *gdbarch)
 
   jit_program_space_data *ps_data = get_jit_program_space_data ();
 
-  /* Fetch the saved objfile.  */
-  objfile *objf = nullptr;
-  if (!ps_data->objfile_and_bps.empty ())
-    objf = (ps_data->objfile_and_bps.begin ())->first;
-
-  /* Read the descriptor from remote memory.  */
-  if (!jit_read_descriptor (gdbarch, &descriptor, objf))
-    return;
-  entry_addr = descriptor.relevant_entry;
-
-  /* Do the corresponding action.  */
-  switch (descriptor.action_flag)
+  for (auto &objf_and_bp : ps_data->objfile_and_bps)
     {
-    case JIT_NOACTION:
-      break;
-    case JIT_REGISTER:
-      jit_read_code_entry (gdbarch, entry_addr, &code_entry);
-      jit_register_code (gdbarch, entry_addr, &code_entry);
-      break;
-    case JIT_UNREGISTER:
-      objf = jit_find_objf_with_entry_addr (entry_addr);
-      if (objf == NULL)
-	printf_unfiltered (_("Unable to find JITed code "
-			     "entry at address: %s\n"),
-			   paddress (gdbarch, entry_addr));
-      else
-	objf->unlink ();
+      objfile *objf = objf_and_bp.first;
 
-      break;
-    default:
-      error (_("Unknown action_flag value in JIT descriptor!"));
-      break;
+      /* Read the descriptor from remote memory.  */
+      if (!jit_read_descriptor (gdbarch, &descriptor, objf))
+	continue;
+      entry_addr = descriptor.relevant_entry;
+
+      /* Do the corresponding action.  */
+      switch (descriptor.action_flag)
+	{
+	case JIT_NOACTION:
+	  break;
+	case JIT_REGISTER:
+	  jit_read_code_entry (gdbarch, entry_addr, &code_entry);
+	  jit_register_code (gdbarch, entry_addr, &code_entry);
+	  break;
+	case JIT_UNREGISTER:
+	  objf = jit_find_objf_with_entry_addr (entry_addr);
+	  if (objf == NULL)
+	    printf_unfiltered (_("Unable to find JITed code "
+				 "entry at address: %s\n"),
+			       paddress (gdbarch, entry_addr));
+	  else
+	    objf->unlink ();
+
+	  break;
+	default:
+	  error (_("Unknown action_flag value in JIT descriptor!"));
+	  break;
+	}
     }
 }
 
diff --git a/gdb/testsuite/gdb.base/jit-reader-simple.exp b/gdb/testsuite/gdb.base/jit-reader-simple.exp
index c036e71c3fb..930c59c0124 100644
--- a/gdb/testsuite/gdb.base/jit-reader-simple.exp
+++ b/gdb/testsuite/gdb.base/jit-reader-simple.exp
@@ -34,6 +34,7 @@  standard_testfile
 set libname $testfile-jit
 set srcfile_lib $srcdir/$subdir/$libname.c
 set binfile_lib [standard_output_file $libname.so]
+set binfile_lib2 [standard_output_file ${libname}2.so]
 
 # Build a standalone JIT binary.
 
@@ -53,12 +54,15 @@  proc build_standalone_jit {{options ""}} {
 
 proc build_shared_jit {{options ""}} {
     global testfile
-    global srcfile_lib binfile_lib
+    global srcfile_lib binfile_lib binfile_lib2
 
     lappend options "debug additional_flags=-fPIC"
     if { [gdb_compile_shlib $srcfile_lib $binfile_lib $options] != "" } {
 	return -1
     }
+    if { [gdb_compile_shlib $srcfile_lib $binfile_lib2 $options] != "" } {
+	return -1
+    }
 
     return 0
 }
@@ -83,6 +87,15 @@  if {[gdb_compile ${srcdir}/${subdir}/${srcfile_dl} $binfile_dl executable \
     return -1
 }
 
+# Build the program that loads *two* JIT libraries.
+set binfile_dl2 $binfile-dl2
+set options [list debug shlib=${binfile_lib} shlib=${binfile_lib2}]
+if {[gdb_compile ${srcdir}/${subdir}/${srcfile_dl} $binfile_dl2 executable \
+	 $options] == -1 } {
+    untested "failed to compile two-jitter binary"
+    return -1
+}
+
 # STANDALONE is true when the JIT reader is included directly in the
 # main program.  False when the JIT reader is in a separate shared
 # library.  If CHANGE_ADDR is true, force changing the JIT descriptor
@@ -160,3 +173,31 @@  foreach standalone {1 0} {
 	}
     }
 }
+
+# Now start the program that loads two JITer libraries and expect to
+# see JIT breakpoints defined for both.
+
+with_test_prefix "two JITers" {
+    clean_restart $binfile_dl2
+
+    if {![runto_main]} {
+	untested "could not run to main for the two-JITer test"
+	return -1
+    }
+
+    set num_bps 0
+    set ws "\[ \t\]+"
+    gdb_test_multiple "maint info breakpoints" "have two jit breakpoints" {
+	-re "jit events${ws}keep y${ws}$hex <__jit_debug_register_code> inf 1\r\n" {
+	    incr num_bps
+	    exp_continue
+	}
+	-re "$gdb_prompt $" {
+	    if {$num_bps == 2} {
+		pass $gdb_test_name
+	    } else {
+		fail $gdb_test_name
+	    }
+	}
+    }
+}