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

[Ian Lance Taylor <iant@golang.org>]

[-- 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);