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

[Pedro Alves <palves at redhat dot com>]

Hello Markus,

This is OK with a few minor changes pointed out below.

On 04/09/2018 03:19 PM, Metzger, Markus T wrote:

> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 5986ed6..77b370c 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -979,6 +979,13 @@ gdbarch_skip_prologue_noexcept (gdbarch *gdbarch, CORE_ADDR pc) noexcept
>    return new_pc;
>  }
>  
> +/* See arch-utils.h.  */
> +
> +bool default_in_indirect_branch_thunk (gdbarch *gdbarch, CORE_ADDR pc)

Line break after "bool".

> +{
> +  return false;
> +}
> +
>  void
>  _initialize_gdbarch_utils (void)
>  {

> --- 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 true if PC lies inside an indirect branch thunk.
> +m;bool;in_indirect_branch_thunk;CORE_ADDR pc;pc;;default_in_indirect_branch_thunk;;0

If that last 0 is the default (I never remember off hand), then please write
"false" instead.  Assuming that works.

> diff --git a/gdb/testsuite/gdb.base/step-indirect-call-thunk.exp b/gdb/testsuite/gdb.base/step-indirect-call-thunk.exp
> new file mode 100644
> index 0000000..7f9ab60
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/step-indirect-call-thunk.exp
> @@ -0,0 +1,61 @@
> +# Copyright 2018 Free Software Foundation, Inc.
> +

...

> +
> +proc stepi_until { current target test } {

Please add a describing comment, particularly what
the parameters mean.

> +    global gdb_prompt
> +
> +    gdb_test_multiple "stepi" "$test: stepi" {
> +        -re "$current.*$gdb_prompt $" {
> +            send_gdb "stepi\n"
> +            exp_continue

This should probably have some upper bound.

> +        }
> +        -re "$target.*$gdb_prompt $" {
> +            pass "$test: $target reached"
> +        }
> +        -re "$gdb_prompt $" {
> +            fail "$test: $target not reached"
> +        }

It's better for result diffing, buildbot, etc. if FAIL/PASS
have the same test message.  And with that, the "fail" case
won't be necessary, since it's already taken care of by
gdb_test_multiple.  See below.

> +        timeout {
> +            fail "$test: timeout"
> +        }

A timeout handler is usually not needed because
gdb_test_multiple already handles it.

So that leaves you with something like this, with
upper bound for steps added while at it:

    set step_count 0

    set test "stepi until target"
    gdb_test_multiple "stepi" $test {
        -re "$current.*$gdb_prompt $" {
            send_gdb "stepi\n"
 
            # Cap number of steps just in case we end up
            # stuck in an infinite loop.
	    incr step_count
            if {$step_count < 100} {
	      exp_continue
            } else {
              fail $test
            }
        }
        -re "$target.*$gdb_prompt $" {
            pass $test
        }
    }

Same comments apply to step_until in the other test file.

> +++ b/gdb/x86-tdep.c
> @@ -0,0 +1,75 @@
> +/* Target-dependent code for X86-based targets.
> +
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +#include "x86-tdep.h"

"defs.h" should be included here, at the top, instead of ...

> +++ b/gdb/x86-tdep.h
> @@ -0,0 +1,32 @@
> +
> +#ifndef X86_TDEP_H
> +#define X86_TDEP_H
> +
> +#include "defs.h"
> +

... here.

See <https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Include_Files>.

> +
> +bool
> +x86_in_indirect_branch_thunk (CORE_ADDR pc, const char **register_names,
> +			      int lo, int hi)
> +{

...

> +  if (*name == 0)

Nit, we tend to write instead:

  if (*name == '\0')

> +++ b/gdb/x86-tdep.h
> @@ -0,0 +1,32 @@
> +
> +#ifndef X86_TDEP_H
> +#define X86_TDEP_H
> +
> +#include "defs.h"
> +
Thanks,
Pedro Alves