Messages in this thread | | | Date | Thu, 1 Apr 2021 19:28:11 +0100 | From | Mark Brown <> | Subject | 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:
> >> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > >> + { (unsigned long) &ftrace_graph_call, 0 }, > >> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > >> + { (unsigned long) ftrace_graph_caller, 0 },
> > 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. [unhandled content-type:application/pgp-signature] | |