[RFC,v4,8/9] Link frame_info to thread_info
Commit Message
Linux kernel stacks on S390 are spread over several memory locations.
These locations differ for every kernel task/thread. Thus we need to know
to which task/thread a frame belongs to check whether a stack pointer is
valid and when to stop unwinding. To do so add a pointer to corresponding
thread_info to frame_info.
This connection already exists implicitly by the fact that switch_to_thread
reinitializes the frame cache.
gdb/ChangeLog:
* frame.h (frame_get_thread): New extern declaration.
* frame.c (frame_info) <thread>: New field.
(frame_get_thread): New function.
(create_sentinel_frame, create_new_frame, get_prev_frame_raw): Set
new field.
---
gdb/frame.c | 12 ++++++++++++
gdb/frame.h | 2 ++
2 files changed, 14 insertions(+)
Comments
Philipp Rudo <prudo@linux.vnet.ibm.com> writes:
> Linux kernel stacks on S390 are spread over several memory locations.
> These locations differ for every kernel task/thread. Thus we need to know
> to which task/thread a frame belongs to check whether a stack pointer is
> valid and when to stop unwinding. To do so add a pointer to corresponding
> thread_info to frame_info.
>
> This connection already exists implicitly by the fact that switch_to_thread
> reinitializes the frame cache.
The whole frame cache is created for current thread, from sentinel
frame. When the current thread is changed, the frame cache will be
re-created. If we see different frame_info objects are about different
threads, it must be a bug somewhere. I think frame_info.thread always
points to the current thread, no?
Hi Yao
On Mon, 19 Jun 2017 14:07:11 +0100
Yao Qi <qiyaoltc@gmail.com> wrote:
> Philipp Rudo <prudo@linux.vnet.ibm.com> writes:
>
> > Linux kernel stacks on S390 are spread over several memory locations.
> > These locations differ for every kernel task/thread. Thus we need to know
> > to which task/thread a frame belongs to check whether a stack pointer is
> > valid and when to stop unwinding. To do so add a pointer to corresponding
> > thread_info to frame_info.
> >
> > This connection already exists implicitly by the fact that switch_to_thread
> > reinitializes the frame cache.
>
> The whole frame cache is created for current thread, from sentinel
> frame. When the current thread is changed, the frame cache will be
> re-created. If we see different frame_info objects are about different
> threads, it must be a bug somewhere. I think frame_info.thread always
> points to the current thread, no?
That's also the way I understand it how it should work.
My problem with it is that such implicit connections via global variables are
extremely error prone and hard to debug. For example look at
linux-tdep.c:linux_corefile_thread, here the code looks like this
static void
linux_corefile_thread (struct thread_info *info,
struct linux_corefile_thread_data *args)
{
[...]
old_chain = save_inferior_ptid ();
inferior_ptid = info->ptid;
target_fetch_registers (regcache, -1);
[...]
}
At this point the inferior_ptid is changed without re-creating the frame
cache. Thus the assumption that a existing frame_info always belongs to
current_thread is wrong at this point. What makes it worse is that right after
a target hook is called. So suddenly you have a connection between a
target_obs and a gdbarch you'd never expect which can lead to a bug...
For me the easiest way to prevent such long range bugs is not to use global
variables but explicit connections between the different subsystems.
That's why I made this patch.
Philipp
P.S. I know that the example is bad and you shouldn't use the frame cache to
fetch registers but I hope you get the point I was trying to make.
@@ -101,6 +101,9 @@ struct frame_info
/* The frame's address space. */
struct address_space *aspace;
+ /* The thread this frame belongs to. */
+ struct thread_info *thread;
+
/* The frame's low-level unwinder and corresponding cache. The
low-level unwinder is responsible for unwinding register values
for the previous frame. The low-level unwind methods are
@@ -157,6 +160,12 @@ struct frame_info
const char *stop_string;
};
+struct thread_info *
+frame_get_thread (struct frame_info *frame)
+{
+ return frame->thread;
+}
+
/* A frame stash used to speed up frame lookups. Create a hash table
to stash frames previously accessed from the frame cache for
quicker subsequent retrieval. The hash table is emptied whenever
@@ -1527,6 +1536,7 @@ create_sentinel_frame (struct program_space *pspace, struct regcache *regcache)
frame->level = -1;
frame->pspace = pspace;
frame->aspace = get_regcache_aspace (regcache);
+ frame->thread = inferior_thread ();
/* Explicitly initialize the sentinel frame's cache. Provide it
with the underlying regcache. In the future additional
information, such as the frame's thread will be added. */
@@ -1768,6 +1778,7 @@ create_new_frame (CORE_ADDR addr, CORE_ADDR pc)
/* We currently assume that frame chain's can't cross spaces. */
fi->pspace = fi->next->pspace;
fi->aspace = fi->next->aspace;
+ fi->thread = fi->next->thread;
/* Select/initialize both the unwind function and the frame's type
based on the PC. */
@@ -2179,6 +2190,7 @@ get_prev_frame_raw (struct frame_info *this_frame)
spaces. */
prev_frame->pspace = this_frame->pspace;
prev_frame->aspace = this_frame->aspace;
+ prev_frame->thread = this_frame->thread;
/* Don't yet compute ->unwind (and hence ->type). It is computed
on-demand in get_frame_type, frame_register_unwind, and
@@ -103,6 +103,8 @@ enum frame_id_stack_status
struct frame_info;
+extern struct thread_info *frame_get_thread (struct frame_info *frame);
+
/* The frame object's ID. This provides a per-frame unique identifier
that can be used to relocate a `struct frame_info' after a target
resume or a frame cache destruct. It of course assumes that the