Patchwork GCC's -fsplit-stack disturbing Mach's vm_allocate

login
register
mail settings
Submitter Thomas Schwinge
Date June 26, 2013, 9:30 p.m.
Message ID <87ip10o90k.fsf@kepler.schwinge.homeip.net>
Download mbox | patch
Permalink /patch/254873/
State New
Headers show

Comments

Thomas Schwinge - June 26, 2013, 9:30 p.m.
Hi!

On Sat, 22 Jun 2013 08:15:46 -0700, Ian Lance Taylor <iant@google.com> wrote:
> Go can work without split stack.  In that case libgo will use much
> larger stacks for goroutines, to reduce the chance of running out of
> stack space (see StackMin in libgo/runtime/proc.c).  So the number of
> simultaneous goroutines that can be run will be limited.  This is
> usually OK on x86_64 but it does hamper Go programs running on 32-bit
> x86.

OK, but that's not the most pressing issue we're having right now.
Anyway, as it stands, the split-stack code doesn't work on Hurd, so I
disabled it in r200434 as follows:

gcc/
	* config/i386/gnu.h [TARGET_LIBC_PROVIDES_SSP]
	(TARGET_CAN_SPLIT_STACK, TARGET_THREAD_SPLIT_STACK_OFFSET):
	Undefine.



Grüße,
 Thomas
Samuel Thibault - April 4, 2014, 7:14 p.m.
Hello,

Thomas Schwinge, le Wed 26 Jun 2013 23:30:03 +0200, a écrit :
> On Sat, 22 Jun 2013 08:15:46 -0700, Ian Lance Taylor <iant@google.com> wrote:
> > Go can work without split stack.  In that case libgo will use much
> > larger stacks for goroutines, to reduce the chance of running out of
> > stack space (see StackMin in libgo/runtime/proc.c).  So the number of
> > simultaneous goroutines that can be run will be limited.  This is
> > usually OK on x86_64 but it does hamper Go programs running on 32-bit
> > x86.
> 
> OK, but that's not the most pressing issue we're having right now.
> Anyway, as it stands, the split-stack code doesn't work on Hurd, so I
> disabled it in r200434 as follows:

Maybe you'd want to re-enable it, now that we have got rid of threadvars :)

Samuel
Svante Signell - April 9, 2014, 7:05 a.m.
On Fri, 2014-04-04 at 21:14 +0200, Samuel Thibault wrote:
> Hello,
> 
> Thomas Schwinge, le Wed 26 Jun 2013 23:30:03 +0200, a écrit :
> > On Sat, 22 Jun 2013 08:15:46 -0700, Ian Lance Taylor <iant@google.com> wrote:
> > > Go can work without split stack.  In that case libgo will use much
> > > larger stacks for goroutines, to reduce the chance of running out of
> > > stack space (see StackMin in libgo/runtime/proc.c).  So the number of
> > > simultaneous goroutines that can be run will be limited.  This is
> > > usually OK on x86_64 but it does hamper Go programs running on 32-bit
> > > x86.
> > 
> > OK, but that's not the most pressing issue we're having right now.
> > Anyway, as it stands, the split-stack code doesn't work on Hurd, so I
> > disabled it in r200434 as follows:
> 
> Maybe you'd want to re-enable it, now that we have got rid of threadvars :)

I don't think it is a good idea. I've patched gcc-4.9-20140406 to make
gccgo build and tested with -fsplit-stack enabled (with and without the
gold linker). Without split stack around 70 libgo tests pass and 50
fails. With it enabled all tests fail. Simple examples are the following
C code (from Thomas) and GO code:

1)
cat test_split_stack.c 
#include <mach.h>
#include <stdio.h>

int main(void)
{
  int err;
  vm_address_t addr = 0;

  int i;
  for (i = 0; i < 3; ++i)
    {
      err = vm_allocate(mach_task_self(), &addr, 4096, 1);
      printf("%u %p\n", err, (void *) addr);
    }
  return 0;
}
    $ gcc-4.9 test_split_stack.c -fsplit-stack
    $ ./a.out
    0 (nil)
    0 0x102c000
    0 0x1027800

    $ gcc-4.9 test_split_stack.c
    $ ./a.out
    0 0x102c000
    0 0x125b000
    0 0x125c000

2)
cat hello.go:
package main

import "fmt"

func main() {
        fmt.Printf("Hello, world.\n")
}

gccgo-4.9 -g hello.go
 ./a.out
