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

On 12/15/23 15:59, Aditya Gupta wrote: > 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 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. > > 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 | 22 ++++++++++++++++++++++ > defs.h | 3 +++ > gdb-10.2.patch | 30 ++++++++++++++++++++++++++++++ > kernel.c | 8 +++++++- > ppc64.c | 3 --- > task.c | 5 +++++ > 6 files changed, 67 insertions(+), 4 deletions(-) > > diff --git a/crash_target.c b/crash_target.c > index 455480679741..223a51acb1e1 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,23 @@ 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 0c80de72e89e..5e6df14e947d 100644 > --- a/defs.h > +++ b/defs.h > @@ -7978,4 +7978,7 @@ enum ppc64_renum { > 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..8c1b43eb07b7 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 \ > @@ -485,6 +486,35 @@ exit 0 > return best_pst; > } > +--- 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. */ > --- gdb-10.2/gdb/symfile.c.orig > +++ gdb-10.2/gdb/symfile.c > @@ -652,7 +652,26 @@ default_symfile_offsets (struct objfile *objfile, > 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); Good changes. > else > error(FATAL, "cannot determine active task on cpu %ld\n", cpu); > diff --git a/ppc64.c b/ppc64.c > index f5b0b7241ea4..1816972fcf2c 100644 > --- a/ppc64.c > +++ b/ppc64.c > @@ -2542,9 +2542,6 @@ ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size, > return FALSE; > } > - /* FIXME: Always setting the context to CURRENT_CONTEXT irrespective of whicher > - * thread we switched to, in gdb > - */ > tc = CURRENT_CONTEXT(); > BZERO(&bt_setup, sizeof(struct bt_info)); > clone_bt_info(&bt_setup, &bt_info, tc); > diff --git a/task.c b/task.c > index ebdb5be3786f..64088eca29a1 100644 > --- a/task.c > +++ b/task.c > @@ -5301,6 +5301,11 @@ set_context(ulong task, ulong pid) > if (found) { > CURRENT_CONTEXT() = tc; > + > + /* change the selected thread in gdb, according to current context */ > + if ( gdb_change_cpu_context(tc->processor) == FALSE ) > + return FALSE; > + > return TRUE; Because the gdb_change_cpu_context() always returns 'TRUE' or 'FALSE', we can do this changes as below: return gdb_change_cpu_context(tc->processor); And remove the redundant 'return TRUE'.