diff mbox series

[v2,1/1] package/go: add support for riscv64 architecture

Message ID 20220414204922.866533-1-christian@paral.in
State Changes Requested, archived
Headers show
Series [v2,1/1] package/go: add support for riscv64 architecture | expand

Commit Message

Christian Stewart April 14, 2022, 8:49 p.m. UTC 
Enable the supported "riscv64" GOARCH.

Add a patch to fix a build failure due to GOARCH leaking into the calls to the
go-bootstrap compiler. Unsets the GOARCH before calling go-bootstrap.

PR: https://github.com/golang/go/pull/52362

Signed-off-by: Christian Stewart <christian@paral.in>

---

v1 -> v2:

 - fixed build error with go-bootstrap

Signed-off-by: Christian Stewart <christian@paral.in>
---
 ...explicit-option-for-crosscompilation.patch |   8 +-
 ...set-environment-before-generating-bu.patch | 111 ++++++++++++++++++
 package/go/Config.in.host                     |   2 +-
 package/go/go.mk                              |   2 +
 4 files changed, 118 insertions(+), 5 deletions(-)
 create mode 100644 package/go/0002-build-bootstrap-set-environment-before-generating-bu.patch

Comments

Arnout Vandecappelle April 21, 2022, 7:54 p.m. UTC | #1 
On 14/04/2022 22:49, Christian Stewart via buildroot wrote:
> Enable the supported "riscv64" GOARCH.
> 
> Add a patch to fix a build failure due to GOARCH leaking into the calls to the
> go-bootstrap compiler. Unsets the GOARCH before calling go-bootstrap.
> 
> PR: https://github.com/golang/go/pull/52362
> 
> Signed-off-by: Christian Stewart <christian@paral.in>
> 
> ---
> 
> v1 -> v2:
> 
>   - fixed build error with go-bootstrap
> 
> Signed-off-by: Christian Stewart <christian@paral.in>
> ---
>   ...explicit-option-for-crosscompilation.patch |   8 +-
>   ...set-environment-before-generating-bu.patch | 111 ++++++++++++++++++
>   package/go/Config.in.host                     |   2 +-
>   package/go/go.mk                              |   2 +
>   4 files changed, 118 insertions(+), 5 deletions(-)
>   create mode 100644 package/go/0002-build-bootstrap-set-environment-before-generating-bu.patch
> 
> diff --git a/package/go/0001-build.go-explicit-option-for-crosscompilation.patch b/package/go/0001-build.go-explicit-option-for-crosscompilation.patch
> index f76c5b1d03..a32a6ee355 100644
> --- a/package/go/0001-build.go-explicit-option-for-crosscompilation.patch
> +++ b/package/go/0001-build.go-explicit-option-for-crosscompilation.patch
> @@ -1,4 +1,4 @@
> -From e1382a731a726293e30901038c6870fa77ef6095 Mon Sep 17 00:00:00 2001
> +From 86fa3da18b5c73cc84dec91d92db4ae95f288bdc Mon Sep 17 00:00:00 2001
>   From: Angelo Compagnucci <angelo@amarulasolutions.com>
>   Date: Tue, 8 May 2018 16:08:44 +0200
>   Subject: [PATCH] build.go: explicit option for crosscompilation
> @@ -17,10 +17,10 @@ Signed-off-by: Anisse Astier <anisse@astier.eu>
>    1 file changed, 2 insertions(+), 1 deletion(-)
>   
>   diff --git a/src/cmd/dist/build.go b/src/cmd/dist/build.go
> -index 99d1db5..eb4097f 100644
> +index d37c3f83ef..d838485bd3 100644
>   --- a/src/cmd/dist/build.go
>   +++ b/src/cmd/dist/build.go
> -@@ -252,12 +252,13 @@ func xinit() {
> +@@ -273,12 +273,13 @@ func xinit() {
>    // $CC_FOR_goos_goarch, if set, applies only to goos/goarch.
>    func compilerEnv(envName, def string) map[string]string {
>    	m := map[string]string{"": def}
> @@ -36,5 +36,5 @@ index 99d1db5..eb4097f 100644
>    		}
>    		m[""] = env
>   --
> -2.7.4
> +2.35.1
>   
> diff --git a/package/go/0002-build-bootstrap-set-environment-before-generating-bu.patch b/package/go/0002-build-bootstrap-set-environment-before-generating-bu.patch
> new file mode 100644
> index 0000000000..44b3fbecc2
> --- /dev/null
> +++ b/package/go/0002-build-bootstrap-set-environment-before-generating-bu.patch
> @@ -0,0 +1,111 @@
> +From 00f3f4ee0d477779c87e4e60b08708362670e122 Mon Sep 17 00:00:00 2001
> +From: Christian Stewart <christian@paral.in>
> +Date: Thu, 14 Apr 2022 13:34:26 -0700
> +Subject: [PATCH] build: bootstrap: set environment before generating buildcfg
> +
> +The GOOS and GOARCH environment variables should be unset before calling
> +mkbuildcfg. This change fixes a build failure when GOARCH=riscv64.
> +
> +Building Go toolchain1 using go-1.4-bootstrap-20171003.
> +src/cmd/compile/internal/ssa/rewriteRISCV64.go:4814
> +invalid operation: y << x (shift count type int64, must be unsigned integer)
> +
> +There is a build issue with go1.4 with the riscv64 code: however, why is the
> +riscv64 code being compiled at all?
> +
> +GOARCH is set when calling mkbuildcfg, so go1.4 is trying to compile riscv64.

  You SoB is missing.

  Also, the reference to the upstream PR is missing. And you should probably 
refer to gerrit instead.

[snip]
> diff --git a/package/go/Config.in.host b/package/go/Config.in.host
> index e82ab6e81a..8be86103ea 100644
> --- a/package/go/Config.in.host
> +++ b/package/go/Config.in.host
> @@ -5,7 +5,7 @@ config BR2_PACKAGE_HOST_GO_TARGET_ARCH_SUPPORTS
>   	depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_ARCH_SUPPORTS
>   	depends on (BR2_arm && BR2_TOOLCHAIN_SUPPORTS_PIE) || BR2_aarch64 \
>   		|| BR2_i386 || BR2_x86_64 || BR2_powerpc64le \
> -		|| BR2_mips64 || BR2_mips64el || BR2_s390x
> +		|| BR2_mips64 || BR2_mips64el || BR2_riscv || BR2_s390x

  Here you enable it both for riscv32 and riscv64, but below you always set 
GOARCH to riscv64.

  Also, are you sure it doesn't need any of the ISA extensions? I can imagine 
that Go would be using atomics, floating point and integer 
multiplication/division. Of course, there's almost no chance of finding a 
riscv64 without these extensions, but better be correct, right?

  Also, does Go support all three ABI types (integer, single precision and 
double precision)? Again, we only ever test lp64d in the autobuilders, but 
better be correct, right?

  Regards,
  Arnout

>   	depends on !BR2_ARM_CPU_ARMV4
>   	# MIPS R6 support in Go has not yet been developed.
>   	depends on !BR2_MIPS_CPU_MIPS64R6
> diff --git a/package/go/go.mk b/package/go/go.mk
> index 3df16c9a68..ba0da74b86 100644
> --- a/package/go/go.mk
> +++ b/package/go/go.mk
> @@ -63,6 +63,8 @@ else ifeq ($(BR2_mips64),y)
>   GO_GOARCH = mips64
>   else ifeq ($(BR2_mips64el),y)
>   GO_GOARCH = mips64le
> +else ifeq ($(BR2_riscv),y)
> +GO_GOARCH = riscv64
>   else ifeq ($(BR2_s390x),y)
>   GO_GOARCH = s390x
>   endif
Christian Stewart April 27, 2022, 12:55 a.m. UTC | #2 
Arnout,

On Thu, Apr 21, 2022 at 12:54 PM Arnout Vandecappelle <arnout@mind.be> wrote:
> On 14/04/2022 22:49, Christian Stewart via buildroot wrote:
> > +GOARCH is set when calling mkbuildcfg, so go1.4 is trying to compile riscv64.
>
>   You SoB is missing.

Go's repository disallows signed-off-by lines.

>   Also, the reference to the upstream PR is missing. And you should probably
> refer to gerrit instead.

Same issue: I've format-patch the exact patch sent upstream, without changes.

Do I modify it specifically for this patch in Buildroot?


> > diff --git a/package/go/Config.in.host b/package/go/Config.in.host
> > index e82ab6e81a..8be86103ea 100644
> > --- a/package/go/Config.in.host
> > +++ b/package/go/Config.in.host
> > @@ -5,7 +5,7 @@ config BR2_PACKAGE_HOST_GO_TARGET_ARCH_SUPPORTS
> >       depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_ARCH_SUPPORTS
> >       depends on (BR2_arm && BR2_TOOLCHAIN_SUPPORTS_PIE) || BR2_aarch64 \
> >               || BR2_i386 || BR2_x86_64 || BR2_powerpc64le \
> > -             || BR2_mips64 || BR2_mips64el || BR2_s390x
> > +             || BR2_mips64 || BR2_mips64el || BR2_riscv || BR2_s390x
>
>   Here you enable it both for riscv32 and riscv64, but below you always set
> GOARCH to riscv64.

I'm not sure if it was in this patch revision, but I have in my local version:

    # Go doesn't support Risc-v 32-bit.
    depends on !BR2_RISCV_32

> Also, are you sure it doesn't need any of the ISA extensions? I can imagine
> that Go would be using atomics, floating point and integer
> multiplication/division. Of course, there's almost no chance of finding a
> riscv64 without these extensions, but better be correct, right?

Probably yes.

>  Also, does Go support all three ABI types (integer, single precision and
> double precision)? Again, we only ever test lp64d in the autobuilders, but
> better be correct, right?

There's only one riscv64 target for Go so I have to assume it means lp64d.

Note: I have now tested this extensively on a Nezha Risc-v CPU as well
as a Qemu VM.

https://github.com/skiffos/buildroot/blob/e1fc20cf9/package/go/Config.in.host#L12

Best,
Christian Stewart
Arnout Vandecappelle April 27, 2022, 7:28 a.m. UTC | #3 
On 27/04/2022 02:55, Christian Stewart wrote:
> Arnout,
> 
> On Thu, Apr 21, 2022 at 12:54 PM Arnout Vandecappelle <arnout@mind.be> wrote:
>> On 14/04/2022 22:49, Christian Stewart via buildroot wrote:
>>> +GOARCH is set when calling mkbuildcfg, so go1.4 is trying to compile riscv64.
>>
>>    You SoB is missing.
> 
> Go's repository disallows signed-off-by lines.
> 
>>    Also, the reference to the upstream PR is missing. And you should probably
>> refer to gerrit instead.
> 
> Same issue: I've format-patch the exact patch sent upstream, without changes.
> 
> Do I modify it specifically for this patch in Buildroot?

  Exactly. Alternatively, you can use GO_PATCH = ... to download the upstream 
patch (cfr. how it's done in zfs). In that case, you also need to add a hash for 
it. We currently don't use that method very often but I think it's a good one in 
fact.

>>> diff --git a/package/go/Config.in.host b/package/go/Config.in.host
>>> index e82ab6e81a..8be86103ea 100644
>>> --- a/package/go/Config.in.host
>>> +++ b/package/go/Config.in.host
>>> @@ -5,7 +5,7 @@ config BR2_PACKAGE_HOST_GO_TARGET_ARCH_SUPPORTS
>>>        depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_ARCH_SUPPORTS
>>>        depends on (BR2_arm && BR2_TOOLCHAIN_SUPPORTS_PIE) || BR2_aarch64 \
>>>                || BR2_i386 || BR2_x86_64 || BR2_powerpc64le \
>>> -             || BR2_mips64 || BR2_mips64el || BR2_s390x
>>> +             || BR2_mips64 || BR2_mips64el || BR2_riscv || BR2_s390x
>>
>>    Here you enable it both for riscv32 and riscv64, but below you always set
>> GOARCH to riscv64.
> 
> I'm not sure if it was in this patch revision, but I have in my local version:
> 
>      # Go doesn't support Risc-v 32-bit.
>      depends on !BR2_RISCV_32

  That is indeed missing from this patch.


>> Also, are you sure it doesn't need any of the ISA extensions? I can imagine
>> that Go would be using atomics, floating point and integer
>> multiplication/division. Of course, there's almost no chance of finding a
>> riscv64 without these extensions, but better be correct, right?
> 
> Probably yes.

  Is there an easy way to find out, or should we just depend on all of them?


>>   Also, does Go support all three ABI types (integer, single precision and
>> double precision)? Again, we only ever test lp64d in the autobuilders, but
>> better be correct, right?
> 
> There's only one riscv64 target for Go so I have to assume it means lp64d.

  Please add that to the BR2_PACKAGE_HOST_GO_TARGET_ARCH_SUPPORTS dependencies.

  Regards,
  Arnout

> 
> Note: I have now tested this extensively on a Nezha Risc-v CPU as well
> as a Qemu VM.
> 
> https://github.com/skiffos/buildroot/blob/e1fc20cf9/package/go/Config.in.host#L12
> 
> Best,
> Christian Stewart
diff mbox series

Patch

diff --git a/package/go/0001-build.go-explicit-option-for-crosscompilation.patch b/package/go/0001-build.go-explicit-option-for-crosscompilation.patch
index f76c5b1d03..a32a6ee355 100644
--- a/package/go/0001-build.go-explicit-option-for-crosscompilation.patch
+++ b/package/go/0001-build.go-explicit-option-for-crosscompilation.patch
@@ -1,4 +1,4 @@ 
-From e1382a731a726293e30901038c6870fa77ef6095 Mon Sep 17 00:00:00 2001
+From 86fa3da18b5c73cc84dec91d92db4ae95f288bdc Mon Sep 17 00:00:00 2001
 From: Angelo Compagnucci <angelo@amarulasolutions.com>
 Date: Tue, 8 May 2018 16:08:44 +0200
 Subject: [PATCH] build.go: explicit option for crosscompilation
@@ -17,10 +17,10 @@  Signed-off-by: Anisse Astier <anisse@astier.eu>
  1 file changed, 2 insertions(+), 1 deletion(-)
 
 diff --git a/src/cmd/dist/build.go b/src/cmd/dist/build.go
-index 99d1db5..eb4097f 100644
+index d37c3f83ef..d838485bd3 100644
 --- a/src/cmd/dist/build.go
 +++ b/src/cmd/dist/build.go
-@@ -252,12 +252,13 @@ func xinit() {
+@@ -273,12 +273,13 @@ func xinit() {
  // $CC_FOR_goos_goarch, if set, applies only to goos/goarch.
  func compilerEnv(envName, def string) map[string]string {
  	m := map[string]string{"": def}
@@ -36,5 +36,5 @@  index 99d1db5..eb4097f 100644
  		}
  		m[""] = env
 -- 
-2.7.4
+2.35.1
 
diff --git a/package/go/0002-build-bootstrap-set-environment-before-generating-bu.patch b/package/go/0002-build-bootstrap-set-environment-before-generating-bu.patch
new file mode 100644
index 0000000000..44b3fbecc2
--- /dev/null
+++ b/package/go/0002-build-bootstrap-set-environment-before-generating-bu.patch
@@ -0,0 +1,111 @@ 
+From 00f3f4ee0d477779c87e4e60b08708362670e122 Mon Sep 17 00:00:00 2001
+From: Christian Stewart <christian@paral.in>
+Date: Thu, 14 Apr 2022 13:34:26 -0700
+Subject: [PATCH] build: bootstrap: set environment before generating buildcfg
+
+The GOOS and GOARCH environment variables should be unset before calling
+mkbuildcfg. This change fixes a build failure when GOARCH=riscv64.
+
+Building Go toolchain1 using go-1.4-bootstrap-20171003.
+src/cmd/compile/internal/ssa/rewriteRISCV64.go:4814
+invalid operation: y << x (shift count type int64, must be unsigned integer)
+
+There is a build issue with go1.4 with the riscv64 code: however, why is the
+riscv64 code being compiled at all?
+
+GOARCH is set when calling mkbuildcfg, so go1.4 is trying to compile riscv64.
+---
+ src/cmd/dist/buildtool.go | 56 ++++++++++++++++++++-------------------
+ 1 file changed, 29 insertions(+), 27 deletions(-)
+
+diff --git a/src/cmd/dist/buildtool.go b/src/cmd/dist/buildtool.go
+index 036f8c52fa..73f6524969 100644
+--- a/src/cmd/dist/buildtool.go
++++ b/src/cmd/dist/buildtool.go
+@@ -112,9 +112,6 @@ func bootstrapBuildTools() {
+ 	}
+ 	xprintf("Building Go toolchain1 using %s.\n", goroot_bootstrap)
+ 
+-	mkbuildcfg(pathf("%s/src/internal/buildcfg/zbootstrap.go", goroot))
+-	mkobjabi(pathf("%s/src/cmd/internal/objabi/zbootstrap.go", goroot))
+-
+ 	// Use $GOROOT/pkg/bootstrap as the bootstrap workspace root.
+ 	// We use a subdirectory of $GOROOT/pkg because that's the
+ 	// space within $GOROOT where we store all generated objects.
+@@ -126,6 +123,34 @@ func bootstrapBuildTools() {
+ 	base := pathf("%s/src/bootstrap", workspace)
+ 	xmkdirall(base)
+ 
++	// Set up environment for invoking Go 1.4 go command.
++	// GOROOT points at Go 1.4 GOROOT,
++	// GOPATH points at our bootstrap workspace,
++	// GOBIN is empty, so that binaries are installed to GOPATH/bin,
++	// and GOOS, GOHOSTOS, GOARCH, and GOHOSTOS are empty,
++	// so that Go 1.4 builds whatever kind of binary it knows how to build.
++	// Restore GOROOT, GOPATH, and GOBIN when done.
++	// Don't bother with GOOS, GOHOSTOS, GOARCH, and GOHOSTARCH,
++	// because setup will take care of those when bootstrapBuildTools returns.
++
++	defer os.Setenv("GOROOT", os.Getenv("GOROOT"))
++	os.Setenv("GOROOT", goroot_bootstrap)
++
++	defer os.Setenv("GOPATH", os.Getenv("GOPATH"))
++	os.Setenv("GOPATH", workspace)
++
++	defer os.Setenv("GOBIN", os.Getenv("GOBIN"))
++	os.Setenv("GOBIN", "")
++
++	os.Setenv("GOOS", "")
++	os.Setenv("GOHOSTOS", "")
++	os.Setenv("GOARCH", "")
++	os.Setenv("GOHOSTARCH", "")
++
++	// Create the build config files.
++	mkbuildcfg(pathf("%s/src/internal/buildcfg/zbootstrap.go", goroot))
++	mkobjabi(pathf("%s/src/cmd/internal/objabi/zbootstrap.go", goroot))
++
+ 	// Copy source code into $GOROOT/pkg/bootstrap and rewrite import paths.
+ 	writefile("module bootstrap\n", pathf("%s/%s", base, "go.mod"), 0)
+ 	for _, dir := range bootstrapDirs {
+@@ -172,30 +197,6 @@ func bootstrapBuildTools() {
+ 		})
+ 	}
+ 
+-	// Set up environment for invoking Go 1.4 go command.
+-	// GOROOT points at Go 1.4 GOROOT,
+-	// GOPATH points at our bootstrap workspace,
+-	// GOBIN is empty, so that binaries are installed to GOPATH/bin,
+-	// and GOOS, GOHOSTOS, GOARCH, and GOHOSTOS are empty,
+-	// so that Go 1.4 builds whatever kind of binary it knows how to build.
+-	// Restore GOROOT, GOPATH, and GOBIN when done.
+-	// Don't bother with GOOS, GOHOSTOS, GOARCH, and GOHOSTARCH,
+-	// because setup will take care of those when bootstrapBuildTools returns.
+-
+-	defer os.Setenv("GOROOT", os.Getenv("GOROOT"))
+-	os.Setenv("GOROOT", goroot_bootstrap)
+-
+-	defer os.Setenv("GOPATH", os.Getenv("GOPATH"))
+-	os.Setenv("GOPATH", workspace)
+-
+-	defer os.Setenv("GOBIN", os.Getenv("GOBIN"))
+-	os.Setenv("GOBIN", "")
+-
+-	os.Setenv("GOOS", "")
+-	os.Setenv("GOHOSTOS", "")
+-	os.Setenv("GOARCH", "")
+-	os.Setenv("GOHOSTARCH", "")
+-
+ 	// Run Go 1.4 to build binaries. Use -gcflags=-l to disable inlining to
+ 	// workaround bugs in Go 1.4's compiler. See discussion thread:
+ 	// https://groups.google.com/d/msg/golang-dev/Ss7mCKsvk8w/Gsq7VYI0AwAJ
+@@ -217,6 +218,7 @@ func bootstrapBuildTools() {
+ 		cmd = append(cmd, "-toolexec="+tool)
+ 	}
+ 	cmd = append(cmd, "bootstrap/cmd/...")
++
+ 	run(base, ShowOutput|CheckExit, cmd...)
+ 
+ 	// Copy binaries into tool binary directory.
+-- 
+2.35.1
+
diff --git a/package/go/Config.in.host b/package/go/Config.in.host
index e82ab6e81a..8be86103ea 100644
--- a/package/go/Config.in.host
+++ b/package/go/Config.in.host
@@ -5,7 +5,7 @@  config BR2_PACKAGE_HOST_GO_TARGET_ARCH_SUPPORTS
 	depends on BR2_PACKAGE_HOST_GO_BOOTSTRAP_ARCH_SUPPORTS
 	depends on (BR2_arm && BR2_TOOLCHAIN_SUPPORTS_PIE) || BR2_aarch64 \
 		|| BR2_i386 || BR2_x86_64 || BR2_powerpc64le \
-		|| BR2_mips64 || BR2_mips64el || BR2_s390x
+		|| BR2_mips64 || BR2_mips64el || BR2_riscv || BR2_s390x
 	depends on !BR2_ARM_CPU_ARMV4
 	# MIPS R6 support in Go has not yet been developed.
 	depends on !BR2_MIPS_CPU_MIPS64R6
diff --git a/package/go/go.mk b/package/go/go.mk
index 3df16c9a68..ba0da74b86 100644
--- a/package/go/go.mk
+++ b/package/go/go.mk
@@ -63,6 +63,8 @@  else ifeq ($(BR2_mips64),y)
 GO_GOARCH = mips64
 else ifeq ($(BR2_mips64el),y)
 GO_GOARCH = mips64le
+else ifeq ($(BR2_riscv),y)
+GO_GOARCH = riscv64
 else ifeq ($(BR2_s390x),y)
 GO_GOARCH = s390x
 endif