Merge lp:~mwhudson/juju-core/gccgo-reflection-fun into lp:~go-bot/juju-core/trunk

[John A Meinel]

Change RPC methods to not return empty structs.
Empty structs cause gccgo to panic, so instead we use struct { _ bool } to ensure they are not empty.

This fixes the linked bug and makes attempting to return a structure fail. I expect this will confusingly break other parts of Juju...

[John A Meinel (jameinel)]

Resubmitting seems to have sorted Launchpad out. Now on to review.

[John A Meinel (jameinel)]

67 +type newlyAvailableMethods struct {
68 + _ bool
69 +}

I think for each of these we probably want to explain why we're doing it, referencing the related bug.

Something like:
67 +type newlyAvailableMethods struct {
68 + _ bool // gccgo Reflect breaks on empty structs: https://bugs.launchpad.net/juju-core/+bug/1251076
69 +}

Just poke me when you feel this is ready to land. I'm happy with just some doc tweaks.

review: Approve

[Michael Hudson-Doyle (mwhudson)]

John A Meinel <email address hidden> writes:

> Review: Approve
>
> 67 +type newlyAvailableMethods struct {
> 68 + _ bool
> 69 +}
>
> I think for each of these we probably want to explain why we're doing it, referencing the related bug.
>
> Something like:
> 67 +type newlyAvailableMethods struct {
> 68 + _ bool // gccgo Reflect breaks on empty structs: https://bugs.launchpad.net/juju-core/+bug/1251076
> 69 +}
>
> Just poke me when you feel this is ready to land. I'm happy with just some doc tweaks.

I think this is fine to land now.

That said, there are other gccgo fixes required to make gccgo and juju
play nice together and we're going to be providing a PPA with this and
those other fixes in it for at least saucy... so maybe this doesn't
actually need to land at all? If this happens, I'm sorry for wasting
your time!

[Go Bot (go-bot)]

Attempt to merge into lp:juju-core failed due to conflicts:

text conflict in state/apiserver/root.go

[Go Bot (go-bot)]

Attempt to merge into lp:juju-core failed due to conflicts:

text conflict in state/apiserver/root.go

[John A Meinel (jameinel)]

I'm pretty sure we decided we don't need this after all, so marking it rejected.

[Michael Hudson-Doyle (mwhudson)]

John A Meinel <email address hidden> writes:

> I'm pretty sure we decided we don't need this after all, so marking it rejected.

Yes indeed. Sorry for not withdrawing this when that became clear.

Unmerged revisions

2076. By Michael Hudson-Doyle on 2013-11-19

another workaround field

2075. By Michael Hudson-Doyle on 2013-11-19

add workaround fields

2074. By Michael Hudson-Doyle on 2013-11-19

discard methods taking or returning empty structures

=== modified file 'rpc/reflect_test.go'
--- rpc/reflect_test.go	2013-09-23 10:06:47 +0000
+++ rpc/reflect_test.go	2013-11-19 07:42:22 +0000
@@ -28,6 +28,7 @@
 		"Discard1",
 		"Discard2",
 		"Discard3",
+		"Discard4",
 	})
 	expect := map[string]reflect.Type{
 		"CallbackMethods":  reflect.TypeOf(&CallbackMethods{}),
@@ -58,6 +59,8 @@
 		"Discard2",
 		"Discard3",
 		"Discard4",
+		"Discard5",
+		"Discard6",
 	})
 	expect := map[string]*rpcreflect.ObjMethod{
 		"SliceArg": {
 
=== modified file 'rpc/rpc_test.go'
--- rpc/rpc_test.go	2013-10-25 18:21:30 +0000
+++ rpc/rpc_test.go	2013-11-19 07:42:22 +0000
@@ -95,6 +95,8 @@
 
 func (r *Root) Discard3(id string) int { return 0 }
 
+func (r *Root) Discard4(id string) (_ struct {}, _ error) { return }
+
 func (r *Root) CallbackMethods(string) (*CallbackMethods, error) {
 	return &CallbackMethods{r}, nil
 }
@@ -183,11 +185,16 @@
 
 func (a *SimpleMethods) Discard1(int) {}
 
-func (a *SimpleMethods) Discard2(struct{}, struct{}) {}
+func (a *SimpleMethods) Discard2(struct{ _ bool }, struct{ _ bool }) {}
 
 func (a *SimpleMethods) Discard3() int { return 0 }
 
-func (a *SimpleMethods) Discard4() (_, _ struct{}) { return }
+func (a *SimpleMethods) Discard4() (_, _ struct{ _ bool }) { return }
+
+func (a *SimpleMethods) Discard5(struct {})  { return }
+
+func (a *SimpleMethods) Discard6() (_ struct {}) { return }
+
 
 type DelayedMethods struct {
 	ready     chan struct{}
@@ -243,13 +250,17 @@
 	a.r.conn.Serve(nil, nil)
 }
 
-type changedAPIRoot struct{}
+type changedAPIRoot struct{
+	_ bool
+}
 
 func (r *changedAPIRoot) NewlyAvailable(string) (newlyAvailableMethods, error) {
 	return newlyAvailableMethods{}, nil
 }
 
-type newlyAvailableMethods struct{}
+type newlyAvailableMethods struct {
+	_ bool
+}
 
 func (newlyAvailableMethods) NewMethod() stringVal {
 	return stringVal{"new method result"}
 
=== modified file 'rpc/rpcreflect/type.go'
--- rpc/rpcreflect/type.go	2013-09-24 07:12:05 +0000
+++ rpc/rpcreflect/type.go	2013-11-19 07:42:22 +0000
@@ -130,6 +130,13 @@
 	return m.Name == "Kill" && m.Type.NumIn() == 1 && m.Type.NumOut() == 0
 }
 
+func isEmptyStruct(t reflect.Type) bool {
+	// gccgo has a bug calling methods that take or return empty
+	// structs via reflection.  See
+	// https://code.google.com/p/go/issues/detail?id=6761.
+	return t.Kind() == reflect.Struct && t.NumField() == 0
+}
+
 func newRootMethod(m reflect.Method) *RootMethod {
 	if m.PkgPath != "" {
 		return nil
@@ -141,6 +148,9 @@
 		t.Out(1) != errorType {
 		return nil
 	}
+	if isEmptyStruct(t.Out(0)) {
+		return nil
+	}
 	f := func(rcvr reflect.Value, id string) (r reflect.Value, err error) {
 		out := rcvr.Method(m.Index).Call([]reflect.Value{reflect.ValueOf(id)})
 		if !out[1].IsNil() {
@@ -325,10 +335,10 @@
 		return nil
 	}
 	// The parameters and return value must be of struct type.
-	if p.Params != nil && p.Params.Kind() != reflect.Struct {
+	if p.Params != nil && (p.Params.Kind() != reflect.Struct || isEmptyStruct(p.Params)) {
 		return nil
 	}
-	if p.Result != nil && p.Result.Kind() != reflect.Struct {
+	if p.Result != nil && (p.Result.Kind() != reflect.Struct || isEmptyStruct(p.Result)) {
 		return nil
 	}
 	return &p
 
=== modified file 'state/apiserver/root.go'
--- state/apiserver/root.go	2013-10-11 09:56:04 +0000
+++ state/apiserver/root.go	2013-11-19 07:42:22 +0000
@@ -233,7 +233,9 @@
 	return srvPinger{}, nil
 }
 
-type srvPinger struct{}
+type srvPinger struct {
+	_ bool // see https://code.google.com/p/go/issues/detail?id=6761
+}
 
 // Ping is a no-op used by client heartbeat monitor.
 func (r srvPinger) Ping() {}