2020-05-28 12:56:20

by YuanJunQing

[permalink] [raw]
Subject: [PATCH] function:stacktrace/mips: Fix function:stacktrace for mips

ftrace_call as global symbol in ftrace_caller(), this
will cause function:stacktrace can not work well.
i.e. echo do_IRQ:stacktrace > set_ftrace_filte

Signed-off-by: YuanJunQing <[email protected]>
---
arch/mips/kernel/mcount.S | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S
index cff52b283e03..cd5545764e5f 100644
--- a/arch/mips/kernel/mcount.S
+++ b/arch/mips/kernel/mcount.S
@@ -87,8 +87,15 @@ EXPORT_SYMBOL(_mcount)
PTR_LA t1, _etext
sltu t3, t1, a0 /* t3 = (a0 > _etext) */
or t1, t2, t3
+ PTR_LA t2, stlab-4 /* t2: "function:stacktrace" return address */
+ move a1, AT /* arg2: parent's return address */
beqz t1, ftrace_call
- nop
+ nop /* "function:stacktrace" return address */
+stlab:
+ PTR_LA t2, stlab-4
+ /* ftrace_call_end: ftrace_call return address */
+ beq t2,ra, ftrace_call_end
+ nop
#if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
PTR_SUBU a0, a0, 16 /* arg1: adjust to module's recorded callsite */
#else
@@ -98,7 +105,9 @@ EXPORT_SYMBOL(_mcount)
.globl ftrace_call
ftrace_call:
nop /* a placeholder for the call to a real tracing function */
- move a1, AT /* arg2: parent's return address */
+ move ra, t2 /* t2: "function:stacktrace" return address */
+
+ftrace_call_end:

#ifdef CONFIG_FUNCTION_GRAPH_TRACER
.globl ftrace_graph_call
--
2.17.1


2020-05-29 09:31:41

by YuanJunQing

[permalink] [raw]
Subject: Re: [PATCH] function:stacktrace/mips: Fix function:stacktrace for mips

May I ask if you have received this email.

?? 2020/5/28 ????8:36, YuanJunQing д??:
> ftrace_call as global symbol in ftrace_caller(), this
> will cause function:stacktrace can not work well.
> i.e. echo do_IRQ:stacktrace > set_ftrace_filte
>
> Signed-off-by: YuanJunQing <[email protected]>
> ---
> arch/mips/kernel/mcount.S | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S
> index cff52b283e03..cd5545764e5f 100644
> --- a/arch/mips/kernel/mcount.S
> +++ b/arch/mips/kernel/mcount.S
> @@ -87,8 +87,15 @@ EXPORT_SYMBOL(_mcount)
> PTR_LA t1, _etext
> sltu t3, t1, a0 /* t3 = (a0 > _etext) */
> or t1, t2, t3
> + PTR_LA t2, stlab-4 /* t2: "function:stacktrace" return address */
> + move a1, AT /* arg2: parent's return address */
> beqz t1, ftrace_call
> - nop
> + nop /* "function:stacktrace" return address */
> +stlab:
> + PTR_LA t2, stlab-4
> + /* ftrace_call_end: ftrace_call return address */
> + beq t2,ra, ftrace_call_end
> + nop
> #if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
> PTR_SUBU a0, a0, 16 /* arg1: adjust to module's recorded callsite */
> #else
> @@ -98,7 +105,9 @@ EXPORT_SYMBOL(_mcount)
> .globl ftrace_call
> ftrace_call:
> nop /* a placeholder for the call to a real tracing function */
> - move a1, AT /* arg2: parent's return address */
> + move ra, t2 /* t2: "function:stacktrace" return address */
> +
> +ftrace_call_end:
>
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> .globl ftrace_graph_call

2020-05-29 09:44:01

by WANG Xuerui

[permalink] [raw]
Subject: Re: [PATCH] function:stacktrace/mips: Fix function:stacktrace for mips

On 2020/5/29 17:29, yuanjunqing wrote:

> May I ask if you have received this email.

At least I have received the complete thread in my inbox, so others may
well have no problem.

It's not polite to ping maintainers just one day after sending your
patch BTW; keep the patch submitting guide[1] under your pillow.

Lastly, don't top-post even though your message body is short.


[1]:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#don-t-get-discouraged-or-impatient


And a bit of review while we're at it...

