Re: [PATCH v10 2/8] tracing: Add documentation for funcgraph-retval and funcgraph-retval-hex

[Mark Rutland]

On Tue, Apr 04, 2023 at 08:02:44PM +0800, Donglin Peng wrote:
> On 2023/4/3 16:30, Mark Rutland wrote:
> > On Fri, Mar 31, 2023 at 05:47:38AM -0700, Donglin Peng wrote:

> > > +At present, there are some limitations when using the funcgraph-retval
> > > +option, and these limitations will be eliminated in the future:
> > > +
> > > +- Even if the function return type is void, a return value will still
> > > +  be printed, and you can just ignore it.
> > > +
> > > +- Even if return values are stored in multiple registers, only the
> > > +  value contained in the first register will be recorded and printed.
> > > +  To illustrate, in the x86 architecture, eax and edx are used to store
> > > +  a 64-bit return value, with the lower 32 bits saved in eax and the
> > > +  upper 32 bits saved in edx. However, only the value stored in eax
> > > +  will be recorded and printed.
> > 
> > With some procedure call standards (e.g. arm64's AAPCS64), when a type is
> > smaller than a GPR it's up to the consumer to perform the narrowing, and the
> > upport bits may contain UNKNOWN values. For example, with a u8 in a 64-bit GPR,
> > bits [3:8] may contain arbitrary values.
> 
> Thank you. Just to clarify, Should it be that bits [63:8] may contain
> arbitrary values in such cases?

Yes; I meant to say bits [63:8].

> > It's probably worth noting that this means *some* manual processing will always
> > be necessary for such cases.
> > 
> > That's mostly visible around where largelr types get truncated (whether
> > explciitly or implicitly), e.g.
> > 
> > 	u8 narrow_to_u8(u64 val)
> > 	{
> > 		// implicitly truncated
> > 		return val;
> > 	}
> > 
> > ... could be compiled to:
> > 
> > 	narrow_to_u8:
> > 		< ... ftrace instrumentation ... >
> > 		RET
> > 
> > ... and so:
> > 	
> > 	narrow_to_u8(0x123456789abcdef);
> > 
> > ... might be recorded as returning 0x123456789abcdef rather than 0xef.
> > 
> > 
> > That can happen in surprising ways, e.g.
> > 
> > 	int error_if_not_4g_aligned(u64 val)
> > 	{
> > 		if (val & GENMASK(63, 32))
> 
> Should it be GENMASK(31, 0)?

Yes; whoops!

> > 			return -EINVAL;
> > 
> > 		return 0;
> > 	}
> > 
> > ... could be compiled to:
> > 
> > 	error_if_not_4g_aligned:
> > 		CBNZ	w0, .Lnot_aligned
> > 		RET				// bits [31:0] are zero, bits
> > 						// [63:32] are UNKNOWN
> > 	.Lnot_aligned:
> > 		MOV	x0, #-EINVAL
> > 		RET
> > 
> > .... and so:
> > 
> > 	error_if_not_4g_aligned(SZ_8G)
> > 
> > ... could return with bits [63:32] non-zero
> > 
> > Thanks,
> > Mark.
> 
> Thank you for sharing this note. I will append the following limitation.

That looks great; thanks!

> In certain procedure call standards, such as arm64's AAPCS64, when a
> type is smaller than a GPR, it is the responsibility of the consumer to
> perform the narrowing, and the upper bits may contain UNKNOWN values.
> Therefore, it is advisable to check the code for such cases. For
> instance,when using a u8 in a 64-bit GPR, bits [63:8] may contain
          ^
Minor nit: missing space here.

> arbitrary values, especially when larger types are truncated, whether
> explicitly or implicitly. Here are some specific cases to illustrate
> this point:
> 
>  - Case One:
> 
>   The function narrow_to_u8 is defined as follows:
> 
>  	u8 narrow_to_u8(u64 val)
> 	{
> 		// implicitly truncated
> 		return val;
> 	}
> 
>   It may be compiled to:
> 
> 	narrow_to_u8:
> 		< ... ftrace instrumentation ... >
> 		RET
> 
>   If you pass 0x123456789abcdef to this function and want to narrow it,
>   it may be recorded as 0x123456789abcdef instead of 0xef.
> 
>   - Case Two:
> 
>   The function error_if_not_4g_aligned is defined as follows:
> 
> 	int error_if_not_4g_aligned(u64 val)
> 	{
> 		if (val & GENMASK(31, 0))
> 			return -EINVAL;
> 
> 		return 0;
> 	}
> 
>   It could be compile to:
                ^^^^^^^

Minor nit: s/compile/compiled here.

Thanks,
Mark.

> 
> 	error_if_not_4g_aligned:
> 		CBNZ	w0, .Lnot_aligned
> 		RET				// bits [31:0] are zero, bits
> 						// [63:32] are UNKNOWN
> 	.Lnot_aligned:
> 		MOV	x0, #-EINVAL
> 		RET
> 
>   When passing 0x2_0000_0000 to it, the return value may be recorded as
>   0x2_0000_0000 instead of 0.