This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFC: fix PR 13987


>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Sorry for the delay.  I'm terribly behind on review...

No problem.

Tom> This patch fixes the bug by changing how the JIT breakpoint is
Tom> managed.  Now it is handled more like the longjmp master
Tom> breakpoint: deleted and recreated during each re-set.  The cache is
Tom> still used, but now just to hold the relevant minimal symbols used
Tom> to re-create the breakpoint.

Pedro> Hmm.  I'm not sure this is a good approach.  It seems to add a
Pedro> race where we could miss JIT events in non-stop.  E.g., if a
Pedro> thread causes a DSO load, and while we handle that, we do a
Pedro> breakpoint reset, we remove the jit event breakpoint from the
Pedro> target for a bit.  In this window where the jit even bkpt is not
Pedro> installed, another thread, still running, could pass by the jit
Pedro> event breakpoint location, thus missing a notification to GDB.

Thanks.  This makes sense to me.

What do you think of the appended?  It is basically similar, but rather
than delete and recreate the JIT breakpoint each time, we instead note
its address, and then delete and recreate it only when the address
changes -- basically providing us a simple way to notice the relocation
caused by PIE.

I don't think this approach is vulnerable to the above.

Built and regtested on x86-64 F16.

Tom

2012-10-22  Tom Tromey  <tromey@redhat.com>

	PR gdb/13987:
	* jit.c (struct jit_inferior_data) <cached_code_address,
	jit_breakpoint>: New fields.
	(jit_breakpoint_re_set_internal): Fix logging.  Only create
	breakpoint if cached address has changed.

2012-10-22  Tom Tromey  <tromey@redhat.com>

	* gdb.base/jit.exp (compile_jit_test): New proc.
	Add PIE tests.

diff --git a/gdb/jit.c b/gdb/jit.c
index 9e8f295..21ef4b4 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -239,6 +239,17 @@ struct jit_inferior_data
      symbols.  */
 
   struct objfile *objfile;
+
+  /* If this inferior has __jit_debug_register_code, this is the
+     cached address from the minimal symbol.  This is used to detect
+     relocations requiring the breakpoint to be re-created.  */
+
+  CORE_ADDR cached_code_address;
+
+  /* This is the JIT event breakpoint, or NULL if it has not been
+     set.  */
+
+  struct breakpoint *jit_breakpoint;
 };
 
 /* Per-objfile structure recording the addresses in the inferior.  */
@@ -966,38 +977,51 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
   struct minimal_symbol *reg_symbol, *desc_symbol;
   struct objfile *objf;
   struct jit_objfile_data *objf_data;
+  CORE_ADDR addr;
 
-  if (inf_data->objfile != NULL)
-    return 0;
+  if (inf_data->objfile == NULL)
+    {
+      /* Lookup the registration symbol.  If it is missing, then we
+	 assume we are not attached to a JIT.  */
+      reg_symbol = lookup_minimal_symbol_and_objfile (jit_break_name, &objf);
+      if (reg_symbol == NULL || SYMBOL_VALUE_ADDRESS (reg_symbol) == 0)
+	return 1;
 
-  /* Lookup the registration symbol.  If it is missing, then we assume
-     we are not attached to a JIT.  */
-  reg_symbol = lookup_minimal_symbol_and_objfile (jit_break_name, &objf);
-  if (reg_symbol == NULL || SYMBOL_VALUE_ADDRESS (reg_symbol) == 0)
-    return 1;
+      desc_symbol = lookup_minimal_symbol (jit_descriptor_name, NULL, objf);
+      if (desc_symbol == NULL || SYMBOL_VALUE_ADDRESS (desc_symbol) == 0)
+	return 1;
 
-  desc_symbol = lookup_minimal_symbol (jit_descriptor_name, NULL, objf);
-  if (desc_symbol == NULL || SYMBOL_VALUE_ADDRESS (desc_symbol) == 0)
-    return 1;
+      objf_data = get_jit_objfile_data (objf);
+      objf_data->register_code = reg_symbol;
+      objf_data->descriptor = desc_symbol;
 
-  objf_data = get_jit_objfile_data (objf);
-  objf_data->register_code = reg_symbol;
-  objf_data->descriptor = desc_symbol;
+      inf_data->objfile = objf;
 
-  inf_data->objfile = objf;
+      jit_inferior_init (gdbarch);
+    }
+  else
+    objf_data = get_jit_objfile_data (inf_data->objfile);
 
