From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 06AB53857C40 for ; Sat, 11 Jul 2020 18:54:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 06AB53857C40 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 0478E1E79B; Sat, 11 Jul 2020 14:54:26 -0400 (EDT) Subject: Re: [PATCH] Implement the skip_solib_resolver gdbarch hook for FreeBSD architectures. To: John Baldwin , gdb-patches@sourceware.org References: <20200711161431.25593-1-jhb@FreeBSD.org> From: Simon Marchi Message-ID: <10b22abc-f273-7a04-fc2f-2d798d8cb03f@simark.ca> Date: Sat, 11 Jul 2020 14:54:24 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20200711161431.25593-1-jhb@FreeBSD.org> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_STOCKGEN, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 11 Jul 2020 18:54:29 -0000 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 > + > + * 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 > > * 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