Received-SPF: pass (google.com: domain of linux-kernel+bounces-50885-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240202220459.527138-1-namhyung@kernel.org> <20240202220459.527138-10-namhyung@kernel.org>
In-Reply-To: <20240202220459.527138-10-namhyung@kernel.org>
From: Ian Rogers <irogers@google.com>
Date: Fri, 2 Feb 2024 19:09:05 -0800
Message-ID: <CAP-5=fWhHb8iomEQ_rhwC50kGhPEVbDZv6X6riY_3pr787bhAQ@mail.gmail.com>
Subject: Re: [PATCH 09/14] perf annotate-data: Handle call instructions
To: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Peter Zijlstra <peterz@infradead.org>, 
	Ingo Molnar <mingo@kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
	linux-perf-users@vger.kernel.org, 
	Linus Torvalds <torvalds@linux-foundation.org>, Stephane Eranian <eranian@google.com>, 
	Masami Hiramatsu <mhiramat@kernel.org>, linux-toolchains@vger.kernel.org, 
	linux-trace-devel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 2, 2024 at 2:05=E2=80=AFPM Namhyung Kim <namhyung@kernel.org> w=
rote:
>
> When updating instruction states, the call instruction should play a
> role since it can change the register states.  For simplicity, mark some
> registers as scratch registers (should be arch-dependent), and
> invalidate them all after a function call.

nit: Volatile or caller-save would be a more conventional name than scratch=
.

Thanks,
Ian

> If the function returns something, the designated register (ret_reg)
> will have the type info.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/annotate-data.c | 45 +++++++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-d=
ata.c
> index e46e162c783f..185cb896b9d6 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -23,10 +23,14 @@
>  #include "symbol.h"
>  #include "symbol_conf.h"
>
> -/* Type information in a register, valid when ok is true */
> +/*
> + * Type information in a register, valid when @ok is true.
> + * The @scratch registers are invalidated after a function call.
> + */
>  struct type_state_reg {
>         Dwarf_Die type;
>         bool ok;
> +       bool scratch;
>  };
>
>  /* Type information in a stack location, dynamically allocated */
> @@ -50,6 +54,7 @@ struct type_state_stack {
>  struct type_state {
>         struct type_state_reg regs[TYPE_STATE_MAX_REGS];
>         struct list_head stack_vars;
> +       int ret_reg;
>  };
>
>  static bool has_reg_type(struct type_state *state, int reg)
> @@ -57,10 +62,23 @@ static bool has_reg_type(struct type_state *state, in=
t reg)
>         return (unsigned)reg < ARRAY_SIZE(state->regs);
>  }
>
> -void init_type_state(struct type_state *state, struct arch *arch __maybe=
_unused)
> +void init_type_state(struct type_state *state, struct arch *arch)
>  {
>         memset(state, 0, sizeof(*state));
>         INIT_LIST_HEAD(&state->stack_vars);
> +
> +       if (arch__is(arch, "x86")) {
> +               state->regs[0].scratch =3D true;
> +               state->regs[1].scratch =3D true;
> +               state->regs[2].scratch =3D true;
> +               state->regs[4].scratch =3D true;
> +               state->regs[5].scratch =3D true;
> +               state->regs[8].scratch =3D true;
> +               state->regs[9].scratch =3D true;
> +               state->regs[10].scratch =3D true;
> +               state->regs[11].scratch =3D true;
> +               state->ret_reg =3D 0;
> +       }
>  }
>
>  void exit_type_state(struct type_state *state)
> @@ -417,6 +435,29 @@ void update_insn_state(struct type_state *state, str=
uct data_loc_info *dloc,
>         int fbreg =3D dloc->fbreg;
>         int fboff =3D 0;
>
> +       if (ins__is_call(&dl->ins)) {
> +               Dwarf_Die func_die;
> +
> +               /* __fentry__ will preserve all registers */
> +               if (dl->ops.target.sym &&
> +                   !strcmp(dl->ops.target.sym->name, "__fentry__"))
> +                       return;
> +
> +               /* Otherwise invalidate scratch registers after call */
> +               for (unsigned i =3D 0; i < ARRAY_SIZE(state->regs); i++) =
{
> +                       if (state->regs[i].scratch)
> +                               state->regs[i].ok =3D false;
> +               }
> +
> +               /* Update register with the return type (if any) */
> +               if (die_find_realfunc(cu_die, dl->ops.target.addr, &func_=
die) &&
> +                   die_get_real_type(&func_die, &type_die)) {
> +                       state->regs[state->ret_reg].type =3D type_die;
> +                       state->regs[state->ret_reg].ok =3D true;
> +               }
> +               return;
> +       }
> +
>         /* FIXME: remove x86 specific code and handle more instructions l=
ike LEA */
>         if (!strstr(dl->ins.name, "mov"))
>                 return;
> --
> 2.43.0.594.gd9cf4e227d-goog
>