-  jit_inferior_init (gdbarch);
+  addr = SYMBOL_VALUE_ADDRESS (objf_data->register_code);
 
   if (jit_debug)
     fprintf_unfiltered (gdb_stdlog,
 			"jit_breakpoint_re_set_internal, "
 			"breakpoint_addr = %s\n",
-			paddress (gdbarch, SYMBOL_VALUE_ADDRESS (reg_symbol)));
+			paddress (gdbarch, addr));
+
+  if (inf_data->cached_code_address == addr)
+    return 1;
+
+  /* Delete the old breakpoint.  */
+  if (inf_data->jit_breakpoint != NULL)
+    delete_breakpoint (inf_data->jit_breakpoint);
 
   /* Put a breakpoint in the registration symbol.  */
-  create_jit_event_breakpoint (gdbarch, SYMBOL_VALUE_ADDRESS (reg_symbol));
+  inf_data->cached_code_address = addr;
+  inf_data->jit_breakpoint = create_jit_event_breakpoint (gdbarch, addr);
 
-  return 0;
+  return 1;
 }
 
 /* The private data passed around in the frame unwind callback
diff --git a/gdb/testsuite/gdb.base/jit.exp b/gdb/testsuite/gdb.base/jit.exp
index 3034e6a..9fbc01a 100644
--- a/gdb/testsuite/gdb.base/jit.exp
+++ b/gdb/testsuite/gdb.base/jit.exp
@@ -28,28 +28,38 @@ if {[get_compiler_info]} {
 # test running programs
 #
 
-set testfile jit-main
-set srcfile ${testfile}.c
-set binfile ${objdir}/${subdir}/${testfile}
-if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
-    untested jit.exp
-    return -1
-}
+proc compile_jit_test {testname options} {
+    global testfile srcfile binfile srcdir subdir
+    global solib_testfile solib_srcfile solib_binfile solib_binfile_test_msg
+    global solib_binfile_target
+
+    set testfile jit-main
+    set srcfile ${testfile}.c
+    set binfile [standard_output_file $testfile]
+    if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+	      executable [concat debug $options]] != "" } {
+	untested $testname
+	return -1
+    }
 
-set solib_testfile "jit-solib"
-set solib_srcfile "${srcdir}/${subdir}/${solib_testfile}.c"
-set solib_binfile "${objdir}/${subdir}/${solib_testfile}.so"
-set solib_binfile_test_msg "SHLIBDIR/${solib_testfile}.so"
+    set solib_testfile "jit-solib"
+    set solib_srcfile "${srcdir}/${subdir}/${solib_testfile}.c"
+    set solib_binfile [standard_output_file ${solib_testfile}.so]
+    set solib_binfile_test_msg "SHLIBDIR/${solib_testfile}.so"
+
+    # Note: compiling without debug info: the library goes through
+    # symbol renaming by munging on its symbol table, and that
+    # wouldn't work for .debug sections.  Also, output for "info
+    # function" changes when debug info is resent.
+    if { [gdb_compile_shlib ${solib_srcfile} ${solib_binfile} {-fPIC}] != "" } {
+	untested $testname
+	return -1
+    }
 
-# Note: compiling without debug info: the library goes through symbol
-# renaming by munging on its symbol table, and that wouldn't work for .debug
-# sections.  Also, output for "info function" changes when debug info is resent.
-if { [gdb_compile_shlib ${solib_srcfile} ${solib_binfile} {-fPIC}] != "" } {
-    untested jit.exp
-    return -1
-}
+    set solib_binfile_target [gdb_download ${solib_binfile}]
 
-set solib_binfile_target [gdb_download ${solib_binfile}]
+    return 0
+}
 
 proc one_jit_test {count match_str} { with_test_prefix "one_jit_test-$count" {
     global verbose testfile solib_binfile_target solib_binfile_test_msg
@@ -93,5 +103,17 @@ proc one_jit_test {count match_str} { with_test_prefix "one_jit_test-$count" {
 	"All functions matching regular expression \"jit_function\":"
 }}
 
+if {[compile_jit_test jit.exp {}] < 0} {
+    return
+}
 one_jit_test 1 "${hex}  jit_function_0000"
 one_jit_test 2 "${hex}  jit_function_0000\[\r\n\]+${hex}  jit_function_0001"
+
+with_test_prefix PIE {
+    if {[compile_jit_test "jit.exp PIE tests" \
+	     {additional_flags=-fPIE ldflags=-pie}] < 0} {
+	return
+    }
+
+    one_jit_test 1 "${hex}  jit_function_0000"
+}


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]