Re: RFC: fix PR 13987

[Pedro Alves <palves at redhat dot com>]

Hi Tom,

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

On 09/17/2012 08:35 PM, Tom Tromey wrote:
> This fixes PR 13987.
> 
> The bug is that a PIE executable that uses the JIT API will cause gdb to
> give an error when re-setting the internal JIT breakpoint.  This happens
> because jit.c caches some data and doesn't update the cache when the
> executable is relocated.
> 
> This patch fixes the bug by changing how the JIT breakpoint is managed.
> Now it is handled more like the longjmp master breakpoint: deleted and
> recreated during each re-set.  The cache is still used, but now just to
> hold the relevant minimal symbols used to re-create the breakpoint.

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

It's OK to delete the *_master breakpoints like that, because those are
never installed on the target, only temporary/momentary copies of the
master are, and those survive re-sets.  (It sounds like it's bp_overlay_event
that should be moved away from the scheme for the same reason, but I haven't
really looked at its specifics.)

> 
> Built and regtested on x86-64 Fedora 16.
> New test case included.
> 
> Tom
> 
> 2012-09-17  Tom Tromey  <tromey@redhat.com>
> 
> 	PR gdb/13987:
> 	* jit.c (jit_breakpoint_re_set_internal): Always re-create jit
> 	breakpoint.
> 	* breakpoint.c (internal_bkpt_re_set): Delete jit event
> 	breakpoint.
> 
> 2012-09-17  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb.base/jit.exp (compile_jit_test): New proc.
> 	Add PIE tests.
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index b841bcd..4bf63ed 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -13017,12 +13017,14 @@ internal_bkpt_re_set (struct breakpoint *b)
>  {
>    switch (b->type)
>      {
> -      /* Delete overlay event and longjmp master breakpoints; they
> -	 will be reset later by breakpoint_re_set.  */
> +      /* Delete overlay event, jit event and longjmp master
> +	 breakpoints; they will be reset later by
> +	 breakpoint_re_set.  */
>      case bp_overlay_event:
>      case bp_longjmp_master:
>      case bp_std_terminate_master:
>      case bp_exception_master:
> +    case bp_jit_event:
>        delete_breakpoint (b);
>        break;
>  
> diff --git a/gdb/jit.c b/gdb/jit.c
> index 9e8f295..7c77780 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -967,35 +967,42 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
>    struct objfile *objf;
>    struct jit_objfile_data *objf_data;
>  
> -  if (inf_data->objfile != NULL)
> -    return 0;
> -
> -  /* 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;
> +  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;
>  
> -  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);
> +      jit_inferior_init (gdbarch);
> +    }
> +  else
> +    objf_data = get_jit_objfile_data (inf_data->objfile);
>  
>    if (jit_debug)
> -    fprintf_unfiltered (gdb_stdlog,
> -			"jit_breakpoint_re_set_internal, "
> -			"breakpoint_addr = %s\n",
> -			paddress (gdbarch, SYMBOL_VALUE_ADDRESS (reg_symbol)));
> +    {
> +      CORE_ADDR addr = SYMBOL_VALUE_ADDRESS (objf_data->descriptor);
> +
> +      fprintf_unfiltered (gdb_stdlog,
> +			  "jit_breakpoint_re_set_internal, "
> +			  "breakpoint_addr = %s\n",
> +			  paddress (gdbarch, addr));
> +    }
>  
>    /* Put a breakpoint in the registration symbol.  */
> -  create_jit_event_breakpoint (gdbarch, SYMBOL_VALUE_ADDRESS (reg_symbol));
> +  create_jit_event_breakpoint (gdbarch,
> +			       SYMBOL_VALUE_ADDRESS (objf_data->register_code));
>  
>    return 0;
>  }
> 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"
> +}
> 


-- 
Pedro Alves