On 2024/02/08 20:21, Aditya Gupta wrote: > Hi Kazu, > > On Thu, Feb 08, 2024 at 08:40:26AM +0000, HAGIO KAZUHITO(萩尾 一仁) wrote: >> <...snip...> >> >>> Currently, both crash context and gdb's current thread context were >>> pretty independent, and could be different, for example, crash commands >>> might be working on thread 6 (CPU 5), but GDB passthroughs will be >>> working on thread 2 (CPU 1). >>> >>> This was not a problem earlier since interaction of crash and gdb was >>> not depending on current context for most part. But for gdb passthroughs >> >>> to work correctly, gdb needs register values from crash, which depend on >>> current context in crash. >> >> Is this true? >> >> CURRENT_CONTEXT() is used in ppc64_get_cpu_reg, but get_cpu_reg returns >> a register value from a cpu, so can't we use get_active_task(cpu) and >> task_to_context()? if usable, maybe some of the temporary set_cpu()s >> can be removed. > > Nice idea, true, 'get_active_task' can be used instead of me depending > on CURRENT_CONTEXT. > > But, seems everytime 'get_cpu_reg' will be called, 'get_active_task' > will have to go through all tasks to get the active task. Can I add this > if condition in 'get_active_task' to optimise it: It looks like most (all?) dumpfiles should have tt->panic_threads, so the linear search may not be done with a dumpfile. if (has_cpu && (tt->flags & POPULATE_PANIC)) tt->panic_threads[tc->processor] = tc->task; but yes, if there is a slow command, it's good to optimize. Thanks, Kazu > > --- a/task.c > +++ b/task.c > @@ -6308,6 +6308,11 @@ get_active_task(int cpu) > if (DUMPFILE() && (task = tt->panic_threads[cpu])) > return task; > > + /* return current task if already set to required active task */ > + tc = CURRENT_CONTEXT(); > + if ((tc->processor == cpu) && is_task_active(tc->task)) > + return tc->task; > + > tc = FIRST_CONTEXT(); > for (i = 0; i < RUNNING_TASKS(); i++, tc++) { > if ((tc->processor == cpu) && is_task_active(tc->task)) > >> >> The synchronization between crash and gdb is nice for usability, though. > > Yes, I think it's better to not remove the set_cpu()s done in > gdb_refresh_regcache(), so that there are no issues due to the gdb and > crash contexts not being in sync. > >> >> (it's very helpful if you would rebase the patch set to the latest, when >> you update this.) > > Sure, i will. > > Thanks, > Aditya Gupta > >> >> Thanks, >> Kazu >> >>> >>> Synchronise 'thread' command in gdb with 'set -c' command in crash. >>> 1. crash -> gdb synchronisation: >>> Everytime crash's context changes, a helper is called to switch to >>> the thread on that CPU in gdb. The function has been implemented in >>> crash_target.c, since gdb functions are accessible inside >>> 'crash_target.c', and the thread ID to CPU ID mapping is also done >>> by the crash_target, during initially registering the threads with >>> gdb. With this implementation, GDB's default thread initially also >>> changes to the crashing thread, so a switch to crashing thread >>> manually isn't required anymore >>> >>> 2. gdb -> crash synchronisation: >>> gdb has been patched to call 'set_cpu' whenever user switches to any >>> thread. >>> >>> Signed-off-by: Aditya Gupta <adityag@xxxxxxxxxxxxx> >>> --- >>> crash_target.c | 24 ++++++++++++++++++++++++ >>> defs.h | 3 +++ >>> gdb-10.2.patch | 30 ++++++++++++++++++++++++++++++ >>> kernel.c | 8 +++++++- >>> task.c | 4 +++- >>> 5 files changed, 67 insertions(+), 2 deletions(-) >>> >>> diff --git a/crash_target.c b/crash_target.c >>> index 455480679741..a9d7eea8a80e 100644 >>> --- a/crash_target.c >>> +++ b/crash_target.c >>> @@ -29,6 +29,8 @@ extern "C" int gdb_readmem_callback(unsigned long, void *, int, int); >>> extern "C" int crash_get_nr_cpus(void); >>> extern "C" int crash_get_cpu_reg (int cpu, int regno, const char *regname, >>> int regsize, void *val); >>> +extern "C" int gdb_change_cpu_context (unsigned int cpu); >>> +extern "C" int set_cpu (int cpu); >>> >>> >>> /* The crash target. */ >>> @@ -133,3 +135,25 @@ crash_target_init (void) >>> /* Now, set up the frame cache. */ >>> reinit_frame_cache (); >>> } >>> + >>> +/* >>> + * Change gdb's thread context to the thread on given CPU >>> + **/ >>> +extern "C" int >>> +gdb_change_cpu_context(unsigned int cpu) >>> +{ >>> + ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); >>> + inferior *inf = current_inferior (); >>> + thread_info *tp = find_thread_ptid (inf, ptid); >>> + >>> + if (tp == nullptr) >>> + return FALSE; >>> + >>> + /* Making sure that crash's context is same */ >>> + set_cpu(cpu); >>> + >>> + /* Switch to the thread */ >>> + switch_to_thread(tp); >>> + return TRUE; >>> +} >>> + >>> diff --git a/defs.h b/defs.h >>> index 615f3a37935a..7f7a12753658 100644 >>> --- a/defs.h >>> +++ b/defs.h >>> @@ -7976,4 +7976,7 @@ enum ppc64_regnum { >>> PPC64_VRSAVE_REGNU = 139 >>> }; >>> >>> +/* crash_target.c */ >>> +extern int gdb_change_cpu_context (unsigned int cpu); >>> + >>> #endif /* !GDB_COMMON */ >>> diff --git a/gdb-10.2.patch b/gdb-10.2.patch >>> index 2f7d585105aa..4698079ca343 100644 >>> --- a/gdb-10.2.patch >>> +++ b/gdb-10.2.patch >>> @@ -10,6 +10,7 @@ >>> >>> tar xvzmf gdb-10.2.tar.gz \ >>> gdb-10.2/gdb/symtab.c \ >>> + gdb-10.2/gdb/thread.c \ >>> gdb-10.2/gdb/printcmd.c \ >>> gdb-10.2/gdb/symfile.c \ >>> gdb-10.2/gdb/Makefile.in \ >>> @@ -3237,3 +3238,32 @@ exit 0 >>> >>> for (compunit_symtab *cust : objfile->compunits ()) >>> { >>> +--- gdb-10.2/gdb/thread.c.orig >>> ++++ gdb-10.2/gdb/thread.c >>> +@@ -58,6 +58,11 @@ static int highest_thread_num; >>> + /* The current/selected thread. */ >>> + static thread_info *current_thread_; >>> + >>> ++#ifdef CRASH_MERGE >>> ++/* Function to set cpu, defined by crash-utility */ >>> ++extern "C" void set_cpu (int); >>> ++#endif >>> ++ >>> + /* RAII type used to increase / decrease the refcount of each thread >>> + in a given list of threads. */ >>> + >>> +@@ -1896,7 +1901,13 @@ thread_command (const char *tidstr, int from_tty) >>> + { >>> + ptid_t previous_ptid = inferior_ptid; >>> + >>> +- thread_select (tidstr, parse_thread_id (tidstr, NULL)); >>> ++ struct thread_info* thread_id = parse_thread_id (tidstr, NULL); >>> ++ >>> ++#ifdef CRASH_MERGE >>> ++ set_cpu(thread_id->ptid.tid()); >>> ++#endif >>> ++ >>> ++ thread_select (tidstr, thread_id); >>> + >>> + /* Print if the thread has not changed, otherwise an event will >>> + be sent. */ >>> diff --git a/kernel.c b/kernel.c >>> index 52b7ba09f390..a8d60507dd95 100644 >>> --- a/kernel.c >>> +++ b/kernel.c >>> @@ -6504,7 +6504,13 @@ set_cpu(int cpu) >>> if (hide_offline_cpu(cpu)) >>> error(FATAL, "invalid cpu number: cpu %d is OFFLINE\n", cpu); >>> >>> - if ((task = get_active_task(cpu))) >>> + task = get_active_task(cpu); >>> + >>> + /* Check if context is already set to given cpu */ >>> + if (task == CURRENT_TASK()) >>> + return; >>> + >>> + if (task) >>> set_context(task, NO_PID); >>> else >>> error(FATAL, "cannot determine active task on cpu %ld\n", cpu); >>> diff --git a/task.c b/task.c >>> index ebdb5be3786f..3a190cafbacb 100644 >>> --- a/task.c >>> +++ b/task.c >>> @@ -5301,7 +5301,9 @@ set_context(ulong task, ulong pid) >>> >>> if (found) { >>> CURRENT_CONTEXT() = tc; >>> - return TRUE; >>> + >>> + /* change the selected thread in gdb, according to current context */ >>> + return gdb_change_cpu_context(tc->processor); >>> } else { >>> if (task) >>> error(INFO, "cannot set context for task: %lx\n", task); -- 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