Re: [PATCH] infrun: step through indirect branch thunks

[Pedro Alves <palves at redhat dot com>]

On 02/19/2018 05:16 AM, Markus Metzger wrote:
> With version 7.3 GCC supports new options
> 
>    -mindirect-branch=<choice>
>    -mfunction-return=<choice>
> 
> 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