diff mbox

[RFC,v2,22/27] Export stack_used as __stack_used

Message ID 1465814311-31470-23-git-send-email-gbenson@redhat.com
State New
Headers show

Commit Message

Gary Benson June 13, 2016, 10:38 a.m. UTC
The Infinity note libpthread::thr_iter (broadly equivalent to
libthread_db's td_ta_thr_iter) requires access to both __stack_user
and stack_used, but only the former is visible to it.  This commit
renames "stack_used" as "__stack_used" and changes it from a static
variable in allocatestack.c to an internally exported symbol available
to all nptl source files.
---
 nptl/allocatestack.c      |   33 +++++++++++++++++----------------
 nptl/descr.h              |    2 +-
 nptl/nptl-init.c          |    1 +
 nptl/pthreadP.h           |    2 ++
 nptl/pthread_create.c     |    2 +-
 nptl_db/structs.def       |    2 +-
 nptl_db/td_ta_thr_iter.c  |   15 +++++++++++----
 nptl_db/td_thr_validate.c |    2 +-
 8 files changed, 35 insertions(+), 24 deletions(-)

Comments

Pedro Alves June 13, 2016, 11:02 a.m. UTC | #1
On 06/13/2016 11:38 AM, Gary Benson wrote:
> The Infinity note libpthread::thr_iter (broadly equivalent to
> libthread_db's td_ta_thr_iter) requires access to both __stack_user
> and stack_used, but only the former is visible to it.  This commit
> renames "stack_used" as "__stack_used" and changes it from a static
> variable in allocatestack.c to an internally exported symbol available
> to all nptl source files.

This rename alone should fix glibc PR 17629 (and thus GDB PR 9635).

 [put all symbols nptl_db looks up in the private namespace]
 https://sourceware.org/bugzilla/show_bug.cgi?id=17629

Thus I think it'd be reasonable (and good) to split the renaming part
of this patch out of the series, and get it into the tree.  I
think it could go in quickly.

BTW, the td_ta_thr_iter.c::iterate_thread_list hunk looks
like just a code style change unrelated to the subject of
the patch.

Thanks,
Pedro Alves
Gary Benson June 13, 2016, 11:20 a.m. UTC | #2
Pedro Alves wrote:
> On 06/13/2016 11:38 AM, Gary Benson wrote:
> > The Infinity note libpthread::thr_iter (broadly equivalent to
> > libthread_db's td_ta_thr_iter) requires access to both
> > __stack_user and stack_used, but only the former is visible to it.
> > This commit renames "stack_used" as "__stack_used" and changes it
> > from a static variable in allocatestack.c to an internally
> > exported symbol available to all nptl source files.
> 
> This rename alone should fix glibc PR 17629 (and thus GDB PR 9635).
> 
>  [put all symbols nptl_db looks up in the private namespace]
>  https://sourceware.org/bugzilla/show_bug.cgi?id=17629
> 
> Thus I think it'd be reasonable (and good) to split the renaming
> part of this patch out of the series, and get it into the tree.  I
> think it could go in quickly.

I wasn't aware of that bug.  I'll leave it a day or so to see if
anyone has any comments, then I'll mail just this change alone.

> BTW, the td_ta_thr_iter.c::iterate_thread_list hunk looks like just
> a code style change unrelated to the subject of the patch.

It's not a style change, it stops it falling through into the rest
of the function if next == 0 and !fake_empty.

I agree it should be a separate patch though.

Cheers,
Gary
Gary Benson June 13, 2016, 11:23 a.m. UTC | #3
Gary Benson wrote:
> Pedro Alves wrote:
> > BTW, the td_ta_thr_iter.c::iterate_thread_list hunk looks like
> > just a code style change unrelated to the subject of the patch.
> 
> It's not a style change, it stops it falling through into the rest
> of the function if next == 0 and !fake_empty.
> 
> I agree it should be a separate patch though.

Oh, wait, no, I remember: the reason it's not a separate patch is
that stack_user is statically initialized, so next == 0 never
occurred.  But, changing it to __stack_user changed how it has to
be initialized, so next == 0 can now occur.

So it should be the same patch (but maybe it needs documenting!)

Cheers,
Gary
diff mbox

Patch

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index c044b20..d384397 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -113,7 +113,8 @@  static int stack_cache_lock = LLL_LOCK_INITIALIZER;
 static LIST_HEAD (stack_cache);
 
 /* List of the stacks in use.  */
-static LIST_HEAD (stack_used);
+list_t (__stack_used) __attribute__ ((nocommon));
+hidden_data_def (__stack_used)
 
 /* We need to record what list operations we are going to do so that,
    in case of an asynchronous interruption due to a fork() call, we
@@ -223,7 +224,7 @@  get_cached_stack (size_t *sizep, void **memp)
   stack_list_del (&result->list);
 
   /* And add to the list of stacks in use.  */
-  stack_list_add (&result->list, &stack_used);
+  stack_list_add (&result->list, &__stack_used);
 
   /* And decrease the cache size.  */
   stack_cache_actsize -= result->stackblock_size;
@@ -592,7 +593,7 @@  allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 	  lll_lock (stack_cache_lock, LLL_PRIVATE);
 
 	  /* And add to the list of stacks in use.  */
-	  stack_list_add (&pd->list, &stack_used);
+	  stack_list_add (&pd->list, &__stack_used);
 
 	  lll_unlock (stack_cache_lock, LLL_PRIVATE);
 
@@ -781,7 +782,7 @@  __make_stacks_executable (void **stack_endp)
   lll_lock (stack_cache_lock, LLL_PRIVATE);
 
   list_t *runp;
-  list_for_each (runp, &stack_used)
+  list_for_each (runp, &__stack_used)
     {
       err = change_stack_perm (list_entry (runp, struct pthread, list)
 #ifdef NEED_SEPARATE_REGISTER_STACK
@@ -838,8 +839,8 @@  __reclaim_stacks (void)
 	     pointers at the head of the list are inconsistent.  */
 	  list_t *l = NULL;
 
-	  if (stack_used.next->prev != &stack_used)
-	    l = &stack_used;
+	  if (__stack_used.next->prev != &__stack_used)
+	    l = &__stack_used;
 	  else if (stack_cache.next->prev != &stack_cache)
 	    l = &stack_cache;
 
@@ -861,7 +862,7 @@  __reclaim_stacks (void)
 
   /* Mark all stacks except the still running one as free.  */
   list_t *runp;
-  list_for_each (runp, &stack_used)
+  list_for_each (runp, &__stack_used)
     {
       struct pthread *curp = list_entry (runp, struct pthread, list);
       if (curp != self)
@@ -905,7 +906,7 @@  __reclaim_stacks (void)
     }
 
   /* Add the stack of all running threads to the cache.  */
-  list_splice (&stack_used, &stack_cache);
+  list_splice (&__stack_used, &stack_cache);
 
   /* Remove the entry for the current thread to from the cache list
      and add it to the list of running threads.  Which of the two
@@ -913,13 +914,13 @@  __reclaim_stacks (void)
   stack_list_del (&self->list);
 
   /* Re-initialize the lists for all the threads.  */
-  INIT_LIST_HEAD (&stack_used);
+  INIT_LIST_HEAD (&__stack_used);
   INIT_LIST_HEAD (&__stack_user);
 
   if (__glibc_unlikely (THREAD_GETMEM (self, user_stack)))
     list_add (&self->list, &__stack_user);
   else
-    list_add (&self->list, &stack_used);
+    list_add (&self->list, &__stack_used);
 
   /* There is one thread running.  */
   __nptl_nthreads = 1;
@@ -945,7 +946,7 @@  __find_thread_by_id (pid_t tid)
 
   /* Iterate over the list with system-allocated threads first.  */
   list_t *runp;
-  list_for_each (runp, &stack_used)
+  list_for_each (runp, &__stack_used)
     {
       struct pthread *curp;
 
@@ -1098,7 +1099,7 @@  __nptl_setxid (struct xid_command *cmdp)
 
   /* Iterate over the list with system-allocated threads first.  */
   list_t *runp;
-  list_for_each (runp, &stack_used)
+  list_for_each (runp, &__stack_used)
     {
       struct pthread *t = list_entry (runp, struct pthread, list);
       if (t == self)
@@ -1124,7 +1125,7 @@  __nptl_setxid (struct xid_command *cmdp)
     {
       signalled = 0;
 
-      list_for_each (runp, &stack_used)
+      list_for_each (runp, &__stack_used)
 	{
 	  struct pthread *t = list_entry (runp, struct pthread, list);
 	  if (t == self)
@@ -1154,7 +1155,7 @@  __nptl_setxid (struct xid_command *cmdp)
 
   /* Clean up flags, so that no thread blocks during exit waiting
      for a signal which will never come.  */
-  list_for_each (runp, &stack_used)
+  list_for_each (runp, &__stack_used)
     {
       struct pthread *t = list_entry (runp, struct pthread, list);
       if (t == self)
@@ -1218,7 +1219,7 @@  __pthread_init_static_tls (struct link_map *map)
 
   /* Iterate over the list with system-allocated threads first.  */
   list_t *runp;
-  list_for_each (runp, &stack_used)
+  list_for_each (runp, &__stack_used)
     init_one_static_tls (list_entry (runp, struct pthread, list), map);
 
   /* Now the list with threads using user-allocated stacks.  */
@@ -1239,7 +1240,7 @@  __wait_lookup_done (void)
 
   /* Iterate over the list with system-allocated threads first.  */
   list_t *runp;
-  list_for_each (runp, &stack_used)
+  list_for_each (runp, &__stack_used)
     {
       struct pthread *t = list_entry (runp, struct pthread, list);
       if (t == self || t->header.gscope_flag == THREAD_GSCOPE_FLAG_UNUSED)
diff --git a/nptl/descr.h b/nptl/descr.h
index 8e4938d..87c5354 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -160,7 +160,7 @@  struct pthread
     void *__padding[24];
   };
 
-  /* This descriptor's link on the `stack_used' or `__stack_user' list.  */
+  /* This descriptor's link on the `__stack_used' or `__stack_user' list.  */
   list_t list;
 
   /* Thread ID - which is also a 'is this thread descriptor (and
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index bdbdfed..9bde618 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -384,6 +384,7 @@  __pthread_initialize_minimal_internal (void)
   THREAD_SETMEM (pd, stackblock_size, (size_t) __libc_stack_end);
 
   /* Initialize the list of all running threads with the main thread.  */
+  INIT_LIST_HEAD (&__stack_used);
   INIT_LIST_HEAD (&__stack_user);
   list_add (&pd->list, &__stack_user);
 
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 4edc74b..e762f94 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -183,7 +183,9 @@  extern int __is_smp attribute_hidden;
 
 /* Thread descriptor handling.  */
 extern list_t __stack_user;
+extern list_t __stack_used;
 hidden_proto (__stack_user)
+hidden_proto (__stack_used)
 
 /* Attribute handling.  */
 extern struct pthread_attr *__attr_list attribute_hidden;
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 5216041..ec49b15 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -87,7 +87,7 @@  __find_in_stack_list (struct pthread *pd)
 
   lll_lock (stack_cache_lock, LLL_PRIVATE);
 
-  list_for_each (entry, &stack_used)
+  list_for_each (entry, &__stack_used)
     {
       struct pthread *curp;
 
diff --git a/nptl_db/structs.def b/nptl_db/structs.def
index a9b621b..ebcc065 100644
--- a/nptl_db/structs.def
+++ b/nptl_db/structs.def
@@ -70,7 +70,7 @@  DB_STRUCT (td_eventbuf_t)
 DB_STRUCT_FIELD (td_eventbuf_t, eventnum)
 DB_STRUCT_FIELD (td_eventbuf_t, eventdata)
 
-DB_SYMBOL (stack_used)
+DB_SYMBOL (__stack_used)
 DB_SYMBOL (__stack_user)
 DB_SYMBOL (nptl_version)
 DB_FUNCTION (__nptl_create_event)
diff --git a/nptl_db/td_ta_thr_iter.c b/nptl_db/td_ta_thr_iter.c
index a990fed..aae4dd2 100644
--- a/nptl_db/td_ta_thr_iter.c
+++ b/nptl_db/td_ta_thr_iter.c
@@ -38,15 +38,22 @@  iterate_thread_list (td_thragent_t *ta, td_thr_iter_f *callback,
   if (err != TD_OK)
     return err;
 
-  if (next == 0 && fake_empty)
+  if (next == 0)
     {
       /* __pthread_initialize_minimal has not run.  There is just the main
 	 thread to return.  We cannot rely on its thread register.  They
 	 sometimes contain garbage that would confuse us, left by the
 	 kernel at exec.  So if it looks like initialization is incomplete,
 	 we only fake a special descriptor for the initial thread.  */
-      td_thrhandle_t th = { ta, 0 };
-      return callback (&th, cbdata_p) != 0 ? TD_DBERR : TD_OK;
+      if (fake_empty)
+	{
+	  td_thrhandle_t th = { ta, 0 };
+
+	  if (callback (&th, cbdata_p) != 0)
+	    return TD_DBERR;
+	}
+
+      return TD_OK;
     }
 
   /* Cache the offset from struct pthread to its list_t member.  */
@@ -161,7 +168,7 @@  td_ta_thr_iter (const td_thragent_t *ta_arg, td_thr_iter_f *callback,
 
   /* And the threads with stacks allocated by the implementation.  */
   if (err == TD_OK)
-    err = DB_GET_SYMBOL (list, ta, stack_used);
+    err = DB_GET_SYMBOL (list, ta, __stack_used);
   if (err == TD_OK)
     err = iterate_thread_list (ta, callback, cbdata_p, state, ti_pri,
 			       list, false, pid);
diff --git a/nptl_db/td_thr_validate.c b/nptl_db/td_thr_validate.c
index f3c8a7b..34b6112 100644
--- a/nptl_db/td_thr_validate.c
+++ b/nptl_db/td_thr_validate.c
@@ -70,7 +70,7 @@  td_thr_validate (const td_thrhandle_t *th)
      using implementation allocated stacks.  */
   if (err == TD_NOTHR)
     {
-      err = DB_GET_SYMBOL (list, th->th_ta_p, stack_used);
+      err = DB_GET_SYMBOL (list, th->th_ta_p, __stack_used);
       if (err == TD_OK)
 	err = check_thread_list (th, list, &uninit);