Message-ID: <CAErzpms+qYntOEqbRtvVah_khBTG_ObU6keJyz8zbD9rF8BRLQ@mail.gmail.com>
Date: Tue, 9 Dec 2025 09:42:02 +0800
From: Donglin Peng <dolinux.peng@...il.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: rostedt@...dmis.org, linux-trace-kernel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, pengdonglin <pengdonglin@...omi.com>, 
	Xiaoqin Zhang <zhangxiaoqin@...omi.com>
Subject: Re: [PATCH v2 1/2] fgraph: Use BTF to trim and filter return values

On Tue, Dec 9, 2025 at 8:48 AM Masami Hiramatsu <mhiramat@...nel.org> wrote:
>
> On Mon,  8 Dec 2025 21:19:16 +0800
> Donglin Peng <dolinux.peng@...il.com> wrote:
>
> > From: pengdonglin <pengdonglin@...omi.com>
> >
> > The current funcgraph-retval implementation has two limitations:
> >
> > 1. It prints a return value even when the traced function returns void.
> > 2. When the return type is narrower than a register, the printed value may
> >    be incorrect because high bits can contain undefined data.
> >
> > Both issues are addressed by using BTF to obtain the precise return type
> > of each traced function:
> >
> > - Return values are now printed only for functions whose return type is
> >   not void.
> > - The value is truncated to the actual width of the return type, ensuring
> >   correct representation.
> >
> > These changes make the funcgraph-retval output more accurate and remove
> > noise from void functions.
> >
> > Cc: Steven Rostedt (Google) <rostedt@...dmis.org>
> > Cc: Masami Hiramatsu <mhiramat@...nel.org>
> > Cc: Xiaoqin Zhang <zhangxiaoqin@...omi.com>
> > Signed-off-by: pengdonglin <pengdonglin@...omi.com>
> > ---
> >  kernel/trace/trace_functions_graph.c | 64 +++++++++++++++++++++++-----
> >  1 file changed, 54 insertions(+), 10 deletions(-)
> >
> > diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> > index 17c75cf2348e..9e63665c81e2 100644
> > --- a/kernel/trace/trace_functions_graph.c
> > +++ b/kernel/trace/trace_functions_graph.c
> > @@ -15,6 +15,7 @@
> >
> >  #include "trace.h"
> >  #include "trace_output.h"
> > +#include "trace_btf.h"
> >
> >  /* When set, irq functions might be ignored */
> >  static int ftrace_graph_skip_irqs;
> > @@ -865,6 +866,46 @@ static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_e
> >
> >  #if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
> >
> > +static void trim_retval(unsigned long func, unsigned long *retval, bool *print_retval)
> > +{
> > +     const struct btf_type *t;
> > +     char name[KSYM_NAME_LEN];
> > +     struct btf *btf;
> > +     u32 v, msb;
> > +
> > +     if (!IS_ENABLED(CONFIG_DEBUG_INFO_BTF))
> > +             return;
> > +
> > +     if (lookup_symbol_name(func, name))
> > +             return;
> > +
> > +     t = btf_find_func_proto(name, &btf);
> > +     if (IS_ERR_OR_NULL(t))
> > +             return;
> > +
> > +     t = btf_type_skip_modifiers(btf, t->type, NULL);
> > +     switch (t ? BTF_INFO_KIND(t->info) : BTF_KIND_UNKN) {
> > +     case BTF_KIND_UNKN:
> > +             *print_retval = false;
> > +             break;
> > +     case BTF_KIND_ENUM:
> > +     case BTF_KIND_ENUM64:
> > +             msb = BITS_PER_BYTE * t->size - 1;
> > +             *retval &= GENMASK(msb, 0);
> > +             break;
> > +     case BTF_KIND_INT:
> > +             v = *(u32 *)(t + 1);
> > +             if (BTF_INT_ENCODING(v) == BTF_INT_BOOL)
> > +                     msb = 0;
> > +             else
> > +                     msb = BTF_INT_BITS(v) - 1;
> > +             *retval &= GENMASK(msb, 0);
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +}
>
> Hmm, I think we should have another flag which directly tells the print
> format of retval from BTF.

Thanks, I thought about it and the format flag could have the following values:

1. HEX - hexadecimal (%lx for pointers/unsigned)
2. DEC - decimal (%ld for signed/error_code)
3. BOOL - boolean strings ("true"/"false")

WDYT?

Thanks,
Donglin
>
> > +
> >  static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entry *entry,
> >                               struct ftrace_graph_ret *graph_ret, void *func,
> >                               u32 opt_flags, u32 trace_flags, int args_size)
> > @@ -884,17 +925,20 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> >       print_retaddr = !!(opt_flags & TRACE_GRAPH_PRINT_RETADDR);
> >  #endif
> >
> > -     if (print_retval && retval && !hex_format) {
> > -             /* Check if the return value matches the negative format */
> > -             if (IS_ENABLED(CONFIG_64BIT) && (retval & BIT(31)) &&
> > -                     (((u64)retval) >> 32) == 0) {
> > -                     err_code = sign_extend64(retval, 31);
> > -             } else {
> > -                     err_code = retval;
>
> The original code just guesses the type (signed/unsigned) from
> the retval, but we don't need to do that with BTF.

Thanks. I will fix it in the next version.

Thanks,
Donglin
>
> Thank you,
>
> > +     if (print_retval) {
> > +             trim_retval((unsigned long)func, &retval, &print_retval);
> > +             if (print_retval && retval && !hex_format) {
> > +                     /* Check if the return value matches the negative format */
> > +                     if (IS_ENABLED(CONFIG_64BIT) && (retval & BIT(31)) &&
> > +                             (((u64)retval) >> 32) == 0) {
> > +                             err_code = sign_extend64(retval, 31);
> > +                     } else {
> > +                             err_code = retval;
> > +                     }
> > +
> > +                     if (!IS_ERR_VALUE(err_code))
> > +                             err_code = 0;
> >               }
> > -
> > -             if (!IS_ERR_VALUE(err_code))
> > -                     err_code = 0;
> >       }
> >
> >       if (entry) {
> > --
> > 2.34.1
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists