diff mbox

nptl: support thread stacks that grow up

Message ID 1439108237-31537-1-git-send-email-vapier@gentoo.org
State Committed
Delegated to: Mike Frysinger
Headers show

Commit Message

Mike Frysinger Aug. 9, 2015, 8:17 a.m. UTC
From: Carlos O'Donell <carlos@systemhalted.org>

Gentoo has been carrying this for all arches since 2.17.

URL: http://bugs.gentoo.org/301642

2015-08-09  Carlos O'Donell  <carlos@systemhalted.org>

	* nptl/allocatestack.c (allocate_stack): Declare new stackaddr,
	assign attr->stackaddr to it, and adjust it down when
	_STACK_GROWS_UP.  Change all attr->stackaddr to stackaddr.
	[_STACK_GROWS_UP]: Delete assert.
	* nptl/pthread_create.c (START_THREAD_DEFN) [!_STACK_GROWS_DOWN]:
	Implement stack grows up logic.
	* nptl/pthread_getattr_np.c (pthread_getattr_np): Implement
	stack grows up logic.
---
 nptl/allocatestack.c      | 20 +++++++++++++-------
 nptl/pthread_create.c     | 19 ++++++++++++++++---
 nptl/pthread_getattr_np.c | 17 +++++++++++++++--
 3 files changed, 44 insertions(+), 12 deletions(-)

Comments

Mike Frysinger Dec. 29, 2015, 10:57 p.m. UTC | #1
ping: https://sourceware.org/ml/libc-alpha/2015-08/msg00277.html
-mike
Mike Frysinger Jan. 9, 2016, 8:32 a.m. UTC | #2
On 29 Dec 2015 17:57, Mike Frysinger wrote:
> ping: https://sourceware.org/ml/libc-alpha/2015-08/msg00277.html

ping^2.  would be nice to get into the next release, or at least some
feedback.  Gentoo has been carrying this for a while now for all arches.
-mike
Roland McGrath Jan. 12, 2016, 9:03 p.m. UTC | #3
It looks OK to me.  If you can verify that it doesn't change the generated
code nontrivially on x86_64, then I think that is sufficient to declare it
safe enough for working machines.  And grows-up machines are sufficiently
broken today in trunk that I'm not concerned about you making them worse.
Mike Frysinger Jan. 12, 2016, 10 p.m. UTC | #4
On 12 Jan 2016 13:03, Roland McGrath wrote:
> It looks OK to me.  If you can verify that it doesn't change the generated
> code nontrivially on x86_64, then I think that is sufficient to declare it
> safe enough for working machines.  And grows-up machines are sufficiently
> broken today in trunk that I'm not concerned about you making them worse.

pthread_create.c & pthread_getattr_np.c have mostly the same disassembly.
my gcc is being weird and changing a constant in one insn depending on
whitespace in the file.

allocatestack.c has some code shuffling, but it's the same size overall.

tests still pass on x86_64.

i'll push after the 2.23 branch.
-mike
Carlos O'Donell Jan. 13, 2016, 12:07 a.m. UTC | #5
On 01/12/2016 05:00 PM, Mike Frysinger wrote:
> On 12 Jan 2016 13:03, Roland McGrath wrote:
>> It looks OK to me.  If you can verify that it doesn't change the generated
>> code nontrivially on x86_64, then I think that is sufficient to declare it
>> safe enough for working machines.  And grows-up machines are sufficiently
>> broken today in trunk that I'm not concerned about you making them worse.
> 
> pthread_create.c & pthread_getattr_np.c have mostly the same disassembly.
> my gcc is being weird and changing a constant in one insn depending on
> whitespace in the file.
> 
> allocatestack.c has some code shuffling, but it's the same size overall.
> 
> tests still pass on x86_64.
> 
> i'll push after the 2.23 branch.

Thanks. To be clear I have tested these patches on x86_64 and they passed
with no regressions.

Cheers,
Carlos.
diff mbox

Patch

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 753da61..f182e9a 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -372,6 +372,13 @@  allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
   if (__glibc_unlikely (attr->flags & ATTR_FLAG_STACKADDR))
     {
       uintptr_t adj;
+      char *stackaddr = (char *) attr->stackaddr;
+
+      /* Assume the same layout as the _STACK_GROWS_DOWN case, with struct
+	 pthread at the top of the stack block.  Later we adjust the guard
+	 location and stack address to match the _STACK_GROWS_UP case.  */
+      if (_STACK_GROWS_UP)
+	stackaddr += attr->stacksize;
 
       /* If the user also specified the size of the stack make sure it
 	 is large enough.  */
@@ -381,11 +388,11 @@  allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 
       /* Adjust stack size for alignment of the TLS block.  */
 #if TLS_TCB_AT_TP
-      adj = ((uintptr_t) attr->stackaddr - TLS_TCB_SIZE)
+      adj = ((uintptr_t) stackaddr - TLS_TCB_SIZE)
 	    & __static_tls_align_m1;
       assert (size > adj + TLS_TCB_SIZE);
 #elif TLS_DTV_AT_TP
-      adj = ((uintptr_t) attr->stackaddr - __static_tls_size)
+      adj = ((uintptr_t) stackaddr - __static_tls_size)
 	    & __static_tls_align_m1;
       assert (size > adj);
 #endif
@@ -395,10 +402,10 @@  allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 	 the stack.  It is the user's responsibility to do this if it
 	 is wanted.  */
 #if TLS_TCB_AT_TP
-      pd = (struct pthread *) ((uintptr_t) attr->stackaddr
+      pd = (struct pthread *) ((uintptr_t) stackaddr
 			       - TLS_TCB_SIZE - adj);
 #elif TLS_DTV_AT_TP
-      pd = (struct pthread *) (((uintptr_t) attr->stackaddr
+      pd = (struct pthread *) (((uintptr_t) stackaddr
 				- __static_tls_size - adj)
 			       - TLS_PRE_TCB_SIZE);
 #endif
@@ -410,7 +417,7 @@  allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
       pd->specific[0] = pd->specific_1stblock;
 
       /* Remember the stack-related values.  */
-      pd->stackblock = (char *) attr->stackaddr - size;
+      pd->stackblock = (char *) stackaddr - size;
       pd->stackblock_size = size;
 
       /* This is a user-provided stack.  It will not be queued in the
@@ -634,7 +641,7 @@  allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 	  char *guard = mem + (((size - guardsize) / 2) & ~pagesize_m1);
 #elif _STACK_GROWS_DOWN
 	  char *guard = mem;
-# elif _STACK_GROWS_UP
+#elif _STACK_GROWS_UP
 	  char *guard = (char *) (((uintptr_t) pd - guardsize) & ~pagesize_m1);
 #endif
 	  if (mprotect (guard, guardsize, PROT_NONE) != 0)
@@ -734,7 +741,6 @@  allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 # endif
 #else
   *stack = pd->stackblock;
-  assert (*stack > 0);
 #endif
 
   return 0;
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index d10f4ea..b08ba45 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -428,12 +428,25 @@  START_THREAD_DEFN
 #ifdef _STACK_GROWS_DOWN
   char *sp = CURRENT_STACK_FRAME;
   size_t freesize = (sp - (char *) pd->stackblock) & ~pagesize_m1;
-#else
-# error "to do"
-#endif
   assert (freesize < pd->stackblock_size);
   if (freesize > PTHREAD_STACK_MIN)
     __madvise (pd->stackblock, freesize - PTHREAD_STACK_MIN, MADV_DONTNEED);
+#else
+  /* Page aligned start of memory to free (higher than or equal
+     to current sp plus the minimum stack size).  */
+  void *freeblock = (void*)((size_t)(CURRENT_STACK_FRAME
+				     + PTHREAD_STACK_MIN
+				     + pagesize_m1)
+				    & ~pagesize_m1);
+  char *free_end = (char *) (((uintptr_t) pd - pd->guardsize) & ~pagesize_m1);
+  /* Is there any space to free?  */
+  if (free_end > (char *)freeblock)
+    {
+      size_t freesize = (size_t)(free_end - (char *)freeblock);
+      assert (freesize < pd->stackblock_size);
+      __madvise (freeblock, freesize, MADV_DONTNEED);
+    }
+#endif
 
   /* If the thread is detached free the TCB.  */
   if (IS_DETACHED (pd))
diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
index 52a4602..9834302 100644
--- a/nptl/pthread_getattr_np.c
+++ b/nptl/pthread_getattr_np.c
@@ -60,7 +60,11 @@  pthread_getattr_np (thread_id, attr)
   if (__glibc_likely (thread->stackblock != NULL))
     {
       iattr->stacksize = thread->stackblock_size;
+#if _STACK_GROWS_DOWN
       iattr->stackaddr = (char *) thread->stackblock + iattr->stacksize;
+#else
+      iattr->stackaddr = (char *) thread->stackblock;
+#endif
     }
   else
     {
@@ -105,7 +109,9 @@  pthread_getattr_np (thread_id, attr)
 
 	      char *line = NULL;
 	      size_t linelen = 0;
+#if _STACK_GROWS_DOWN
 	      uintptr_t last_to = 0;
+#endif
 
 	      while (! feof_unlocked (fp))
 		{
@@ -129,17 +135,24 @@  pthread_getattr_np (thread_id, attr)
 		         stack extension request.  */
 		      iattr->stacksize = (iattr->stacksize
 					  & -(intptr_t) GLRO(dl_pagesize));
-
+#if _STACK_GROWS_DOWN
 		      /* The limit might be too high.  */
 		      if ((size_t) iattr->stacksize
 			  > (size_t) iattr->stackaddr - last_to)
 			iattr->stacksize = (size_t) iattr->stackaddr - last_to;
-
+#else
+		      /* The limit might be too high.  */
+		      if ((size_t) iattr->stacksize
+			  > to - (size_t) iattr->stackaddr)
+			iattr->stacksize = to - (size_t) iattr->stackaddr;
+#endif
 		      /* We succeed and no need to look further.  */
 		      ret = 0;
 		      break;
 		    }
+#if _STACK_GROWS_DOWN
 		  last_to = to;
+#endif
 		}
 
 	      free (line);