Re: [PATCH] Implement the skip_solib_resolver gdbarch hook for FreeBSD architectures.

[Simon Marchi <simark@simark.ca>]

On 2020-07-11 12:14 p.m., John Baldwin wrote:
> The ELF runtime linker on all FreeBSD architectures uses the
> "_rtld_bind" entry point for unresolved PTL entries.  FreeBSD/mips has
> an additional entry point called "_mips_rtld_bind".
> 
> gdb/ChangeLog:
> 
> 	* fbsd-tdep.c (fbsd_skip_solib_resolver): New function.
> 	(fbsd_init_abi): Install gdbarch "skip_solib_resolver" method.
> 	* fbsd-tdep.h (fbsd_skip_solib_resolver): New prototype.
> 	* mips-fbsd-tdep.c (mips_fbsd_skip_solib_resolver): New function.
> 	(mips_fbsd_init_abi): Install gdbarch "skip_solib_resolver"
> 	method.
> ---
>  gdb/ChangeLog        |  9 +++++++++
>  gdb/fbsd-tdep.c      | 15 +++++++++++++++
>  gdb/fbsd-tdep.h      |  3 +++
>  gdb/mips-fbsd-tdep.c | 16 ++++++++++++++++
>  4 files changed, 43 insertions(+)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index c292927e49..219c2b110b 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,12 @@
> +2020-07-10  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* fbsd-tdep.c (fbsd_skip_solib_resolver): New function.
> +	(fbsd_init_abi): Install gdbarch "skip_solib_resolver" method.
> +	* fbsd-tdep.h (fbsd_skip_solib_resolver): New prototype.
> +	* mips-fbsd-tdep.c (mips_fbsd_skip_solib_resolver): New function.
> +	(mips_fbsd_init_abi): Install gdbarch "skip_solib_resolver"
> +	method.
> +
>  2020-07-10  John Baldwin  <jhb@FreeBSD.org>
>  
>  	* fbsd-nat.h (fbsd_nat_target::supports_multi_process): New
> diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
> index 557c5d3d73..02d1b1fd40 100644
> --- a/gdb/fbsd-tdep.c
> +++ b/gdb/fbsd-tdep.c
> @@ -21,6 +21,7 @@
>  #include "auxv.h"
>  #include "gdbcore.h"
>  #include "inferior.h"
> +#include "objfiles.h"
>  #include "regcache.h"
>  #include "regset.h"
>  #include "gdbthread.h"
> @@ -2071,6 +2072,19 @@ fbsd_get_thread_local_address (struct gdbarch *gdbarch, CORE_ADDR dtv_addr,
>    return addr + offset;
>  }
>  
> +/* Implement the "skip_solib_resolver" gdbarch method.  */

`/* See fbsd-tdep.h.  */` here, and move the actual comment to the .h.

> +
> +CORE_ADDR
> +fbsd_skip_solib_resolver (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{
> +  struct bound_minimal_symbol msym;
> +
> +  msym = lookup_minimal_symbol("_rtld_bind", NULL, NULL);

Space before parenthesis.  For new code, we prefer nullptr rather than NULL (unless
maybe if there are already some NULL in the existing surrounding code in the same
function).

Declare on first use:

  bound_minimal_symbol msym = ...

> +  if (msym.minsym && BMSYMBOL_VALUE_ADDRESS (msym) == pc)

msym.minsym != nullptr

Just curious, why do you have to validate that `BMSYMBOL_VALUE_ADDRESS (msym) == pc`,
in which case would it not be true?

> +    return frame_unwind_caller_pc (get_current_frame ());
> +  return 0;

Just a personal preference, but I think an empty line after an `if` helps readability.

> @@ -462,6 +462,20 @@ static const struct tramp_frame mips64_fbsd_sigframe =
>  
>  /* Shared library support.  */
>  
> +/* FreeBSD/mips can use an alternate routine in the runtime linker to
> +   resolve functions.  */
> +
> +CORE_ADDR
> +mips_fbsd_skip_solib_resolver (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{
> +  struct bound_minimal_symbol msym;
> +
> +  msym = lookup_minimal_symbol("_mips_rtld_bind", NULL, NULL);
> +  if (msym.minsym && BMSYMBOL_VALUE_ADDRESS (msym) == pc)
> +    return frame_unwind_caller_pc (get_current_frame ());
> +  return fbsd_skip_solib_resolver (gdbarch, pc);

Same comments as above.

Simon