Hi Tao, > > > <...snip...> > > > > > > Currently I'm still struggling with some failing cases of x86_64 > > > unwinding. So I didn't arrange my patchsets, along with the patch > > > commit log well, since they are all "trial" patches. > > > > > > I agree the patch "ppc64 arbitrary task stack unwind support" is > > > better to go with the ppc patch series. But I suggest we make some > > > modifications for it: > > > > > > 1) I'm OK with it being a stand alone patch, or merging the code > > > changes of this one into your previous patches, but I prefer the > > > latter one :) > > > > > > 2) If you'd like to go with a stand alone patch, could you please > > > rewrite a commit log and title for this one? > > > > Sure, I am okay with either. Let's go with the latter one you suggested, > > in that case I will add the line adding 'need_free' in 'bt_info' > > structure also, to make that work. By the way, shouldn't it be > > 'FREEBUF(bt_info.stackbuf)' instead of 'FREEBUF(pt_regs)' ? > > No it isn't. Let's take a look at the code in > https://github.com/liutgnu/crash-dev/blob/master/x86_64.c#L5006: > x86_64_get_stack_frame(): > > For non active tasks, bt->machdep = user_regs, user_regs is allocated > here and get the regs value from stack frame. However for active > tasks, bt->machdep is get from get_netdump_regs_x86_64(), the values > are get from vmcore's elf note section. > > Then in x86_64_get_cpu_reg(), pt_regs = (struct > x86_64_user_regs_struct *)bt_info.machdep, so the regs value will be > get from pt_regs. And if the pt_regs is allocated in > x86_64_get_stack_frame(), it should be freed. Else if pt_regs are > allocated from get_netdump_regs_x86_64(), then we don't need to do > anything. > > So back to your question, ppc64 case is the same as x86_64, so we > should use FREEBUF(pt_regs) here. > > In additiion, in > https://github.com/liutgnu/crash-dev/blob/master/ppc64.c#L2509: > ppc64_get_cpu_reg(), there is both 'FREEBUF(bt_info.stackbuf)' and > 'FREEBUF(pt_regs)', they deal with different buffer release. Thank you for the detailed explanation ! Understood. After reading, went through the code again, it all makes sense. Thanks, Aditya Gupta > > Thanks, > Tao Liu > > > > > > > > > Also, should I add you as a co-author of the patch ? > > > > Thanks, > > Aditya Gupta > > > > > > > > Thanks, > > > Tao Liu > > > > > > > > > > > > > > To test various gdb passthroughs: > > > > > > > > (crash) set > > > > (crash) set gdb on > > > > gdb> thread > > > > gdb> bt > > > > gdb> info threads > > > > gdb> info threads > > > > gdb> info locals > > > > gdb> info variables irq_rover_lock > > > > gdb> info args > > > > gdb> thread 2 > > > > gdb> set gdb off > > > > (crash) set > > > > (crash) set -c 6 > > > > (crash) gdb thread > > > > (crash) bt > > > > (crash) gdb bt > > > > (crash) frame > > > > (crash) gdb up > > > > (crash) gdb down > > > > (crash) info locals > > > > > > > > Known Issues: > > > > ============= > > > > > > > > 1. In gdb mode, 'bt' might fail to show backtrace in few vmcores collected > > > > from older kernels. This is a known issue due to register mismatch, and > > > > its fix has been merged upstream: > > > > > > > > This can also cause some 'invalid kernel virtual address' errors during gdb > > > > unwinding the stack registers > > > > > > > > Commit: https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef785819e72db79 > > > > > > > > Fixing GDB passthroughs on other architectures > > > > ============================================== > > > > > > > > Much of the work for making gdb passthroughs like 'gdb bt', 'gdb > > > > thread', 'gdb info locals' etc. has been done by the patches introducing > > > > 'machdep->get_cpu_reg' and this series fixing some issues in that. > > > > > > > > Other architectures should be able to fix these gdb functionalities by > > > > simply implementing 'machdep->get_cpu_reg (cpu, regno, ...)'. > > > > > > > > The reasoning behind that has been explained with a diagram in commit > > > > description of patch #1 > > > > > > > > I will assist with my findings/observations fixing it on ppc64 whenever needed. > > > > > > > > Changelog: > > > > ========== > > > > > > > > V9: > > > > + minor change in patch #5: sync gdb context on a 'set' and 'set -p' > > > > + add taoliu's patch for using current context, and fixes in ppc64_get_cpu_reg > > > > > > > > V8: > > > > + use get_active_task instead of depending on CURRENT_CONTEXT in ppc64_get_cpu_reg > > > > + rebase to upstream/master (5977936c0a91) > > > > > > > > V7: > > > > + move changes in gdb-10.2.patch to the end (minor change in patch #3,4,5) > > > > + fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1) > > > > + use ascii diagram in patch #1 description > > > > > > > > V6: > > > > + changes in patch #5: fix bug introduced in v5 that caused initial gdb thread > > > > to be thread 1 > > > > > > > > V5: > > > > + changes in patch #1: made ppc64_get_cpu_reg static, and remove unreachable > > > > code > > > > + changes in patch #3: fixed typo 'ppc64_renum' instead of 'ppc64_regnum', > > > > remove unneeded if condition > > > > + changes in patch #5: implement refresh regcache on per thread, instead of all > > > > threads at once > > > > > > > > V4: > > > > + fix segmentation fault in live debugging (change in patch #1) > > > > + mention live debugging not supported in cover letter and patch #1 > > > > + fixed some checkpatch warnings (change in patch #5) > > > > > > > > V3: > > > > + default gdb thread will be the crashing thread, instead of being > > > > thread '0' > > > > + synchronise crash cpu and gdb thread context > > > > + fix bug in gdb_interface, that replaced gdb's output stream, losing > > > > output in some cases, such as info threads and extra output in info > > > > variables > > > > + fix 'info threads' > > > > > > > > RFC V2: > > > > - removed patch implementing 'frame', 'up', 'down' in crash > > > > - updated the cover letter by removing the mention of those commands other > > > > than the respective gdb passthrough > > > > > > > > Aditya Gupta (5): > > > > ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg > > > > remove 'frame' from prohibited commands list > > > > synchronise cpu context changes between crash/gdb > > > > fix gdb_interface: restore gdb's output streams at end of > > > > gdb_interface > > > > fix 'info threads' command > > > > > > > > crash_target.c | 44 ++++++++++++++++ > > > > defs.h | 130 +++++++++++++++++++++++++++++++++++++++++++++++- > > > > gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++- > > > > gdb_interface.c | 2 +- > > > > kernel.c | 47 +++++++++++++++-- > > > > ppc64.c | 95 +++++++++++++++++++++++++++++++++-- > > > > task.c | 14 ++++++ > > > > tools.c | 2 +- > > > > 8 files changed, 434 insertions(+), 10 deletions(-) > > > > > > > > -- > > > > 2.41.0 > > > > > > > > > > -- Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/ Contribution Guidelines: https://github.com/crash-utility/crash/wiki
