From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2662 invoked by alias); 26 Mar 2018 17:38:38 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 2651 invoked by uid 89); 26 Mar 2018 17:38:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_STOCKGEN,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=resume, 3*, 2* X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 26 Mar 2018 17:38:36 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 99962EC001; Mon, 26 Mar 2018 17:38:34 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id ED0E9215CDAE; Mon, 26 Mar 2018 17:38:33 +0000 (UTC) Subject: Re: [PATCH] infrun: step through indirect branch thunks To: Markus Metzger , gdb-patches@sourceware.org References: <1519017382-24335-1-git-send-email-markus.t.metzger@intel.com> From: Pedro Alves Message-ID: <4dfab882-016f-01b5-bb28-67cd6637acea@redhat.com> Date: Mon, 26 Mar 2018 17:38:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <1519017382-24335-1-git-send-email-markus.t.metzger@intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-03/txt/msg00535.txt.bz2 On 02/19/2018 05:16 AM, Markus Metzger wrote: > With version 7.3 GCC supports new options > > -mindirect-branch= > -mfunction-return= > > The choices are: > > keep behaves as before > thunk jumps through a thunk > thunk-external jumps through an external thunk > thunk-inline jumps through an inlined thunk > > For thunk and thunk-external, GDB would, on a call to the thunk, step into the > thunk and then resume to its caller assuming that this is an undebuggable > function. On a return thunk, GDB would stop inside the thunk. I was expecting to see the testscase looping over all possible combinations, but only "thunk" is tested, it seems. Why is that? > > The tests assume a fixed number of instruction steps to reach a thunk. This > depends on the compiler as well as the architecture. They may need adjustments > when we add support for more architectures. Or we can simply drop those tests > that cover being able to step into thunks using instruction stepping. The tests sound useful, but isn't there some way we can make them more robust to compiler's whims? Maybe an upper-bounded number of instruction steps until some pattern? > > When using an older GCC, the tests will fail and be reported as untested: I guess you meant s/will fail/will fail to build/ > > Running .../gdb.base/step-indirect-call-thunk.exp ... > gdb compile failed, \ > gcc: error: unrecognized command line option '-mindirect-branch=thunk' > gcc: error: unrecognized command line option '-mfunction-return=thunk' > > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c > index b589d93..8bd7109 100644 > --- a/gdb/amd64-tdep.c > +++ b/gdb/amd64-tdep.c > @@ -3032,6 +3032,57 @@ static const int amd64_record_regmap[] = > AMD64_DS_REGNUM, AMD64_ES_REGNUM, AMD64_FS_REGNUM, AMD64_GS_REGNUM > }; > > +/* Check whether NAME is a register used in an indirect branch thunk. */ > + > +static int > +amd64_is_thunk_register_name (const char *name) Use C++ bool. > +{ > + int reg; > + for (reg = AMD64_RAX_REGNUM; reg < AMD64_RIP_REGNUM; ++reg) for (int reg = AMD64_RAX_REGNUM; reg < AMD64_RIP_REGNUM; ++reg) > + if (strcmp (name, amd64_register_names[reg]) == 0) > + return 1; > + > + return 0; > +} > + > +/* Implement the "in_indirect_branch_thunk" gdbarch function. */ > + > +static int bool. > +amd64_in_indirect_branch_thunk (struct gdbarch *gdbarch, CORE_ADDR pc) > +{ > diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh > index a929e13..0b71df7 100755 > --- a/gdb/gdbarch.sh > +++ b/gdb/gdbarch.sh > @@ -660,6 +660,9 @@ m;CORE_ADDR;skip_solib_resolver;CORE_ADDR pc;pc;;generic_skip_solib_resolver;;0 > # Some systems also have trampoline code for returning from shared libs. > m;int;in_solib_return_trampoline;CORE_ADDR pc, const char *name;pc, name;;generic_in_solib_return_trampoline;;0 > > +# Return non-zero if PC lies inside an indirect branch thunk. > +M;int;in_indirect_branch_thunk;CORE_ADDR pc;pc bool. s/non-zero/true/ > + > # A target might have problems with watchpoints as soon as the stack > # frame of the current function has been destroyed. This mostly happens > # as the first action in a function's epilogue. stack_frame_destroyed_p() > diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c > index cd56642..36d5855 100644 > --- a/gdb/i386-tdep.c > +++ b/gdb/i386-tdep.c Same cosmetic comments apply here. > @@ -4421,6 +4421,57 @@ i386_gnu_triplet_regexp (struct gdbarch *gdbarch) > > > > +/* Check whether NAME is a register used in an indirect branch thunk. */ > + > +static int > +i386_is_thunk_register_name (const char *name) > +{ > + int reg; > + for (reg = I386_EAX_REGNUM; reg < I386_EIP_REGNUM; ++reg) > + if (strcmp (name, i386_register_names[reg]) == 0) > + return 1; > + > + return 0; > +} > + > +/* Implement the "in_indirect_branch_thunk" gdbarch function. */ > + > +static int > +i386_in_indirect_branch_thunk (struct gdbarch *gdbarch, CORE_ADDR pc) > +{ > + struct bound_minimal_symbol bmfun = lookup_minimal_symbol_by_pc (pc); > + if (bmfun.minsym == nullptr) > + return 0; > + > + const char *name = MSYMBOL_LINKAGE_NAME (bmfun.minsym); > + if (name == nullptr) > + return 0; > + > + /* Check the indirect return thunk first. */ > + if (strcmp (name, "__x86_return_thunk") == 0) > + return 1; > + > + /* Then check a family of indirect call/jump thunks. */ > + static const char thunk[] = "__x86_indirect_thunk"; > + static const size_t length = sizeof (thunk) - 1; > + if (strncmp (name, thunk, length) != 0) > + return 0; > + > + /* If that's the complete name, we're in the memory thunk. */ > + name += length; > + if (*name == 0) > + return 1; > + > + /* Check for suffixes. */ > + if (*name++ != '_') > + return 0; > + > + if (i386_is_thunk_register_name (name)) > + return 1; > + > + return 0; > +} Guess all this code could be shared betwee 32-bit/64-bit, if you made this take the names array and a range as parameters: bool x86_in_indirect_branch_thunk (struct gdbarch *gdbarch, CORE_ADDR pc, const char *register_names, int reg_lo, int reg_hi) { ... mostly as above ... } And then: static bool i386_in_indirect_branch_thunk (struct gdbarch *gdbarch, CORE_ADDR pc) { return x86_in_indirect_branch_thunk (gdbarch, pc, i386_register_names, I386_EAX_REGNUM, I386_EIP_REGNUM); } static bool amd64_in_indirect_branch_thunk (struct gdbarch *gdbarch, CORE_ADDR pc) { return x86_in_indirect_branch_thunk (gdbarch, pc, amd64_register_names, AMD64_RAX_REGNUM, AMD64_RIP_REGNUM); } > } > > +/* Check whether PC lies inside an indirect branch thunk. */ > + > +static int > +in_indirect_branch_thunk (struct gdbarch *gdbarch, CORE_ADDR pc) > +{ > + if (!gdbarch_in_indirect_branch_thunk_p (gdbarch)) > + return 0; Do we need to check the _p predicate elsewhere? Why not just make the default return false, and always call the hook? > + > + return gdbarch_in_indirect_branch_thunk (gdbarch, pc); > +} > + > +gdb_test "step" "twice\.1.*" "step into twice ()" > +gdb_test "next" "twice\.2.*" "step through thunks and over inc ()" > +gdb_test "step" "inc\.2.*" "step through call thunk into inc ()" > +gdb_test "step" "inc\.3.*" "step inside inc ()" > +gdb_test "step" "twice\.3.*" "step through return thunk back into twice ()" No trailing " ()" in test names: https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages Either drop the space before (), or drop the ()'s altogether. The other testcase too. Should the gdb.base/ testcase have tests for stepping into the thunks? Thanks, Pedro Alves