NetBSD Problem Report #39465

From tho@openrobots.net  Fri Sep  5 20:29:20 2008
Return-Path: <tho@openrobots.net>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id 1751C63B853
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  5 Sep 2008 20:29:20 +0000 (UTC)
Message-Id: <20080905202910.E50825E3D6@ficus.home>
Date: Fri,  5 Sep 2008 22:29:10 +0200 (CEST)
From: anthony.mallet@useless-ficus.net
Reply-To: anthony.mallet@useless-ficus.net
To: gnats-bugs@gnats.NetBSD.org
Subject: threads stack is not aligned properly for gcc on i386
X-Send-Pr-Version: 3.95

>Number:         39465
>Category:       lib
>Synopsis:       threads stack is not aligned properly for gcc on i386
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Sep 05 20:30:00 +0000 2008
>Last-Modified:  Wed Nov 09 22:05:02 +0000 2011
>Originator:     anthony.mallet@useless-ficus.net
>Release:        NetBSD 4.99.72
>Organization:

>Environment:


System: NetBSD ficus 4.99.72 NetBSD 4.99.72 (FICUS) #8: Sat Aug 30 13:19:16 CEST 2008 troot@ficus:/usr/obj/sys/arch/i386/compile/FICUS i386
Architecture: i386
Machine: i386
>Description:
gcc expects the stack of functions to be setup so that the first argument of a
function is aligned on 16 bytes (on i386). It takes care of aligning main()'s
stack on a 16 bytes boundary and then generates code that relies on this
assumption.

The current code in makecontext(3) doesn't take care of this. This has the only
(but annoying) consequence that the __attribute__((aligned(xx))) declaration
does not work in functions called from a thread. In particular, SSE variables
on the stack cannot be used. 

