git: 28d945204ea1 - main - Handle functions that use a nop in the arm64 fbt

[Jessica Clarke]

> On 3 Mar 2021, at 15:09, Andrew Turner <andrew at freebsd.org> wrote:
> 
>> 
>> On 3 Mar 2021, at 14:59, Jessica Clarke <jrtc27 at freebsd.org> wrote:
>> 
>> On 3 Mar 2021, at 14:55, Andrew Turner <andrew at FreeBSD.org> wrote:
>>> 
>>>> On 3 Mar 2021, at 14:37, Jessica Clarke <jrtc27 at freebsd.org> wrote:
>>>> 
>>>> On 3 Mar 2021, at 14:29, Jessica Clarke <jrtc27 at freebsd.org> wrote:
>>>>> On 3 Mar 2021, at 14:26, Andrew Turner <andrew at FreeBSD.org> wrote:
>>>>>> 
>>>>>> The branch main has been updated by andrew:
>>>>>> 
>>>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=28d945204ea1014d7de6906af8470ed8b3311335
>>>>>> 
>>>>>> commit 28d945204ea1014d7de6906af8470ed8b3311335
>>>>>> Author:     Andrew Turner <andrew at FreeBSD.org>
>>>>>> AuthorDate: 2021-01-13 11:08:19 +0000
>>>>>> Commit:     Andrew Turner <andrew at FreeBSD.org>
>>>>>> CommitDate: 2021-03-03 14:18:03 +0000
>>>>>> 
>>>>>> Handle functions that use a nop in the arm64 fbt
>>>>>> 
>>>>>> To trace leaf asm functions we can insert a single nop instruction as
>>>>>> the first instruction in a function and trigger off this.
>>>>>> 
>>>>>> Reviewed by:    gnn
>>>>>> Sponsored by:   Innovate UK
>>>>>> Differential Revision:  https://reviews.freebsd.org/D28132
>>>>>> ---
>>>>>> sys/arm64/include/asm.h                            |  8 +++-
>>>>>> .../contrib/opensolaris/uts/common/sys/dtrace.h    |  2 +
>>>>>> sys/cddl/dev/dtrace/aarch64/dtrace_subr.c          |  5 +++
>>>>>> sys/cddl/dev/fbt/aarch64/fbt_isa.c                 | 51 ++++++++++++++--------
>>>>>> 4 files changed, 46 insertions(+), 20 deletions(-)
>>>>>> 
>>>>>> diff --git a/sys/arm64/include/asm.h b/sys/arm64/include/asm.h
>>>>>> index 05e618500e59..32b79d256e80 100644
>>>>>> --- a/sys/arm64/include/asm.h
>>>>>> +++ b/sys/arm64/include/asm.h
>>>>>> @@ -38,9 +38,15 @@
>>>>>> 
>>>>>> #define	_C_LABEL(x)	x
>>>>>> 
>>>>>> +#ifdef KDTRACE_HOOKS
>>>>>> +#define	DTRACE_NOP	nop
>>>>>> +#else
>>>>>> +#define	DTRACE_NOP
>>>>>> +#endif
>>>>>> +
>>>>>> #define	LENTRY(sym)						\
>>>>>> 	.text; .align 2; .type sym,#function; sym:		\
>>>>>> -	.cfi_startproc
>>>>>> +	.cfi_startproc; DTRACE_NOP
>>>>>> #define	ENTRY(sym)						\
>>>>>> 	.globl sym; LENTRY(sym)
>>>>> 
>>>>> Doesn't this mean ENTRY incorrectly also has the nop?
>>>> 
>>>> Hm, right, the L in LENTRY means local not leaf. Isn't this a problem
>>>> though? (L)ENTRY are perfectly legal to use for non-leaf assembly
>>>> functions today. Shouldn't there be separate ones specifically for leaf
>>>> functions if you want to treat them differently?
>>> 
>>> Other than early boot handling, pmap_switch, and the exception handlers I think we only have a few non-leaf asm functions on arm64. The only ones I can think of use tail recursion, e.g. memmove -> memcpy when possible. Other than exception handlers these functions don’t have the needed instructions to manage the stack frame as they don’t use any stack space. I decided it was easier to add the nop instruction to the start of function than try to create an unneeded stack frame.
>> 
>> I don't contest that. My problem is that there is now a hidden
>> requirement that (L)ENTRY only be used for leaf functions lest you get
>> broken FBT for them. That is a surprising restriction, which to me
>> should be indicated by having a different macro name from the generic
>> (L)ENTRY shared across most (all?) ports. Despite its flaws, MIPS does
>> have special LEAF macros that are distinct from the others.
> 
> Why would you get broken FBT? All it cares about is finding an instruction it can emulate and replace it with a specific breakpoint. In a non-leaf asm function we will place a nop as the first instruction followed by the standard stack frame manipulation instructions. In this case the nop is unneeded, but should add minimal overhead if such a function is added.
> 
> I only mentioned leaf functions in the commit message as an example of something that we may not have previously been able to trace due to a lack of stack usage.

Oh I see, I didn't read the commit properly, I assumed it was to
watermark leaf functions for better stack traces. My bad.

Jess