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

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 25 Jun 2024, at 10:45 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> 
> 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

Sure, I will have this change in V5
> 
>> +
>> +/*
>> + * 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