Hello, world.

LD_PRELOAD=../gcc-4.9-4.9-20140406/build/i486-gnu/libgo/.libs/libgo.so.5.0.0 ./a.out
mmap errno 1073741846
fatal error: mmap

runtime stack:
^C

Something is still not OK with the treads library?

Patches for gccgo on GNU/Hurd will be submitted to the Debian BTS.
Thomas Schwinge - April 9, 2014, 7:36 a.m.
Hi!

On Wed, 9 Apr 2014 09:05:46 +0200, Svante Signell <svante.signell@gmail.com> wrote:
> On Fri, 2014-04-04 at 21:14 +0200, Samuel Thibault wrote:
> > Thomas Schwinge, le Wed 26 Jun 2013 23:30:03 +0200, a écrit :
> > > On Sat, 22 Jun 2013 08:15:46 -0700, Ian Lance Taylor <iant@google.com> wrote:
> > > > Go can work without split stack.  In that case libgo will use much
> > > > larger stacks for goroutines, to reduce the chance of running out of
> > > > stack space (see StackMin in libgo/runtime/proc.c).  So the number of
> > > > simultaneous goroutines that can be run will be limited.  This is
> > > > usually OK on x86_64 but it does hamper Go programs running on 32-bit
> > > > x86.
> > > 
> > > OK, but that's not the most pressing issue we're having right now.
> > > Anyway, as it stands, the split-stack code doesn't work on Hurd, so I
> > > disabled it in r200434 as follows:
> > 
> > Maybe you'd want to re-enable it, now that we have got rid of threadvars :)
> 
> I don't think it is a good idea. I've patched gcc-4.9-20140406 to make
> gccgo build and tested with -fsplit-stack enabled (with and without the
> gold linker). Without split stack around 70 libgo tests pass and 50
> fails. With it enabled all tests fail. [...]

> LD_PRELOAD=../gcc-4.9-4.9-20140406/build/i486-gnu/libgo/.libs/libgo.so.5.0.0 ./a.out
> mmap errno 1073741846
> fatal error: mmap

Well, the first step is to verify that TARGET_THREAD_SPLIT_STACK_OFFSET
and similar configury is correct for the Hurd, and figure out what's
going on with the zero-page unmapping (discussed earlier in this thread),
and then mmap failing with 1073741846 (EINVAL).


> Patches for gccgo on GNU/Hurd will be submitted to the Debian BTS.

Just a suggestion, but in my opinion, it'd make more sense to first get
such patches integrated upstream.  (Same also for the GCC Ada patches.)
GCC Go support (as well as Ada) clearly is not a critical thing to first
get into Debian GNU/Hurd, and the total maintenance/integration overhead
would be lower if these patches would just percolate into Debian GCC from
upstream.


Grüße,
 Thomas
Samuel Thibault - April 11, 2014, 9:51 p.m.
Thomas Schwinge, le Wed 09 Apr 2014 09:36:42 +0200, a écrit :
> Well, the first step is to verify that TARGET_THREAD_SPLIT_STACK_OFFSET
> and similar configury is correct for the Hurd,

It's not.  I've checked what TARGET_THREAD_SPLIT_STACK_OFFSET is, it's
an offset inside the tcbhead_t structure, and we don't have that field
at all...  We may want to extend our tcbhead_t the same way as Linux
i386.

> and figure out what's
> going on with the zero-page unmapping (discussed earlier in this thread),
> and then mmap failing with 1073741846 (EINVAL).

Here is the backtrace

#0  __vm_map (target_task=1, address=address@entry=0x1029cc4, size=size@entry=0, mask=mask@entry=0, anywhere=1, memory_object=memory_object@entry=0, offset=offset@entry=0, 
    copy=copy@entry=1, cur_protection=cur_protection@entry=1, max_protection=max_protection@entry=7, inheritance=inheritance@entry=1)
    at /usr/src/eglibc-2.18/build-tree/hurd-i386-libc/mach/vm_map.c:56
#1  0x0118a3e8 in __mmap (addr=0x0, len=0, prot=4, flags=2, fd=-1, offset=0) at ../sysdeps/mach/hurd/mmap.c:145
#2  0x0804960d in __morestack_load_mmap ()
#3  0x08049d12 in __libc_csu_init ()
#4  0x010b4671 in __libc_start_main (main=0x80489dd <main>, argc=1, argv=0x1029de4, init=0x8049cc0 <__libc_csu_init>, fini=0x8049d30 <__libc_csu_fini>, rtld_fini=0xfb90 <_dl_fini>, 
    stack_end=0x1029ddc) at libc-start.c:246
