[Crash-utility] Re: [PATCH v7 3/5] synchronise cpu context changes between crash/gdb

Hi Kazu, On Fri, Feb 09, 2024 at 05:17:33AM +0000, HAGIO KAZUHITO(萩尾 一仁) wrote: True, i missed that. Atleast on ppc, most dumpfiles should have tt->panic_threads, that should be enough optimisation. I will post a V8 with the 'get_active_task' instead of depending on CURRENT_CONTEXT. Thanks, Aditya Gupta > 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(a)linux.ibm.com&gt; > >>> --- > >>> 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);