>
> ?? 2020/5/28 ????8:36, YuanJunQing д??:
>> ftrace_call as global symbol in ftrace_caller(), this
>> will cause function:stacktrace can not work well.
Chinglish. I can't understand this despite being a Chinese myself...
>> i.e. echo do_IRQ:stacktrace > set_ftrace_filte
The commit message is truncated? I can't seem to make sense of this line.
>>
>> Signed-off-by: YuanJunQing <[email protected]>
>> ---
>> arch/mips/kernel/mcount.S | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S
>> index cff52b283e03..cd5545764e5f 100644
>> --- a/arch/mips/kernel/mcount.S
>> +++ b/arch/mips/kernel/mcount.S
>> @@ -87,8 +87,15 @@ EXPORT_SYMBOL(_mcount)
>> PTR_LA t1, _etext
>> sltu t3, t1, a0 /* t3 = (a0 > _etext) */
>> or t1, t2, t3
>> + PTR_LA t2, stlab-4 /* t2: "function:stacktrace" return address */
>> + move a1, AT /* arg2: parent's return address */
>> beqz t1, ftrace_call
>> - nop
>> + nop /* "function:stacktrace" return address */
>> +stlab:
>> + PTR_LA t2, stlab-4
>> + /* ftrace_call_end: ftrace_call return address */
>> + beq t2,ra, ftrace_call_end
>> + nop
>> #if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
>> PTR_SUBU a0, a0, 16 /* arg1: adjust to module's recorded callsite */
>> #else
>> @@ -98,7 +105,9 @@ EXPORT_SYMBOL(_mcount)
>> .globl ftrace_call
>> ftrace_call:
>> nop /* a placeholder for the call to a real tracing function */
>> - move a1, AT /* arg2: parent's return address */
>> + move ra, t2 /* t2: "function:stacktrace" return address */
>> +
>> +ftrace_call_end:
>>
>> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> .globl ftrace_graph_call

2020-06-04 01:55:57

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH] function:stacktrace/mips: Fix function:stacktrace for mips

On Fri, 29 May 2020, WANG Xuerui wrote:

> On 2020/5/29 17:29, yuanjunqing wrote:
>
> >> diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S
> >> index cff52b283e03..cd5545764e5f 100644
> >> --- a/arch/mips/kernel/mcount.S
> >> +++ b/arch/mips/kernel/mcount.S
> >> @@ -87,8 +87,15 @@ EXPORT_SYMBOL(_mcount)
> >> PTR_LA t1, _etext
> >> sltu t3, t1, a0 /* t3 = (a0 > _etext) */
> >> or t1, t2, t3
> >> + PTR_LA t2, stlab-4 /* t2: "function:stacktrace" return address */
> >> + move a1, AT /* arg2: parent's return address */
> >> beqz t1, ftrace_call
> >> - nop
> >> + nop /* "function:stacktrace" return address */
> >> +stlab:
> >> + PTR_LA t2, stlab-4
> >> + /* ftrace_call_end: ftrace_call return address */
> >> + beq t2,ra, ftrace_call_end
> >> + nop

Broken delay slot indentation.

> >> #if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
> >> PTR_SUBU a0, a0, 16 /* arg1: adjust to module's recorded callsite */
> >> #else
> >> @@ -98,7 +105,9 @@ EXPORT_SYMBOL(_mcount)
> >> .globl ftrace_call
> >> ftrace_call:
> >> nop /* a placeholder for the call to a real tracing function */
> >> - move a1, AT /* arg2: parent's return address */
> >> + move ra, t2 /* t2: "function:stacktrace" return address */

Likewise. NB I haven't investigated if the change makes sense. A more
detailed explanation in the change description is certainly needed.

Maciej

2020-06-04 03:37:28

by YuanJunQing

[permalink] [raw]
Subject: Re: [PATCH] function:stacktrace/mips: Fix function:stacktrace for mips


?? 2020/6/4 ????9:17, Maciej W. Rozycki д??:
> On Fri, 29 May 2020, WANG Xuerui wrote:
>
>> On 2020/5/29 17:29, yuanjunqing wrote:
>>
>>>> diff --git a/arch/mips/kernel/mcount.S b/arch/mips/kernel/mcount.S
>>>> index cff52b283e03..cd5545764e5f 100644
>>>> --- a/arch/mips/kernel/mcount.S
>>>> +++ b/arch/mips/kernel/mcount.S
>>>> @@ -87,8 +87,15 @@ EXPORT_SYMBOL(_mcount)
>>>> PTR_LA t1, _etext
>>>> sltu t3, t1, a0 /* t3 = (a0 > _etext) */
>>>> or t1, t2, t3
>>>> + PTR_LA t2, stlab-4 /* t2: "function:stacktrace" return address */
>>>> + move a1, AT /* arg2: parent's return address */
>>>> beqz t1, ftrace_call
>>>> - nop
>>>> + nop /* "function:stacktrace" return address */
>>>> +stlab:
>>>> + PTR_LA t2, stlab-4
>>>> + /* ftrace_call_end: ftrace_call return address */
>>>> + beq t2,ra, ftrace_call_end
>>>> + nop
> Broken delay slot indentation.

Thank you for your reply. For this question that you mentioned about the delay slot, I will modify my patch again.

>
>>>> #if defined(KBUILD_MCOUNT_RA_ADDRESS) && defined(CONFIG_32BIT)
>>>> PTR_SUBU a0, a0, 16 /* arg1: adjust to module's recorded callsite */
>>>> #else
>>>> @@ -98,7 +105,9 @@ EXPORT_SYMBOL(_mcount)
>>>> .globl ftrace_call
>>>> ftrace_call:
>>>> nop /* a placeholder for the call to a real tracing function */
>>>> - move a1, AT /* arg2: parent's return address */
>>>> + move ra, t2 /* t2: "function:stacktrace" return address */
> Likewise. NB I haven't investigated if the change makes sense. A more
> detailed explanation in the change description is certainly needed.

I will attach a specific description for further explanation about the second patch later.

>
> Maciej