Patchwork [RFC,v4,8/9] Link frame_info to thread_info

login
register
mail settings
Submitter Philipp Rudo
Date June 12, 2017, 5:08 p.m.
Message ID <20170612170836.25174-9-prudo@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/20966/
State New
Headers show

Comments

Philipp Rudo - June 12, 2017, 5:08 p.m.
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(+)
Yao Qi - June 19, 2017, 1:07 p.m.
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?
Philipp Rudo - June 19, 2017, 3:45 p.m.
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.

Patch

diff --git a/gdb/frame.c b/gdb/frame.c
index 30e4aeab7e..5a2fdda783 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -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
diff --git a/gdb/frame.h b/gdb/frame.h
index 56cbd4400b..26079e8215 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -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