Fwd: libgo patch committed: Use -fgo-c-header to share between Go and C

Fwd: libgo patch committed: Use -fgo-c-header to share between Go and C

[Ian Lance Taylor]

Resending without the attachment as it was blocked as spam.

The actual change be seen in Subversion or at
https://groups.google.com/forum/?pli=1#!topic/gofrontend-dev/fT2-PlvVZ9o
.

Ian


---------- Forwarded message ----------
From: Ian Lance Taylor <iant@golang.org>
Date: Tue, Aug 30, 2016 at 2:07 PM
Subject: libgo patch committed: Use -fgo-c-header to share between Go and C
To: gcc-patches <gcc-patches@gcc.gnu.org>,
"gofrontend-dev@googlegroups.com" <gofrontend-dev@googlegroups.com>


This patch to libgo uses the new -fgo-c-header option to share the
definitions of data structures and constants between Go and C.  Data
structures are defined in new files in libgo/go/runtime.  The
-fgo-c-header option is used to generate a C header file.  That header
file is #include'd by libgo/runtime/runtime.h and used when building
the code in libgo/runtime.  The C versions of the data structures are
removed.  This is a step toward converting much of the libgo runtime
to Go without breaking things as we go.

Putting the structs in Go means using the naming conventions of the Go
runtime package, so many of the constants pick up a leading underscore
and all other underscores are dropped.  The C code is adjusted
accordingly.

This also picks up a change made a while back to the gc runtime:
instead of using two different thread local variables, g and m, the m
variable is removed and the value is now always accessed as g->m.
This required a few additions to the C code to make sure that g->m is
always set correctly.

This change also passes the new -fgo-compiling-runtime option when
compiling the runtime package, although the option doesn't do anything
yet.

Bootstrapped and ran Go testsuite on x86_64-pc-linux-gnu and
sparc-sun-solaris11.  Committed to mainline.

Ian

Re: Fwd: libgo patch committed: Use -fgo-c-header to share between Go and C

[Andreas Schwab]

That breaks libgo on ia64.  The problem is that _ucontext_t isn't
properly aligned.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Fwd: libgo patch committed: Use -fgo-c-header to share between Go and C

[Ian Lance Taylor]

On Fri, Sep 2, 2016 at 9:14 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> That breaks libgo on ia64.  The problem is that _ucontext_t isn't
> properly aligned.

Interesting.  Thanks for looking into it.  What is the required
alignment?  This code should be aligning it to a pointer boundary.

Ian

Re: Fwd: libgo patch committed: Use -fgo-c-header to share between Go and C

[Andreas Schwab]

On Sep 02 2016, Ian Lance Taylor <iant@golang.org> wrote:

> On Fri, Sep 2, 2016 at 9:14 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>> That breaks libgo on ia64.  The problem is that _ucontext_t isn't
>> properly aligned.
>
> Interesting.  Thanks for looking into it.  What is the required
> alignment?  This code should be aligning it to a pointer boundary.

That is too small.  It needs at least 16 byte alignment.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Fwd: libgo patch committed: Use -fgo-c-header to share between Go and C

[Andreas Schwab]

On Sep 02 2016, Andreas Schwab <schwab@linux-m68k.org> wrote:

> On Sep 02 2016, Ian Lance Taylor <iant@golang.org> wrote:
>
>> On Fri, Sep 2, 2016 at 9:14 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>
>>> That breaks libgo on ia64.  The problem is that _ucontext_t isn't
>>> properly aligned.
>>
>> Interesting.  Thanks for looking into it.  What is the required
>> alignment?  This code should be aligning it to a pointer boundary.
>
> That is too small.  It needs at least 16 byte alignment.

And PowerPC has a similar requirement.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Fwd: libgo patch committed: Use -fgo-c-header to share between Go and C

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

