Hello Lianbo,
On Mon, Dec 18, 2023 at 08:33:59PM +0800, Lianbo Jiang wrote:
> 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 <[email protected]>
> > ---
> > 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'.
Sure, I will do it.
Thanks,
Aditya Gupta
>
>
> Thanks.
>
> Lianbo
>
> > } else {
> > if (task)
>
--
Crash-utility mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