#5  0x08048901 in _start ()

It's indeed:

/* This function is called at program startup time to make sure that
   mmap, munmap, and getpagesize are resolved if linking dynamically.
   We want to resolve them while we have enough stack for them, rather
   than calling into the dynamic linker while low on stack space.  */

void
__morestack_load_mmap (void)
{
  /* Call with bogus values to run faster.  We don't care if the call
     fails.  Pass __MORESTACK_CURRENT_SEGMENT to make sure that any
     TLS accessor function is resolved.  */
  mmap (__morestack_current_segment, 0, PROT_READ, MAP_ANONYMOUS, -1, 0);
  mprotect (NULL, 0, 0);
  munmap (0, getpagesize ());
}

Yes...

So, do we really want to let munmap poke a hole at address 0 and thus
let further vm_map() return address 0?

Samuel
Richard Braun - April 15, 2014, 6:33 a.m.
On Fri, Apr 11, 2014 at 11:51:44PM +0200, Samuel Thibault wrote:
> It's indeed:
> 
> /* This function is called at program startup time to make sure that
>    mmap, munmap, and getpagesize are resolved if linking dynamically.
>    We want to resolve them while we have enough stack for them, rather
>    than calling into the dynamic linker while low on stack space.  */
> 
> void
> __morestack_load_mmap (void)
> {
>   /* Call with bogus values to run faster.  We don't care if the call
>      fails.  Pass __MORESTACK_CURRENT_SEGMENT to make sure that any
>      TLS accessor function is resolved.  */
>   mmap (__morestack_current_segment, 0, PROT_READ, MAP_ANONYMOUS, -1, 0);
>   mprotect (NULL, 0, 0);
>   munmap (0, getpagesize ());
> }
> 
> Yes...
> 
> So, do we really want to let munmap poke a hole at address 0 and thus
> let further vm_map() return address 0?

We probably don't. AIUI, the first page is always mapped, and always
with PROT_NONE to make sure null pointers are catched. Considering other
systems have predefined ranges depending on the mapping type, instead
of blindly starting at the beginning of the map like vm_map() does, it's
perfectly valid to unmap the first page, which is normally the right
way to catch null pointers. So, since we do want to catch null pointers,
we do want to keep that first page, but only the first page. Or rather,
a range large enough to catch accesses through null pointers, e.g. it
could even be 64 or 128 KiB. We could alter glibc so that the first
mapping has this special size, and have munmap override its given range
to skip that area.
Samuel Thibault - April 16, 2014, 10:03 p.m.
Thomas Schwinge, le Wed 09 Apr 2014 09:36:42 +0200, a écrit :
> Well, the first step is to verify that TARGET_THREAD_SPLIT_STACK_OFFSET
> and similar configury is correct for the Hurd,

I have added the corresponding field, so we can just use the same offset
as on Linux.

Samuel
Samuel Thibault - April 18, 2014, 8:03 a.m.
Samuel Thibault, le Thu 17 Apr 2014 00:03:45 +0200, a écrit :
> Thomas Schwinge, le Wed 09 Apr 2014 09:36:42 +0200, a écrit :
> > Well, the first step is to verify that TARGET_THREAD_SPLIT_STACK_OFFSET
> > and similar configury is correct for the Hurd,
> 
> I have added the corresponding field, so we can just use the same offset
> as on Linux.

I have uploaded packages on http://people.debian.org/~sthibault/tmp/ so
Svante can try setting TARGET_THREAD_SPLIT_STACK_OFFSET to 0x30 with
them.

Samuel

Patch

diff --git gcc/config/i386/gnu.h gcc/config/i386/gnu.h
index 35063e6..4a91c84 100644
--- gcc/config/i386/gnu.h
+++ gcc/config/i386/gnu.h
@@ -36,6 +36,12 @@  along with GCC.  If not, see <http://www.gnu.org/licenses/>.
 #endif
 
 #ifdef TARGET_LIBC_PROVIDES_SSP
+
+/* Not supported yet.  */
+# undef TARGET_THREAD_SSP_OFFSET
+
 /* Not supported yet.  */
-#undef TARGET_THREAD_SSP_OFFSET
+# undef TARGET_CAN_SPLIT_STACK
+# undef TARGET_THREAD_SPLIT_STACK_OFFSET
+
 #endif