On Fri, Sep 2, 2016 at 9:27 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Sep 02 2016, Ian Lance Taylor <iant@golang.org> wrote:
>
>> On Fri, Sep 2, 2016 at 9:14 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>
>>> That breaks libgo on ia64.  The problem is that _ucontext_t isn't
>>> properly aligned.
>>
>> Interesting.  Thanks for looking into it.  What is the required
>> alignment?  This code should be aligning it to a pointer boundary.
>
> That is too small.  It needs at least 16 byte alignment.

I have committed this patch to fix this problem (I hope).  Since the
relevant data structures are now defined in Go, and since they are
defined as simply an array of unsafe.Pointer in Go, and since Go does
not have a way to increase the alignment of a field in a struct, I
increased the size of the structures and used code to select the
appropriately aligned element of the structure.

Bootstrapped and ran Go testsuite on x86_64-pc-linux-gnu, which
admittedly does not show the problem.  Committed to mainline.

Ian

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 6620 bytes --]

Index: gcc/go/gofrontend/MERGE
===================================================================
--- gcc/go/gofrontend/MERGE	(revision 239894)
+++ gcc/go/gofrontend/MERGE	(working copy)
@@ -1,4 +1,4 @@
-c8cf90f2daf62428ca6aa0b5674572cd99f25fe3
+4f033f29553655ad90493d55059a7bbc6cd63108
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
Index: libgo/go/runtime/runtime2.go
===================================================================
--- libgo/go/runtime/runtime2.go	(revision 239872)
+++ libgo/go/runtime/runtime2.go	(working copy)
@@ -805,7 +805,11 @@ var (
 
 // _ucontext_t is a Go version of the C ucontext_t type, used by getcontext.
 // _sizeof_ucontext_t is defined by the Makefile from <ucontext.h>.
-type _ucontext_t [_sizeof_ucontext_t / unsafe.Sizeof(uintptr(0))]unsafe.Pointer
+// On some systems getcontext and friends require a value that is
+// aligned to a 16-byte boundary.  We implement this by increasing the
+// required size and picking an appropriate offset when we use the
+// array.
+type _ucontext_t [(_sizeof_ucontext_t + 15) / unsafe.Sizeof(unsafe.Pointer(nil))]unsafe.Pointer
 
 // traceback is used to collect stack traces from other goroutines.
 type traceback struct {
Index: libgo/runtime/proc.c
===================================================================
--- libgo/runtime/proc.c	(revision 239872)
+++ libgo/runtime/proc.c	(working copy)
@@ -156,6 +156,20 @@ fixcontext(ucontext_t *c)
 
 #endif
 
+// ucontext_arg returns a properly aligned ucontext_t value.  On some
+// systems a ucontext_t value must be aligned to a 16-byte boundary.
+// The g structure that has fields of type ucontext_t is defined in
+// Go, and Go has no simple way to align a field to such a boundary.
+// So we make the field larger in runtime2.go and pick an appropriate
+// offset within the field here.
+static ucontext_t*
+ucontext_arg(void** go_ucontext)
+{
+	uintptr_t p = (uintptr_t)go_ucontext;
+	p = (p + 15) &~ (uintptr_t)0xf;
+	return (ucontext_t*)p;
+}
+
 // We can not always refer to the TLS variables directly.  The
 // compiler will call tls_get_addr to get the address of the variable,
 // and it may hold it in a register across a call to schedule.  When
@@ -245,8 +259,8 @@ runtime_gogo(G* newg)
 #endif
 	g = newg;
 	newg->fromgogo = true;
-	fixcontext((ucontext_t*)&newg->context[0]);
-	setcontext((ucontext_t*)&newg->context[0]);
+	fixcontext(ucontext_arg(&newg->context[0]));
+	setcontext(ucontext_arg(&newg->context[0]));
 	runtime_throw("gogo setcontext returned");
 }
 
@@ -278,7 +292,7 @@ runtime_mcall(void (*pfn)(G*))
 		gp->gcnextsp = &pfn;
 #endif
 		gp->fromgogo = false;
-		getcontext((ucontext_t*)&gp->context[0]);
+		getcontext(ucontext_arg(&gp->context[0]));
 
 		// When we return from getcontext, we may be running
 		// in a new thread.  That means that g may have
@@ -305,8 +319,8 @@ runtime_mcall(void (*pfn)(G*))
 		// the getcontext call just above.
 		g = mp->g0;
 
-		fixcontext((ucontext_t*)&mp->g0->context[0]);
-		setcontext((ucontext_t*)&mp->g0->context[0]);
+		fixcontext(ucontext_arg(&mp->g0->context[0]));
+		setcontext(ucontext_arg(&mp->g0->context[0]));
 		runtime_throw("runtime: mcall function returned");
 	}
 }
@@ -709,7 +723,7 @@ runtime_tracebackothers(G * volatile me)
 #ifdef USING_SPLIT_STACK
 		__splitstack_getcontext(&me->stackcontext[0]);
 #endif
-		getcontext((ucontext_t*)&me->context[0]);
+		getcontext(ucontext_arg(&me->context[0]));
 
 		if(gp->traceback != nil) {
 		  runtime_gogo(gp);
@@ -750,7 +764,7 @@ runtime_tracebackothers(G * volatile me)
 #ifdef USING_SPLIT_STACK
 			__splitstack_getcontext(&me->stackcontext[0]);
 #endif
-			getcontext((ucontext_t*)&me->context[0]);
+			getcontext(ucontext_arg(&me->context[0]));
 
 			if(gp->traceback != nil) {
 				runtime_gogo(gp);
@@ -1063,7 +1077,7 @@ runtime_mstart(void* mp)
 	g->gcstacksize = 0;
 	g->gcnextsp = &mp;
 #endif
-	getcontext((ucontext_t*)&g->context[0]);
+	getcontext(ucontext_arg(&g->context[0]));
 
 	if(g->entry != nil) {
 		// Got here from mcall.
@@ -1251,7 +1265,7 @@ runtime_needm(void)
 	g->gcstacksize = 0;
 	g->gcnextsp = &mp;
 #endif
-	getcontext((ucontext_t*)&g->context[0]);
+	getcontext(ucontext_arg(&g->context[0]));
 
 	if(g->entry != nil) {
 		// Got here from mcall.
@@ -1282,6 +1296,7 @@ runtime_newextram(void)
 	G *gp;
 	byte *g0_sp, *sp;
 	uintptr g0_spsize, spsize;
+	ucontext_t *uc;
 
 	// Create extra goroutine locked to extra m.
 	// The goroutine is the context in which the cgo callback will run.
@@ -1302,10 +1317,11 @@ runtime_newextram(void)
 
 	// The context for gp will be set up in runtime_needm.  But
 	// here we need to set up the context for g0.
-	getcontext((ucontext_t*)&mp->g0->context[0]);
-	((ucontext_t*)&mp->g0->context[0])->uc_stack.ss_sp = g0_sp;
-	((ucontext_t*)&mp->g0->context[0])->uc_stack.ss_size = (size_t)g0_spsize;
-	makecontext((ucontext_t*)&mp->g0->context[0], kickoff, 0);
+	uc = ucontext_arg(&mp->g0->context[0]);
+	getcontext(uc);
+	uc->uc_stack.ss_sp = g0_sp;
+	uc->uc_stack.ss_size = (size_t)g0_spsize;
+	makecontext(uc, kickoff, 0);
 
 	// Add m to the extra list.
 	mnext = lockextra(true);
@@ -2007,7 +2023,7 @@ runtime_entersyscall()
 {
 	// Save the registers in the g structure so that any pointers
 	// held in registers will be seen by the garbage collector.
-	getcontext((ucontext_t*)&g->gcregs[0]);
+	getcontext(ucontext_arg(&g->gcregs[0]));
 
 	// Do the work in a separate function, so that this function
 	// doesn't save any registers on its own stack.  If this
@@ -2086,7 +2102,7 @@ runtime_entersyscallblock(void)
 
 	// Save the registers in the g structure so that any pointers
 	// held in registers will be seen by the garbage collector.
-	getcontext((ucontext_t*)&g->gcregs[0]);
+	getcontext(ucontext_arg(&g->gcregs[0]));
 
 	g->atomicstatus = _Gsyscall;
 
@@ -2375,14 +2391,16 @@ __go_go(void (*fn)(void*), void* arg)
 		byte * volatile vsp = sp;
 		size_t volatile vspsize = spsize;
 		G * volatile vnewg = newg;
+		ucontext_t * volatile uc;
 
-		getcontext((ucontext_t*)&vnewg->context[0]);
-		((ucontext_t*)&vnewg->context[0])->uc_stack.ss_sp = vsp;
+		uc = ucontext_arg(&vnewg->context[0]);
+		getcontext(uc);
+		uc->uc_stack.ss_sp = vsp;
 #ifdef MAKECONTEXT_STACK_TOP
-		((ucontext_t*)&vnewg->context[0])->uc_stack.ss_sp += vspsize;
+		uc->uc_stack.ss_sp += vspsize;
 #endif
-		((ucontext_t*)&vnewg->context[0])->uc_stack.ss_size = vspsize;
-		makecontext((ucontext_t*)&vnewg->context[0], kickoff, 0);
+		uc->uc_stack.ss_size = vspsize;
+		makecontext(uc, kickoff, 0);
 
 		runqput(p, vnewg);
 

Re: Fwd: libgo patch committed: Use -fgo-c-header to share between Go and C

[Andreas Schwab]

On Sep 09 2016, Ian Lance Taylor <iant@golang.org> wrote:

> Index: libgo/runtime/proc.c
> ===================================================================
> --- libgo/runtime/proc.c	(revision 239872)
> +++ libgo/runtime/proc.c	(working copy)
> @@ -156,6 +156,20 @@ fixcontext(ucontext_t *c)
>  
>  #endif
>  
> +// ucontext_arg returns a properly aligned ucontext_t value.  On some
> +// systems a ucontext_t value must be aligned to a 16-byte boundary.
> +// The g structure that has fields of type ucontext_t is defined in
> +// Go, and Go has no simple way to align a field to such a boundary.
> +// So we make the field larger in runtime2.go and pick an appropriate
> +// offset within the field here.
> +static ucontext_t*
> +ucontext_arg(void** go_ucontext)
> +{
> +	uintptr_t p = (uintptr_t)go_ucontext;
> +	p = (p + 15) &~ (uintptr_t)0xf;
> +	return (ucontext_t*)p;
> +}
> +

You should use alignof(ucontext_t) instead of hardcoding 16.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Fwd: libgo patch committed: Use -fgo-c-header to share between Go and C

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 259 bytes --]

On Fri, Sep 9, 2016 at 6:52 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> You should use alignof(ucontext_t) instead of hardcoding 16.

Fair enough.  Done like so.  Bootstrapped and ran Go testsuite on
x86_64-pc-linux-gnu, committed to mainline.

Ian

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1040 bytes --]

Index: gcc/go/gofrontend/MERGE
===================================================================
--- gcc/go/gofrontend/MERGE	(revision 240045)
+++ gcc/go/gofrontend/MERGE	(working copy)
@@ -1,4 +1,4 @@
-b37a9e66ea584885043240f8f6f1d1c0284eadec
+6c1f159cdcb56ebff617f6bbc6c97943a1a8a34d
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
Index: libgo/runtime/proc.c
===================================================================
--- libgo/runtime/proc.c	(revision 240045)
+++ libgo/runtime/proc.c	(working copy)
@@ -166,7 +166,13 @@ static ucontext_t*
 ucontext_arg(void** go_ucontext)
 {
 	uintptr_t p = (uintptr_t)go_ucontext;
-	p = (p + 15) &~ (uintptr_t)0xf;
+	size_t align = __alignof__(ucontext_t);
+	if(align > 16) {
+		// We only ensured space for up to a 16 byte alignment
+		// in libgo/go/runtime/runtime2.go.
+		runtime_throw("required alignment of ucontext_t too large");
+	}
+	p = (p + align - 1) &~ (uintptr_t)(align - 1);
 	return (ucontext_t*)p;
 }