Re: [RFC PATCH v1 3/4] arm64: Detect FTRACE cases that make the stack trace unreliable

On Thu, Apr 01, 2021 at 12:43:25PM -0500, Madhavan T. Venkataraman wrote:

quoted

quoted

+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+	{ (unsigned long) &ftrace_graph_call, 0 },
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	{ (unsigned long) ftrace_graph_caller, 0 },

quoted

It's weird that we take the address of ftrace_graph_call but not the
other functions - we should be consistent or explain why.  It'd probably
also look nicer to not nest the ifdefs, the dependencies in Kconfig will
ensure we only get things when we should.

I have explained it in the comment in the FTRACE trampoline right above
ftrace_graph_call().

Ah, right - it's a result of it being an inner label.  I'd suggest
putting a brief note right at that line of code explaining this (eg,
"Inner label, not a function"), it wasn't confusing due to the use of
that symbol but rather due to it being different from everything else
in the list and that's kind of lost in the main comment.

So, it is only defined if CONFIG_FUNCTION_GRAPH_TRACER is defined. I can address
this as well as your comment by defining another label whose name is more meaningful
to our use:

+SYM_INNER_LABEL(ftrace_trampoline, SYM_L_GLOBAL) // checked by the unwinder
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller();
        nop                             // If enabled, this will be replaced
                                        // "b ftrace_graph_caller"
#endif

I'm not sure we need to bother with that, you'd still need the & I think.