The i386 ABI doesn't require that stacks are aligned on more than 4 bytes, so
strictly speaking the current makecontext(3) function is correct. Also, I
didn't check if code compiled with pcc works (I don't know if pcc supports
aligning variables on a particular boundary). However, it would be great if
code compiled with gcc would honor the aligned() attribute (perhaps only
within an #ifdef __GNUC__ ?)

Below is a fix that fixes the problem for me.
>How-To-Repeat:
Here is a small test program that exhibit the problem

#include <stdio.h>
#include <inttypes.h>
#include <stdbool.h>
#include <pthread.h>
#include <strings.h>

static void *
test16(void *data)
{
  char test __attribute__((aligned(16)));
  uintptr_t v = (uintptr_t)&test;
  size_t align;

  align = ffs((int)v);

  printf("aligned(16): %s: address: %p alignement: %d (2^%zu)\n",
	 align >= 5 ? "PASS" : "FAIL", &test, 1<<(align-1), (align-1));
  return NULL;
}

int
main()
{
  pthread_t tg;

  printf("Testing on main's stack:\n");
  test16(NULL);

  printf("Testing on thread's stack:\n");

  pthread_create(&tg, NULL, test16, NULL);
  pthread_join(tg, NULL);

  return 0;
}

>Fix:
Index: makecontext.c
===================================================================
RCS file: /cvsroot/src/lib/libc/arch/i386/gen/makecontext.çv
retrieving revision 1.4
diff -u -r1.4 makecontext.c
--- makecontext.c       28 Apr 2008 20:22:56 -0000      1.4
+++ makecontext.c       5 Sep 2008 20:11:07 -0000
@@ -70,7 +70,9 @@
        /* Align on word boundary. */
        /* LINTED uintptr_t is safe */
        sp  = (unsigned int *)((uintptr_t)sp & ~0x3);
-       sp -= argc + 1;                 /* Make room for ret and args. */
+       sp -= argc;                     /* Make room for ret and args. */
+       sp  = (unsigned int *)((uintptr_t)sp & ~0xf);
+       sp --;                  /* Make room for ret and args. */
        /* LINTED __greg_t is safe */
        gr[_REG_UESP] = (__greg_t)sp;
        gr[_REG_EBP] = (__greg_t)0;     /* Wipe out frame pointer. */

>Audit-Trail:
From: Matthew Mondor <mm_lists@pulsar-zone.net>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: lib/39465: threads stack is not aligned properly for gcc on
 i386
Date: Sun, 28 Aug 2011 03:39:57 -0400

 I ran the test and this still occurs on recent netbsd-5.  I cannot test
 it on -current, though.  The fix appears simple, harmless and might
 enhance performance.

 Perhaps that this would also fix warnings sent by ffmpeg about it
 possibly being slow because the stacks aren't aligned optimally...

 Thanks,
 -- 
 Matt

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: lib/39465: threads stack is not aligned properly for gcc on i386
Date: Sun, 28 Aug 2011 20:12:28 +0000

 On Sun, Aug 28, 2011 at 07:45:02AM +0000, Matthew Mondor wrote:
  >  I ran the test and this still occurs on recent netbsd-5.  I cannot test
  >  it on -current, though.  The fix appears simple, harmless and might
  >  enhance performance.

 The code in -current doesn't appear to be different.

  >  Perhaps that this would also fix warnings sent by ffmpeg about it
  >  possibly being slow because the stacks aren't aligned optimally...

 As I recall there's an issue about sse/mmx requiring this stack
 alignment and there was a lengthy discussion about it somewhere, but I
 forget the details.

 AIUI something like this patch should be committed. Although:

  : -       sp -= argc + 1;                 /* Make room for ret and args. */
  : +       sp -= argc;                     /* Make room for ret and args. */
  : +       sp  = (unsigned int *)((uintptr_t)sp & ~0xf);
  : +       sp --;                  /* Make room for ret and args. */

 Separating out an extra "sp--" seems bogus, and that should be

    (unsigned int *)((uintptr_t)sp & ~(uintptr_t)0xf);

 particularly if the code is going to get shared with or pasted into
 the amd64 version.

 -- 
 David A. Holland
 dholland@netbsd.org

From: Anthony Mallet <anthony.mallet@useless-ficus.net>
To: gnats-bugs@NetBSD.org
Cc: lib-bug-people@netbsd.org,
    gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org
Subject: Re: lib/39465: threads stack is not aligned properly for gcc on i386
Date: Mon, 29 Aug 2011 11:18:47 +0200

 On Sunday, at 20:15, David Holland wrote:
 |  AIUI something like this patch should be committed. Although:
 |  
 |   : -       sp -= argc + 1;                 /* Make room for ret and args. */
 |   : +       sp -= argc;                     /* Make room for ret and args. */
 |   : +       sp  = (unsigned int *)((uintptr_t)sp & ~0xf);
 |   : +       sp --;                  /* Make room for ret and args. */
 |  
 |  Separating out an extra "sp--" seems bogus

 I do not remember all the details, but IIRC separating sp-- was the way to get
 it right (i.e. have the test program pass). I agree that this seemed/seems
 bogus to me as well... Couldn't it be because gcc assumes that the first
 variable on the stack is aligned on 16 bytes, handling the ret address
 differently?

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: lib/39465: threads stack is not aligned properly for gcc on i386
Date: Sat, 5 Nov 2011 12:59:15 +0000

 On Mon, Aug 29, 2011 at 09:20:05AM +0000, Anthony Mallet wrote:
  >  |  Separating out an extra "sp--" seems bogus
  >  
  >  I do not remember all the details, but IIRC separating sp-- was
  > the way to get it right (i.e. have the test program pass). I agree
  > that this seemed/seems bogus to me as well... Couldn't it be
  > because gcc assumes that the first variable on the stack is aligned
  > on 16 bytes, handling the ret address differently?

 It could be any number of similar things... but it's important to
 figure out what, or it'll probably break if something shifts around a
 little.

 Also, AIUI it's an ABI issue that can't fully be solved this way, and
 there's resistance to applying a patch like this - not correct
 resistance IMO but it's there nonetheless, so it's unlikely to be
 accepted for -5.

 Meanwhile, AFAIK now that HEAD has gcc 4.5 it's no longer necessary
 there.

 -- 
 David A. Holland
 dholland@netbsd.org

From: Anthony Mallet <anthony.mallet@useless-ficus.net>
To: gnats-bugs@NetBSD.org
Cc: lib-bug-people@netbsd.org,
    gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org
Subject: Re: lib/39465: threads stack is not aligned properly for gcc on i386
Date: Sat, 5 Nov 2011 23:09:40 +0100

 On Saturday, at 13:00, David Holland wrote:
 |  Meanwhile, AFAIK now that HEAD has gcc 4.5 it's no longer necessary
 |  there.

 Unfortunately, I just ran the same test program compiled with `gcc -pthread` on
 NetBSD 5.99.56 (FICUS) #6: Sun Oct 30 00:14:18 CEST 2011
 troot@ficus:/usr/obj/sys/arch/i386/compile/FICUS i386

 And it still says:
 Testing on main's stack:
 aligned(16): PASS: address: 0xbfbfe780 alignement: 128 (2^7)
 Testing on thread's stack:
 aligned(16): FAIL: address: 0xbb7ffd9c alignement: 4 (2^2)

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: lib/39465: threads stack is not aligned properly for gcc on i386
Date: Sun, 6 Nov 2011 00:30:35 +0000

 On Sat, Nov 05, 2011 at 10:15:05PM +0000, Anthony Mallet wrote:
  >  On Saturday, at 13:00, David Holland wrote:
  >  |  Meanwhile, AFAIK now that HEAD has gcc 4.5 it's no longer necessary
  >  |  there.
  >  
  >  Unfortunately, I just ran the same test program compiled with `gcc -pthread` on
  >  NetBSD 5.99.56 (FICUS) #6: Sun Oct 30 00:14:18 CEST 2011
  >  troot@ficus:/usr/obj/sys/arch/i386/compile/FICUS i386
  >  
  >  And it still says:
  >  Testing on main's stack:
  >  aligned(16): PASS: address: 0xbfbfe780 alignement: 128 (2^7)
  >  Testing on thread's stack:
  >  aligned(16): FAIL: address: 0xbb7ffd9c alignement: 4 (2^2)

 Blah. All right, this clearly needs to be dealt with in full gory
 detail.

 -- 
 David A. Holland
 dholland@netbsd.org

From: Anthony Mallet <anthony.mallet@useless-ficus.net>
To: gnats-bugs@NetBSD.org
Cc: lib-bug-people@netbsd.org,
    gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org
Subject: Re: lib/39465: threads stack is not aligned properly for gcc on i386
Date: Wed, 9 Nov 2011 21:55:34 +0100

 On Saturday, at 13:00, David Holland wrote:
 |   >  I do not remember all the details, but IIRC separating sp-- was
 |   > the way to get it right (i.e. have the test program pass). I agree
 |   > that this seemed/seems bogus to me as well... Couldn't it be
 |   > because gcc assumes that the first variable on the stack is aligned
 |   > on 16 bytes, handling the ret address differently?
 |  
 |  It could be any number of similar things... but it's important to
 |  figure out what, or it'll probably break if something shifts around a
 |  little.

 Checking FreeBSD code, I found that they align the stack pointer _after_ the
 "sp -= argc + 1", probably to prepare the stack for the upcoming function
 calls, not for the very next one (that probably doesn't care, although it
 should probably be 4 bytes aligned anyway).

 This would be with current NetBSD code (i386) :
         sp  = (unsigned int *)((uintptr_t)ucp->uc_stack.ss_sp +
             ucp->uc_stack.ss_size);
         sp -= argc + 1;                 /* Make room for ret and args. */
         /* Align on 16 bytes boundary. */
         sp  = (unsigned int *)((uintptr_t)sp & ~0xf);

 Or, if you're strict:
         sp  = (unsigned int *)((uintptr_t)ucp->uc_stack.ss_sp +
             ucp->uc_stack.ss_size);
         /* Align on word boundary. Required?? */
         sp  = (unsigned int *)((uintptr_t)sp & ~0x3);
         sp -= argc + 1;                 /* Make room for ret and args. */
         /* Align on 16 bytes boundary. */
         sp  = (unsigned int *)((uintptr_t)sp & ~0xf);

 Does it make more sense like this? - apart from the fact that the ABI requires
 4 bytes alignment only, of course, but that's another issue -

 (I haven't tested).

From: Anthony Mallet <anthony.mallet@useless-ficus.net>
To: gnats-bugs@NetBSD.org
Cc: lib-bug-people@netbsd.org,
    gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org
Subject: Re: lib/39465: threads stack is not aligned properly for gcc on i386
Date: Wed, 9 Nov 2011 22:06:59 +0100

 On Wednesday, at 21:00, Anthony Mallet wrote:
 |  Does it make more sense like this? - apart from the fact that the ABI requires
 |  4 bytes alignment only, of course, but that's another issue -

 Also, this comment in the OpenBSD commit for the same problem is interesting:

 /*
  * Locate the initial frame at the top of the stack.  For the
  * stack to end up properly (16-byte) aligned, we need to
  * align the frame at an odd 8-byte boundary.
  */
 f = (struct frame *)((((int)base + len - sizeof *f) & ~15) - 8);

 http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libpthread/arch/i386/uthread_machdep.c.diff?r1=1.5;r2=1.6;f=h

>Unformatted:

Home	PR Database Search