Re: [PATCH v2 1/2] fgraph: Use BTF to trim and filter return values

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Mon,  8 Dec 2025 21:19:16 +0800
Donglin Peng <dolinux.peng@gmail.com> wrote:

> From: pengdonglin <pengdonglin@xiaomi.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@goodmis.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Xiaoqin Zhang <zhangxiaoqin@xiaomi.com>
> Signed-off-by: pengdonglin <pengdonglin@xiaomi.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.

> +
>  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.

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@kernel.org>