| Message ID | 20220801032016.1524-1-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: Remove the special printing format of pc and lr in __show_regs() | expand |
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.
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 >
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 > >> >
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);
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(-)