[PATCH] ARM: Remove the special printing format of pc and lr in __show_regs()

[PATCH] ARM: Remove the special printing format of pc and lr in __show_regs()

[Zhen Lei]

Currently, instruction pointers are printed in [<%08lx>] format to make
them more visible. However, it is not necessary in __show_regs() because
they have the prefix 'pc :' or 'lr :', and it is also inconsistent with
that of other registers, which causes misalignment.

Before:
pc : [<8019a48c>]    lr : [<8019a48c>]    psr: 60000013
sp : c0965f28  ip : 00000001  fp : 00000001
r10: be6052d8  r9 : 431bde82  r8 : d7b634db

After:
pc : 8019a46c  lr : 8019a46c  psr: 60000013
sp : c8a71f28  ip : 00000001  fp : 00000002
r10: 1ef6a458  r9 : 431bde82  r8 : d7b634db

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
KernelVersion: v5.19
 arch/arm/kernel/process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 3d9cace63884013..3fb30d734c3568a 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -136,7 +136,7 @@ void __show_regs(struct pt_regs *regs)
 
 	printk("PC is at %pS\n", (void *)instruction_pointer(regs));
 	printk("LR is at %pS\n", (void *)regs->ARM_lr);
-	printk("pc : [<%08lx>]    lr : [<%08lx>]    psr: %08lx\n",
+	printk("pc : %08lx  lr : %08lx  psr: %08lx\n",
 	       regs->ARM_pc, regs->ARM_lr, regs->ARM_cpsr);
 	printk("sp : %08lx  ip : %08lx  fp : %08lx\n",
 	       regs->ARM_sp, regs->ARM_ip, regs->ARM_fp);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: Remove the special printing format of pc and lr in __show_regs()

[Russell King (Oracle)]

On Mon, Aug 01, 2022 at 11:20:16AM +0800, Zhen Lei wrote:
> Currently, instruction pointers are printed in [<%08lx>] format to make
> them more visible. However, it is not necessary in __show_regs() because
> they have the prefix 'pc :' or 'lr :', and it is also inconsistent with
> that of other registers, which causes misalignment.

The formatting is not "to make them more visible" - it was to mark the
addresses that we wanted the ksymoops utility to translate to kernel
symbols before we had kallsyms in the kernel. If one disables kallsyms,
then we still need a way to translate kernel addresses to symbols.

I notice there is a script which helps with this that is part of the
kernel source - scripts/decode_stacktrace.sh. I haven't tried this on
arm32 since I always use kallsyms - and I suspect that is rather
universally true as it avoids needing System.map files etc to decode
the oops. That said, if you're building a kernel for small systems,
you probably don't want the overhead of kallsyms.

So, there's an argument for keeping it - it's an API in that it
provides hints to scripting to identify which values need to be
converted to symbols. There's also the argument for getting rid of it,
which is that hardly anyone does that anymore.

The question is, which is the more important argument, and I don't
think there's a definite answer. So I'm inclined to leave this
as-is.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: Remove the special printing format of pc and lr in __show_regs()

[Leizhen (ThunderTown)]

On 2022/8/8 17:41, Russell King (Oracle) wrote:
> On Mon, Aug 01, 2022 at 11:20:16AM +0800, Zhen Lei wrote:
>> Currently, instruction pointers are printed in [<%08lx>] format to make
>> them more visible. However, it is not necessary in __show_regs() because
>> they have the prefix 'pc :' or 'lr :', and it is also inconsistent with
>> that of other registers, which causes misalignment.
> 
> The formatting is not "to make them more visible" - it was to mark the
> addresses that we wanted the ksymoops utility to translate to kernel
> symbols before we had kallsyms in the kernel. If one disables kallsyms,
> then we still need a way to translate kernel addresses to symbols.

I searched the git log and found that the ksymoops utility is discarded.

See:
073a9ecb3a73401662430bb955aedeac1de643d1
However, a commit in the pre-git era [1] had added the statement,
"ksymoops is useless on 2.6.  Please use the Oops in its original format".

That statement existed until commit 4eb9241127a0 ("Documentation:
admin-guide: update bug-hunting.rst") finally removed the stale
ksymoops information.

4eb9241127a0b5ac3aaaf1b246728009527ebc86
- delete all references to ksymoops since it is no longer applicable;

> 
> I notice there is a script which helps with this that is part of the
> kernel source - scripts/decode_stacktrace.sh. I haven't tried this on
> arm32 since I always use kallsyms - and I suspect that is rather
> universally true as it avoids needing System.map files etc to decode
> the oops. That said, if you're building a kernel for small systems,
> you probably don't want the overhead of kallsyms.

Yes, I read scripts/decode_stacktrace.sh, it requires the format "[<...>]".
But if that's the only concern, maybe we can do the conversion from
"pc: addr" and "lr: addr" to "[<addr>]" first in scripts/decode_stacktrace.sh

I'm usually "objdump -d vmlinux > asm_file", then search "addr:" in asm_file.

Honestly, I think format "[<...>]" is dump_backtrace()'s requirement, not __show_regs()'s.


> 
> So, there's an argument for keeping it - it's an API in that it
> provides hints to scripting to identify which values need to be
> converted to symbols. There's also the argument for getting rid of it,
> which is that hardly anyone does that anymore.
> 
> The question is, which is the more important argument, and I don't
> think there's a definite answer. So I'm inclined to leave this
> as-is.

OK

> 

-- 
Regards,
  Zhen Lei

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: Remove the special printing format of pc and lr in __show_regs()

[Leizhen (ThunderTown)]

On 2022/8/9 10:09, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/8/8 17:41, Russell King (Oracle) wrote:
>> On Mon, Aug 01, 2022 at 11:20:16AM +0800, Zhen Lei wrote:
>>> Currently, instruction pointers are printed in [<%08lx>] format to make
>>> them more visible. However, it is not necessary in __show_regs() because
>>> they have the prefix 'pc :' or 'lr :', and it is also inconsistent with
>>> that of other registers, which causes misalignment.
>>
>> The formatting is not "to make them more visible" - it was to mark the
>> addresses that we wanted the ksymoops utility to translate to kernel
>> symbols before we had kallsyms in the kernel. If one disables kallsyms,
>> then we still need a way to translate kernel addresses to symbols.
> 
> I searched the git log and found that the ksymoops utility is discarded.
> 
> See:
> 073a9ecb3a73401662430bb955aedeac1de643d1
> However, a commit in the pre-git era [1] had added the statement,
> "ksymoops is useless on 2.6.  Please use the Oops in its original format".
> 
> That statement existed until commit 4eb9241127a0 ("Documentation:
> admin-guide: update bug-hunting.rst") finally removed the stale
> ksymoops information.
> 
> 4eb9241127a0b5ac3aaaf1b246728009527ebc86
> - delete all references to ksymoops since it is no longer applicable;
> 
>>
>> I notice there is a script which helps with this that is part of the
>> kernel source - scripts/decode_stacktrace.sh. I haven't tried this on
>> arm32 since I always use kallsyms - and I suspect that is rather
>> universally true as it avoids needing System.map files etc to decode
>> the oops. That said, if you're building a kernel for small systems,
>> you probably don't want the overhead of kallsyms.

Hi, Russell:

I re-examined the code and found that 'pc' and 'lr' had extra printing
in __show_regs(). Therefore, maybe v2 should be changed as follows, as
is done in dump_backtrace_entry():

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 96f3fbd51764292..2b0b49821cbf2f5 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -134,9 +134,15 @@ void __show_regs(struct pt_regs *regs)

        show_regs_print_info(KERN_DEFAULT);

+#ifndef CONFIG_KALLSYMS
+       printk("PC is at [<%08lx>]\n", instruction_pointer(regs));
+       printk("LR is at [<%08lx>]\n", regs->ARM_lr);
+#else
        printk("PC is at %pS\n", (void *)instruction_pointer(regs));
        printk("LR is at %pS\n", (void *)regs->ARM_lr);
-       printk("pc : [<%08lx>]    lr : [<%08lx>]    psr: %08lx\n",
+#endif
+
+       printk("pc : %08lx  lr : %08lx  psr: %08lx\n",
               regs->ARM_pc, regs->ARM_lr, regs->ARM_cpsr);
        printk("sp : %08lx  ip : %08lx  fp : %08lx\n",
               regs->ARM_sp, regs->ARM_ip, regs->ARM_fp);


So that there are no concerns that you mentioned.

CONFIG_KALLSYMS=n:
PC is at [<801993d4>]
LR is at [<801993d4>]
pc : 801993d4  lr : 801993d4  psr: 60000013
sp : c49f9f28  ip : 00000001  fp : 00000001

CONFIG_KALLSYMS=y:
PC is at ktime_get+0x4c/0xe8
LR is at ktime_get+0x4c/0xe8
pc : 8019a4ac  lr : 8019a4ac  psr: 60000013
sp : c49f9f28  ip : 00000001  fp : 00000001


> 
> Yes, I read scripts/decode_stacktrace.sh, it requires the format "[<...>]".
> But if that's the only concern, maybe we can do the conversion from
> "pc: addr" and "lr: addr" to "[<addr>]" first in scripts/decode_stacktrace.sh
> 
> I'm usually "objdump -d vmlinux > asm_file", then search "addr:" in asm_file.
> 
> Honestly, I think format "[<...>]" is dump_backtrace()'s requirement, not __show_regs()'s.
> 
> 
>>
>> So, there's an argument for keeping it - it's an API in that it
>> provides hints to scripting to identify which values need to be
>> converted to symbols. There's also the argument for getting rid of it,
>> which is that hardly anyone does that anymore.
>>
>> The question is, which is the more important argument, and I don't
>> think there's a definite answer. So I'm inclined to leave this
>> as-is.
> 
> OK
> 
>>
> 

-- 
Regards,
  Zhen Lei

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel