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

[Namhyung Kim <namhyung@kernel.org>]

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
>