[PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
Add support for reflection for gccgo in gcc 4.9. This is not a backport because reflection support in gcc trunk is done using FFI. Bootstrap built and tested on ppc64, ppc64le. 2015-01-06Lynn Boger * libgo/Makefile.am: Build the new files for libgo reflection support on ppc64, ppc64le * libgo/Makefile.in: same * libgo/go/reflect/makefunc.go: Invoke reflection functions for ppc64, ppc64le * libgo/go/reflect/makefunc_ppc.c: makeFuncStub for ppc64, ppc64le * libgo/go/reflect/makefuncgo_ppc.go: MakeFuncStubGo and argument handling for ppc64, ppc64le Index: libgo/Makefile.am === --- libgo/Makefile.am (revision 219198) +++ libgo/Makefile.am (working copy) @@ -925,11 +925,25 @@ go_reflect_makefunc_file = \ go_reflect_makefunc_s_file = \ go/reflect/makefunc_386.S else +if LIBGO_IS_PPC64 +go_reflect_makefunc_file = \ + go/reflect/makefuncgo_ppc.go +go_reflect_makefunc_s_file = \ + go/reflect/makefunc_ppc.c +else +if LIBGO_IS_PPC64LE +go_reflect_makefunc_file = \ + go/reflect/makefuncgo_ppc.go +go_reflect_makefunc_s_file = \ + go/reflect/makefunc_ppc.c +else go_reflect_makefunc_file = go_reflect_makefunc_s_file = \ go/reflect/makefunc_dummy.c endif endif +endif +endif go_reflect_files = \ go/reflect/deepequal.go \ Index: libgo/Makefile.in === --- libgo/Makefile.in (revision 219198) +++ libgo/Makefile.in (working copy) @@ -1097,7 +1097,13 @@ go_path_files = \ go/path/match.go \ go/path/path.go -@LIBGO_IS_386_FALSE@@LIBGO_IS_X86_64_FALSE@go_reflect_makefunc_file = +@LIBGO_IS_386_FALSE@@LIBGO_IS_PPC64LE_FALSE@@LIBGO_IS_PPC64_FALSE@@LIBGO_IS_X86_64_FALSE@go_reflect_makefunc_file = +@LIBGO_IS_386_FALSE@@LIBGO_IS_PPC64LE_TRUE@@LIBGO_IS_PPC64_FALSE@@LIBGO_IS_X86_64_FALSE@go_reflect_makefunc_file = \ +@LIBGO_IS_386_FALSE@@LIBGO_IS_PPC64LE_TRUE@@LIBGO_IS_PPC64_FALSE@@LIBGO_IS_X86_64_FALSE@ go/reflect/makefuncgo_ppc.go + +@LIBGO_IS_386_FALSE@@LIBGO_IS_PPC64_TRUE@@LIBGO_IS_X86_64_FALSE@go_reflect_makefunc_file = \ +@LIBGO_IS_386_FALSE@@LIBGO_IS_PPC64_TRUE@@LIBGO_IS_X86_64_FALSE@ go/reflect/makefuncgo_ppc.go + @LIBGO_IS_386_TRUE@@LIBGO_IS_X86_64_FALSE@go_reflect_makefunc_file = \ @LIBGO_IS_386_TRUE@@LIBGO_IS_X86_64_FALSE@ go/reflect/makefuncgo_386.go @@ -1104,9 +1110,15 @@ go_path_files = \ @LIBGO_IS_X86_64_TRUE@go_reflect_makefunc_file = \ @LIBGO_IS_X86_64_TRUE@ go/reflect/makefuncgo_amd64.go -@LIBGO_IS_386_FALSE@@LIBGO_IS_X86_64_FALSE@go_reflect_makefunc_s_file = \ -@LIBGO_IS_386_FALSE@@LIBGO_IS_X86_64_FALSE@ go/reflect/makefunc_dummy.c +@LIBGO_IS_386_FALSE@@LIBGO_IS_PPC64LE_FALSE@@LIBGO_IS_PPC64_FALSE@@LIBGO_IS_X86_64_FALSE@go_reflect_makefunc_s_file = \ +@LIBGO_IS_386_FALSE@@LIBGO_IS_PPC64LE_FALSE@@LIBGO_IS_PPC64_FALSE@@LIBGO_IS_X86_64_FALSE@ go/reflect/makefunc_dummy.c +@LIBGO_IS_386_FALSE@@LIBGO_IS_PPC64LE_TRUE@@LIBGO_IS_PPC64_FALSE@@LIBGO_IS_X86_64_FALSE@go_reflect_makefunc_s_file = \ +@LIBGO_IS_386_FALSE@@LIBGO_IS_PPC64LE_TRUE@@LIBGO_IS_PPC64_FALSE@@LIBGO_IS_X86_64_FALSE@ go/reflect/makefunc_ppc.c + +@LIBGO_IS_386_FALSE@@LIBGO_IS_PPC64_TRUE@@LIBGO_IS_X86_64_FALSE@go_reflect_makefunc_s_file = \ +@LIBGO_IS_386_FALSE@@LIBGO_IS_PPC64_TRUE@@LIBGO_IS_X86_64_FALSE@ go/reflect/makefunc_ppc.c + @LIBGO_IS_386_TRUE@@LIBGO_IS_X86_64_FALSE@go_reflect_makefunc_s_file = \ @LIBGO_IS_386_TRUE@@LIBGO_IS_X86_64_FALSE@ go/reflect/makefunc_386.S Index: libgo/go/reflect/makefunc.go === --- libgo/go/reflect/makefunc.go (revision 219198) +++ libgo/go/reflect/makefunc.go (working copy) @@ -52,7 +52,7 @@ func MakeFunc(typ Type, fn func(args []Value) (res } switch runtime.GOARCH { - case "amd64", "386": + case "amd64", "386", "ppc64", "ppc64le": default: panic("reflect.MakeFunc not implemented for " + runtime.GOARCH) } @@ -91,7 +91,7 @@ func makeMethodValue(op string, v Value) Value { } switch runtime.GOARCH { - case "amd64", "386": + case "amd64", "386", "ppc64", "ppc64le": default: panic("reflect.makeMethodValue not implemented for " + runtime.GOARCH) } @@ -138,7 +138,7 @@ func makeValueMethod(v Value) Value { } switch runtime.GOARCH { - case "amd64", "386": + case "amd64", "386", "ppc64", "ppc64le": default: panic("reflect.makeValueMethod not implemented for " + runtime.GOARCH) } Index: libgo/go/reflect/makefunc_ppc.c === --- libgo/go/reflect/makefunc_ppc.c (revision 0) +++ libgo/go/reflect/makefunc_ppc.c (working copy) @@ -0,0 +1,102 @@ +// Copyright 2015 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +#include "runtime.h" +#include "go-panic.h" + +extern void MakeFuncStubGo(void *, void *) asm ("reflect.MakeFuncStubGo"); + +/* Structure to store all registers used for paramete
Re: [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
Sorry, hold off on this one for now. I missed the update to libgo/go/reflect/all_test.go for ppc64 and ppc64le in this patch and when changed to actually test these goarch values it is failing. On 01/06/2015 09:37 AM, Lynn A. Boger wrote: Add support for reflection for gccgo in gcc 4.9. This is not a backport because reflection support in gcc trunk is done using FFI. Bootstrap built and tested on ppc64, ppc64le. 2015-01-06Lynn Boger * libgo/Makefile.am: Build the new files for libgo reflection support on ppc64, ppc64le * libgo/Makefile.in: same * libgo/go/reflect/makefunc.go: Invoke reflection functions for ppc64, ppc64le * libgo/go/reflect/makefunc_ppc.c: makeFuncStub for ppc64, ppc64le * libgo/go/reflect/makefuncgo_ppc.go: MakeFuncStubGo and argument handling for ppc64, ppc64le
Re: [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
Would it be possible to add this patch to the gcc 4.9 branch as is? It provides most of the basic support for reflection on ppc64 and ppc64le, which does not work at all currently. Once I get the libgo/go/reflect/all_test.go working I can add the patch for the fixes and enable the testcase for these goarch values in a later submission. Thanks. On 01/06/2015 12:38 PM, Lynn A. Boger wrote: Sorry, hold off on this one for now. I missed the update to libgo/go/reflect/all_test.go for ppc64 and ppc64le in this patch and when changed to actually test these goarch values it is failing. On 01/06/2015 09:37 AM, Lynn A. Boger wrote: Add support for reflection for gccgo in gcc 4.9. This is not a backport because reflection support in gcc trunk is done using FFI. Bootstrap built and tested on ppc64, ppc64le. 2015-01-06Lynn Boger * libgo/Makefile.am: Build the new files for libgo reflection support on ppc64, ppc64le * libgo/Makefile.in: same * libgo/go/reflect/makefunc.go: Invoke reflection functions for ppc64, ppc64le * libgo/go/reflect/makefunc_ppc.c: makeFuncStub for ppc64, ppc64le * libgo/go/reflect/makefuncgo_ppc.go: MakeFuncStubGo and argument handling for ppc64, ppc64le
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
On Tue, Jan 6, 2015 at 7:37 AM, Lynn A. Boger wrote: > Add support for reflection for gccgo in gcc 4.9. This is not a backport > because reflection support in gcc trunk is done using FFI. Bootstrap built > and tested on ppc64, ppc64le. Sorry, I didn't see at first that this is not a backport. This is a complex patch that has never been tested on trunk. We don't this kind of change on release branches. For this kind of work you'll need to maintain your own branch. Sorry. Ian
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
I thought that since this only affects ppc64 and ppc64le, and provides reflection support that doesnt exist in gcc 4.9 for Power (you get a panic if you try to use reflection on ppc64, ppc64le), it would be OK. We'll look at putting it in our own branch. On 01/07/2015 10:06 AM, Ian Lance Taylor wrote: On Tue, Jan 6, 2015 at 7:37 AM, Lynn A. Boger wrote: Add support for reflection for gccgo in gcc 4.9. This is not a backport because reflection support in gcc trunk is done using FFI. Bootstrap built and tested on ppc64, ppc64le. Sorry, I didn't see at first that this is not a backport. This is a complex patch that has never been tested on trunk. We don't this kind of change on release branches. For this kind of work you'll need to maintain your own branch. Sorry. Ian
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
On Wed, Jan 7, 2015 at 8:43 AM, Lynn A. Boger wrote: > > I thought that since this only affects ppc64 and ppc64le, and provides > reflection support that doesnt exist in gcc 4.9 for Power (you get a panic > if you try to use reflection on ppc64, ppc64le), it would be OK. I understand that the code doesn't work today in 4.9, but a release branch is for releases and backports of bug fixes, not for any sort of new development. (As far as I knoew reflection works in general for PPC in 4.9, it's just reflect.MakeFunc, a rarely used feature, that is broken.) Ian
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
In libgo/go/reflect/makefunc.go, calls to MakeFunc, makeMethodValue and makeValueMethod will panic if called when GOARCH is ppc64 or ppc64le. I understand your point about what you allow into a release branch and since it has known bugs with an existing testcase then I agree it shouldn't be accepted the way it is. I will work on fixing that and then figure out what branch is best to include it in. On 01/07/2015 10:54 AM, Ian Lance Taylor wrote: On Wed, Jan 7, 2015 at 8:43 AM, Lynn A. Boger wrote: I thought that since this only affects ppc64 and ppc64le, and provides reflection support that doesnt exist in gcc 4.9 for Power (you get a panic if you try to use reflection on ppc64, ppc64le), it would be OK. I understand that the code doesn't work today in 4.9, but a release branch is for releases and backports of bug fixes, not for any sort of new development. (As far as I knoew reflection works in general for PPC in 4.9, it's just reflect.MakeFunc, a rarely used feature, that is broken.) Ian
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
On Wed, Jan 7, 2015 at 9:26 AM, Lynn A. Boger wrote: > > In libgo/go/reflect/makefunc.go, calls to MakeFunc, makeMethodValue and > makeValueMethod will panic if called when GOARCH is ppc64 or ppc64le. Right, I'm just saying that almost no code actually does that. I just tried a web search and found no uses other than examples of how to use it. I'm sure there are a few, but not many. Ian
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
Ian Lance Taylor writes: > On Wed, Jan 7, 2015 at 9:26 AM, Lynn A. Boger > wrote: >> >> In libgo/go/reflect/makefunc.go, calls to MakeFunc, makeMethodValue and >> makeValueMethod will panic if called when GOARCH is ppc64 or ppc64le. > > Right, I'm just saying that almost no code actually does that. I just > tried a web search and found no uses other than examples of how to use > it. I'm sure there are a few, but not many. There is a somewhat hidden one in Docker: https://github.com/docker/docker/blob/master/api/client/cli.go#L64 (it is also possible to patch docker to do this differently, of course). Cheers, mwh pgpdSphHFvTaL.pgp Description: PGP signature
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
On Wed, Jan 7, 2015 at 4:39 PM, Michael Hudson-Doyle wrote: > Ian Lance Taylor writes: > >> On Wed, Jan 7, 2015 at 9:26 AM, Lynn A. Boger >> wrote: >>> >>> In libgo/go/reflect/makefunc.go, calls to MakeFunc, makeMethodValue and >>> makeValueMethod will panic if called when GOARCH is ppc64 or ppc64le. >> >> Right, I'm just saying that almost no code actually does that. I just >> tried a web search and found no uses other than examples of how to use >> it. I'm sure there are a few, but not many. > > There is a somewhat hidden one in Docker: > > https://github.com/docker/docker/blob/master/api/client/cli.go#L64 > > (it is also possible to patch docker to do this differently, of course). Ah, so that is why it matters. Perhaps you could talk Docker into replace the return statement with return func(a ...string) error { return method.CallSlice([]reflect.Value{reflect.ValueOf(a)})[0].Interface().(error) }, true It would probably be just as efficient. Ian
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
If this change is not included, in the gcc go testsuite, the recover.go testcase fails with this error: panic: reflect.makeMethodValue not implemented for ppc64le when running this test: func test9reflect1() { f := reflect.ValueOf(&T1{}).Method(0).Interface().(func()) defer f() panic(9) } Also as was noted below, some level of reflection support is needed to build Docker. I was given this patch by someone in IBM trying to build Docker on Power who told me it was needed (but didn't really know the details). There is interest in trying to get Docker to build with gcc 4.9 because 5.0 won't be available in distros for a while yet. I was told that the minimal reflection support provided by this patch was enough to get it to build. But if a simple workaround in the Docker source will make it work that might be sufficient. I'll find out. Thanks. On 01/07/2015 07:20 PM, Ian Lance Taylor wrote: On Wed, Jan 7, 2015 at 4:39 PM, Michael Hudson-Doyle wrote: Ian Lance Taylor writes: On Wed, Jan 7, 2015 at 9:26 AM, Lynn A. Boger wrote: In libgo/go/reflect/makefunc.go, calls to MakeFunc, makeMethodValue and makeValueMethod will panic if called when GOARCH is ppc64 or ppc64le. Right, I'm just saying that almost no code actually does that. I just tried a web search and found no uses other than examples of how to use it. I'm sure there are a few, but not many. There is a somewhat hidden one in Docker: https://github.com/docker/docker/blob/master/api/client/cli.go#L64 (it is also possible to patch docker to do this differently, of course). Ah, so that is why it matters. Perhaps you could talk Docker into replace the return statement with return func(a ...string) error { return method.CallSlice([]reflect.Value{reflect.ValueOf(a)})[0].Interface().(error) }, true It would probably be just as efficient. Ian
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
On Thu, Jan 8, 2015 at 5:44 AM, Lynn A. Boger wrote: > > Also as was noted below, some level of reflection support is needed to build > Docker. Sorry to be so pedantic, but I'm just trying to clarify that you keep saying "reflection support," but reflection support is there. What is missing is a miniscule rarely used part of the reflect package, namely support for reflect.MakeFunc, and, it's true, calling reflect.ValueOf, then Method or MethodByName, then Interface. Ian
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
The only bugs I am aware of when using reflection on ppc64 & ppc64le in gcc 4.9 are in the gccgo testcase recover.go and what is needed to build Docker. When I run the gccgo testsuite and see a failing testcase with a panic message saying that a function is not implemented it makes me think some major pieces are missing so that is why I've used that phrase. Also I noticed that the libgo/go/reflect/all_test.go testcase is not running some tests when GOARCH is ppc64 or ppc64le, and if those are enabled they fail. So some might call those "bugs" or "unimplemented features" or "missing support". To me, a message saying that something is not implemented is considered missing support even though it might be minor. The information I received from those who had attempted to build Docker on ppc64le in gcc 4.9 said that it needed reflection and it wasn't there and this patch was used to make it work. I hope that answers your question? I have not done a thorough testing/investigation of reflection in gccgo 4.9 on Power and my statement refers only on the above information. On 01/08/2015 08:53 AM, Ian Lance Taylor wrote: On Thu, Jan 8, 2015 at 5:44 AM, Lynn A. Boger wrote:I Also as was noted below, some level of reflection support is needed to build Docker. Sorry to be so pedantic, but I'm just trying to clarify that you keep saying "reflection support," but reflection support is there. What is missing is a miniscule rarely used part of the reflect package, namely support for reflect.MakeFunc, and, it's true, calling reflect.ValueOf, then Method or MethodByName, then Interface. Ian
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
I have an updated patch for the reflection functions in gcc 4.9 for ppc64 & ppc64le which allows the recover.go test to pass and the updated version of libgo/go/reflect/all_tests.go to pass once all testcases are enabled for ppc64 & ppc64le. Based on the discussion we've had on this patch, has the decision changed concerning whether or not this patch might be accepted into the gcc 4.9 branch? If so, I will post the updated patch. On 01/08/2015 08:53 AM, Ian Lance Taylor wrote: On Thu, Jan 8, 2015 at 5:44 AM, Lynn A. Boger wrote: Also as was noted below, some level of reflection support is needed to build Docker. Sorry to be so pedantic, but I'm just trying to clarify that you keep saying "reflection support," but reflection support is there. What is missing is a miniscule rarely used part of the reflect package, namely support for reflect.MakeFunc, and, it's true, calling reflect.ValueOf, then Method or MethodByName, then Interface. Ian
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
On Wed, Jan 14, 2015 at 5:49 AM, Lynn A. Boger wrote: > I have an updated patch for the reflection functions in gcc 4.9 for ppc64 & > ppc64le which allows the recover.go test to pass and the updated version of > libgo/go/reflect/all_tests.go to pass once all testcases are enabled for > ppc64 & ppc64le. > > Based on the discussion we've had on this patch, has the decision changed > concerning whether or not this patch might be accepted into the gcc 4.9 > branch? If so, I will post the updated patch. To me it is not appropriate to add brand new code, code that has never been in mainline, to an existing release branch. That is not what release branches are for. Release branches are for backports of bug fixes, not for new code. I assume that the people who want this code are not going to sit on their hands waiting for the 4.9.3 release, which won't happen for several months even in the best case. Why not provide this code as a separate patch on top of the 4.9 releases? Or manage your own branch if necessary? Ian
Re: [gofrontend-dev] [PATCH 1/2, libgo] Add reflection support to gccgo for ppc64, ppc64le in gcc 4.9
Yes, I have other options, I just wanted to verify that this was still your answer. Thanks. On 01/14/2015 09:24 AM, Ian Lance Taylor wrote: On Wed, Jan 14, 2015 at 5:49 AM, Lynn A. Boger wrote: I have an updated patch for the reflection functions in gcc 4.9 for ppc64 & ppc64le which allows the recover.go test to pass and the updated version of libgo/go/reflect/all_tests.go to pass once all testcases are enabled for ppc64 & ppc64le. Based on the discussion we've had on this patch, has the decision changed concerning whether or not this patch might be accepted into the gcc 4.9 branch? If so, I will post the updated patch. To me it is not appropriate to add brand new code, code that has never been in mainline, to an existing release branch. That is not what release branches are for. Release branches are for backports of bug fixes, not for new code. I assume that the people who want this code are not going to sit on their hands waiting for the 4.9.3 release, which won't happen for several months even in the best case. Why not provide this code as a separate patch on top of the 4.9 releases? Or manage your own branch if necessary? Ian