[PATCH 2/2] Disable breakpoint locations in unloaded jit objects

[Simon Marchi]

On 2020-06-16 7:45 a.m., Mihails Strasuns via Gdb-patches wrote:
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index aead882acd..cc15c47303 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -7487,12 +7487,8 @@ disable_breakpoints_in_shlibs (void)
>    }
>  }
>  
> -/* Disable any breakpoints and tracepoints that are in SOLIB upon
> -   notification of unloaded_shlib.  Only apply to enabled breakpoints,
> -   disabled ones can just stay disabled.  */
> -

I think that this comment should stay attached to the function that actually
does the work, that is shared for shlibs and jit objects.  It perhaps just
need to be adapted to not be specific to solibs.

>  static void
> -disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
> +disable_breakpoints_in_unloaded_objfile (objfile *objfile)
>  {
>    struct bp_location *loc, **locp_tmp;
>    int disabled_shlib_breaks = 0;
> @@ -7502,7 +7498,7 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
>      /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
>      struct breakpoint *b = loc->owner;
>  
> -    if (solib->pspace == loc->pspace
> +    if (objfile->pspace == loc->pspace
>  	&& !loc->shlib_disabled
>  	&& (((b->type == bp_breakpoint
>  	      || b->type == bp_jit_event
> @@ -7510,7 +7506,7 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
>  	     && (loc->loc_type == bp_loc_hardware_breakpoint
>  		 || loc->loc_type == bp_loc_software_breakpoint))
>  	    || is_tracepoint (b))
> -	&& solib_contains_address_p (solib, loc->address))
> +	&& is_addr_in_objfile (loc->address, objfile))

With this change, solib_contains_address_p becomes only used in solib.c, the file it's
defined in.  So another patch on top that makes it static would be nice.

> diff --git a/gdb/jit.c b/gdb/jit.c
> index 1b5ef46469..16e38078bb 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -925,6 +925,27 @@ jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
>    return NULL;
>  }
>  
> +/* This function unregisters JITed code and does the necessary cleanup.  */
> +
> +static void
> +jit_unregister_code (struct gdbarch *gdbarch, CORE_ADDR entry_addr)
> +{
> +  struct objfile *objfile = nullptr;

Declare on first use, below.

> +  if (jit_debug)
> +    fprintf_unfiltered (gdb_stdlog, "jit_unregister_code (%s)\n",

If this is meant to refer to the function name, use __func__, in case
the function changes name.  Also, maybe give a name to the information
you are printing, otherwise it just appears as an address and we don't
really know what it is.  So, something like:

  "%s: objfile=%s", __func__, host_address_to_string (objfile)

I would avoid the parenthesis, unless you are showing the values of the
arguments passed to the funciton (which is not the case here).  And in
fact, won't `objfile` always be nullptr there?  The debug print should
probably be moved lower.

> +			host_address_to_string (objfile));

Newline after the `if`.

> diff --git a/gdb/observable.c b/gdb/observable.c
> index 81aa392cc2..92354c64a3 100644
> --- a/gdb/observable.c
> +++ b/gdb/observable.c
> @@ -46,6 +46,7 @@ DEFINE_OBSERVABLE (inferior_created);
>  DEFINE_OBSERVABLE (record_changed);
>  DEFINE_OBSERVABLE (solib_loaded);
>  DEFINE_OBSERVABLE (solib_unloaded);
> +DEFINE_OBSERVABLE (jit_object_unloaded);
>  DEFINE_OBSERVABLE (new_objfile);
>  DEFINE_OBSERVABLE (free_objfile);
>  DEFINE_OBSERVABLE (new_thread);
> diff --git a/gdb/observable.h b/gdb/observable.h
> index 070cf0f18e..b60ee7f3ef 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -111,6 +111,8 @@ extern observable<struct so_list */* solib */> solib_loaded;
>     been unloaded yet, and thus are still available.  */
>  extern observable<struct so_list */* solib */> solib_unloaded;
>  
> +extern observable<objfile */* objfile */> jit_object_unloaded;

We'll need some doc here.

> diff --git a/gdb/testsuite/gdb.base/jit-elf-reregister.exp b/gdb/testsuite/gdb.base/jit-elf-reregister.exp
> new file mode 100644
> index 0000000000..614b7b9795
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/jit-elf-reregister.exp
> @@ -0,0 +1,78 @@
> +# Copyright 2019 Free Software Foundation, Inc.

2020, unless you copied significant portions of an existing test case, in which
case you should copy the copyright too (whose range should finish in 2020 in any
case).

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +#
> +# Tests global symbol lookup in case a symbol's name in the kernel
> +# coincides with another in the main executable.

That comment is unrelated.  Please add one that's related to this test case.

> +
> +if {[skip_shlib_tests]} {
> +    untested "skipping shared library tests"
> +    return -1
> +}
> +
> +load_lib jit-elf-helpers.exp
> +
> +# The main code that loads and registers JIT objects.
> +set main_basename "jit-elf-reregister"
> +set main_srcfile ${srcdir}/${subdir}/${main_basename}.c
> +set main_binfile [standard_output_file ${main_basename}]
> +
> +# The shared library that gets loaded as JIT objects.
> +set jit_solib_basename jit-elf-solib
> +set jit_solib_srcfile ${srcdir}/${subdir}/${jit_solib_basename}.c
> +
> +# Compile one shared library to use as JIT object.
> +set jit_solibs_target [compile_and_download_n_jit_so \
> +		      $jit_solib_basename $jit_solib_srcfile 1 {debug}]
> +if { $jit_solibs_target == -1 } {
> +    return
> +}
> +
> +proc test_inferior_initial {num} {

Please add a comment to say what this proc does.

> +    global jit_solibs_target main_srcfile main_binfile jit_solib_basename
> +
> +    gdb_test "inferior ${num}"
> +
> +    if { ![runto_main] } {
> +	fail "can't run to main for the inferior ${num}"
> +	return
> +    }
> +
> +    # Poke desired values directly into inferior instead of using "set args"
> +    # because "set args" does not work under gdbserver.

I don't think that's true anymore?

> +    gdb_test_no_output "set var argc=1" "forging argc in inferior ${num}"
> +    gdb_test_no_output "set var argv=fake_argv" "forging argv in inferior ${num}"
> +    set jit_solib_target [lindex $jit_solibs_target 0]
> +    gdb_test_no_output "set var argv\[1\]=\"${jit_solib_target}\"" \
> +    "forging argv\[1\] in inferior ${num}"
> +
> +    gdb_breakpoint [gdb_get_line_number "first-call" $main_srcfile] {temporary}
> +    gdb_continue_to_breakpoint "first-call in inferior ${num}"
> +    gdb_breakpoint "jit_function_0001"
> +    gdb_continue_to_breakpoint "hit before reload inferior ${num}" ".*$jit_solib_basename.*"
> +}
> +
> +# Compile the main code (which loads the JIT objects).
> +if { [compile_jit_main ${main_srcfile} ${main_binfile} {}] != 0 } {
> +    return
> +}
> +
> +clean_restart ${main_binfile}
> +gdb_test "add-inferior -exec ${main_binfile}" "Added inferior 2.*" \
> +"Add second inferior"

Missing indentation.

Simon