Hi Kazu,
On Fri, Feb 09, 2024 at 05:17:33AM +0000, HAGIO KAZUHITO(萩尾 一仁) wrote:
> 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.
>
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@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
