Re: [V4 01/16] tools/perf: Move the data structures related to register type to header file

Namhyung Kim
2024-06-25T05:15:51Z

Hello,

On Fri, Jun 14, 2024 at 10:56:16PM +0530, Athira Rajeev wrote:
> Data type profiling uses instruction tracking by checking each
> instruction and updating the register type state in some data
> structures. This is useful to find the data type in cases when the
> register state gets transferred from one reg to another. Example, in
> x86, "mov" instruction and in powerpc, "mr" instruction. Currently these
> structures are defined in annotate-data.c and instruction tracking is
> implemented only for x86. Move these data structures to
> "annotate-data.h" header file so that other arch implementations can use
> it in arch specific files as well.
>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
> tools/perf/util/annotate-data.c | 53 +------------------------------
> tools/perf/util/annotate-data.h | 55 +++++++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 52 deletions(-)
>
> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
> index 965da6c0b542..a4c7f98a75e3 100644
> --- a/tools/perf/util/annotate-data.c
> +++ b/tools/perf/util/annotate-data.c
> @@ -31,15 +31,6 @@
>
> static void delete_var_types(struct die_var_type *var_types);
>
> -enum type_state_kind {
> - TSR_KIND_INVALID = 0,
> - TSR_KIND_TYPE,
> - TSR_KIND_PERCPU_BASE,
> - TSR_KIND_CONST,
> - TSR_KIND_POINTER,
> - TSR_KIND_CANARY,
> -};
> -
> #define pr_debug_dtp(fmt, ...) \
> do { \
> if (debug_type_profile) \
> @@ -140,49 +131,7 @@ static void pr_debug_location(Dwarf_Die *die, u64 pc, int reg)
> }
> }
>
> -/*
> - * Type information in a register, valid when @ok is true.
> - * The @caller_saved registers are invalidated after a function call.
> - */
> -struct type_state_reg {
> - Dwarf_Die type;
> - u32 imm_value;
> - bool ok;
> - bool caller_saved;
> - u8 kind;
> -};
> -
> -/* Type information in a stack location, dynamically allocated */
> -struct type_state_stack {
> - struct list_head list;
> - Dwarf_Die type;
> - int offset;
> - int size;
> - bool compound;
> - u8 kind;
> -};
> -
> -/* FIXME: This should be arch-dependent */
> -#define TYPE_STATE_MAX_REGS 16
> -
> -/*
> - * State table to maintain type info in each register and stack location.
> - * It'll be updated when new variable is allocated or type info is moved
> - * to a new location (register or stack). As it'd be used with the
> - * shortest path of basic blocks, it only maintains a single table.
> - */
> -struct type_state {
> - /* state of general purpose registers */
> - struct type_state_reg regs[TYPE_STATE_MAX_REGS];
> - /* state of stack location */
> - struct list_head stack_vars;
> - /* return value register */
> - int ret_reg;
> - /* stack pointer register */
> - int stack_reg;
> -};
> -
> -static bool has_reg_type(struct type_state *state, int reg)
> +bool has_reg_type(struct type_state *state, int reg)
> {
> return (unsigned)reg < ARRAY_SIZE(state->regs);
> }
> diff --git a/tools/perf/util/annotate-data.h b/tools/perf/util/annotate-data.h
> index 0a57d9f5ee78..ef235b1b15e1 100644
> --- a/tools/perf/util/annotate-data.h
> +++ b/tools/perf/util/annotate-data.h
> @@ -6,6 +6,9 @@
> #include <linux/compiler.h>
> #include <linux/rbtree.h>
> #include <linux/types.h>
> +#include "dwarf-aux.h"
> +#include "annotate.h"
> +#include "debuginfo.h"
>
> struct annotated_op_loc;
> struct debuginfo;
> @@ -15,6 +18,15 @@ struct hist_entry;
> struct map_symbol;
> struct thread;
>
> +enum type_state_kind {
> + TSR_KIND_INVALID = 0,
> + TSR_KIND_TYPE,
> + TSR_KIND_PERCPU_BASE,
> + TSR_KIND_CONST,
> + TSR_KIND_POINTER,
> + TSR_KIND_CANARY,
> +};
> +
> /**
> * struct annotated_member - Type of member field
> * @node: List entry in the parent list
> @@ -142,6 +154,48 @@ struct annotated_data_stat {
> };
> extern struct annotated_data_stat ann_data_stat;
>
> +/*
> + * Type information in a register, valid when @ok is true.
> + * The @caller_saved registers are invalidated after a function call.
> + */
> +struct type_state_reg {
> + Dwarf_Die type;
> + u32 imm_value;
> + bool ok;
> + bool caller_saved;
> + u8 kind;
> +};
> +
> +/* Type information in a stack location, dynamically allocated */
> +struct type_state_stack {
> + struct list_head list;
> + Dwarf_Die type;
> + int offset;
> + int size;
> + bool compound;
> + u8 kind;
> +};
> +
> +/* FIXME: This should be arch-dependent */
> +#define TYPE_STATE_MAX_REGS 32

Can you please define this for powerpc separately? I think x86 should
remain in 16.

Thanks,
Namhyung

> +
> +/*
> + * State table to maintain type info in each register and stack location.
> + * It'll be updated when new variable is allocated or type info is moved
> + * to a new location (register or stack). As it'd be used with the
> + * shortest path of basic blocks, it only maintains a single table.
> + */
> +struct type_state {
> + /* state of general purpose registers */
> + struct type_state_reg regs[TYPE_STATE_MAX_REGS];
> + /* state of stack location */
> + struct list_head stack_vars;
> + /* return value register */
> + int ret_reg;
> + /* stack pointer register */
> + int stack_reg;
> +};
> +
> #ifdef HAVE_DWARF_SUPPORT
>
> /* Returns data type at the location (ip, reg, offset) */
> @@ -160,6 +214,7 @@ void global_var_type__tree_delete(struct rb_root *root);
>
> int hist_entry__annotate_data_tty(struct hist_entry *he, struct evsel *evsel);
>
> +bool has_reg_type(struct type_state *state, int reg);
> #else /* HAVE_DWARF_SUPPORT */
>
> static inline struct annotated_data_type *
> --
> 2.43.0
>