Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: failure to build with riscv64 and go-bootstrap #52583

Closed
paralin opened this issue Apr 27, 2022 · 7 comments
Closed

build: failure to build with riscv64 and go-bootstrap #52583

paralin opened this issue Apr 27, 2022 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@paralin
Copy link
Contributor

paralin commented Apr 27, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18.1 linux/amd64

Description

Using the go-bootstrap-1.4.x C-based Go compiler to bootstrap Go with build.bash,

When the target GOARCH is set to 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)

The code attempts to unset GOARCH before calling the bootstrap compiler, since the bootstrap compiler only needs to build for the host OS during this stage of the bootstrap. In later bootstrap passes, support for the actual target arch is usually compiled in.

However, the environment variable is unset /after/ calling bootstrapRewriteFile, which causes the issue.

By unsetting GOARCH /before/ calling bootstrapRewriteFile, the build.bash script completes successfully.

Fixed by PR: https://go-review.googlesource.com/c/go/+/400376

@ianlancetaylor
Copy link
Contributor

CC @4a6f656c

@mengzhuo
Copy link
Contributor

BTW, I can't reproduce with go 1.16
GOARCH=riscv64 GOOS=linux ./bootstrap.sh

@paralin
Copy link
Contributor Author

paralin commented Apr 28, 2022

@mengzhuo the issue appears only when bootstrapping with go 1.4.x.

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 2, 2022
@cagedmantis cagedmantis added this to the Backlog milestone May 2, 2022
@paralin
Copy link
Contributor Author

paralin commented May 11, 2022

It's a simple fix, not sure what more investigation is needed. The buildcfg in that bootstrap step does not need the GOARCH environment variables to be set. It was a mistake in the code to call buildcfg before unsetting those environment variables. The original intent of the code was to have those unset during this portion of the bootstrap process.

This fixes a build failure in Buildroot and is currently included as a .patch in the tree there. We build Go with the go1.4.x go-bootstrap as per this: https://go.dev/doc/install/source#bootstrapFromSource

Please review when possible so we can upstream this patch. Thanks.

paralin added a commit to paralin/go that referenced this issue May 11, 2022
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.

[Buildroot]: submitted to upstream:

 - golang#52583
 - https://go-review.googlesource.com/c/go/+/400376
 - GitHub-Pull-Request: golang#52362

Signed-off-by: Christian Stewart <christian@paral.in>
@mengzhuo
Copy link
Contributor

cc @bcmills

paralin added a commit to paralin/go that referenced this issue May 13, 2022
The GOOS and GOARCH environment variables should be unset before calling
mkbuildcfg, as the target GOOS and GOARCH is not relevant while compiling the
bootstrap Go compiler using the C-based go-bootstrap go1.4 compiler.

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)

This is because:

 - buildtool.go:198: calls bootstrapRewriteFile(src)
 - bootstrapRewriteFile: buildtool.go:283 calls:
 - isUnneededSSARewriteFile: checks os.Getenv("GOARCH")
 - isUnneededSSARewriteFile: returns "", false
 - bootstrapRewriteFile: calls bootstrapFixImports
 - boostrapFixImports: generates code go1.4 cannot compile

By unsetting GOARCH here we are causing isUnneededSSARewriteFile to return true
instead of false which generates stub functions bypassing the incompatible code.

buildtool.go:272 in isUnneededSSARewriteFile is the only place the GOARCH
environment variable is checked apart from the xinit function.

This patch simply moves the os.Setenv("GOARCH", "") to before the block of code
where bootstrapRewriteFile is called.

[Buildroot]: submitted to upstream:

- golang#52583
- https://go-review.googlesource.com/c/go/+/400376
- GitHub-Pull-Request: golang#52362

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/go that referenced this issue May 13, 2022
The GOOS and GOARCH environment variables should be unset before calling
mkbuildcfg, as the target GOOS and GOARCH is not relevant while compiling the
bootstrap Go compiler using the C-based go-bootstrap go1.4 compiler.

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)

This is because:

 - buildtool.go:198: calls bootstrapRewriteFile(src)
 - bootstrapRewriteFile: buildtool.go:283 calls:
 - isUnneededSSARewriteFile: checks os.Getenv("GOARCH")
 - isUnneededSSARewriteFile: returns "", false
 - bootstrapRewriteFile: calls bootstrapFixImports
 - boostrapFixImports: generates code go1.4 cannot compile

By unsetting GOARCH here we are causing isUnneededSSARewriteFile to return true
instead of false which generates stub functions bypassing the incompatible code.

buildtool.go:272 in isUnneededSSARewriteFile is the only place the GOARCH
environment variable is checked apart from the xinit function.

This patch simply moves the os.Setenv("GOARCH", "") to before the block of code
where bootstrapRewriteFile is called.

[Buildroot]: submitted to upstream:

- golang#52583
- https://go-review.googlesource.com/c/go/+/400376
- GitHub-Pull-Request: golang#52362

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/go that referenced this issue May 13, 2022
The GOOS and GOARCH environment variables should be unset before calling
mkbuildcfg, as the target GOOS and GOARCH is not relevant while compiling the
bootstrap Go compiler using the C-based go-bootstrap go1.4 compiler.

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)

This is because:

 - buildtool.go:198: calls bootstrapRewriteFile(src)
 - bootstrapRewriteFile: buildtool.go:283 calls:
 - isUnneededSSARewriteFile: checks os.Getenv("GOARCH")
 - isUnneededSSARewriteFile: returns "", false
 - bootstrapRewriteFile: calls bootstrapFixImports
 - boostrapFixImports: generates code go1.4 cannot compile

By unsetting GOARCH here we are causing isUnneededSSARewriteFile to return true
instead of false which generates stub functions bypassing the incompatible code.

buildtool.go:272 in isUnneededSSARewriteFile is the only place the GOARCH
environment variable is checked apart from the xinit function.

This patch simply moves the os.Setenv("GOARCH", "") to before the block of code
where bootstrapRewriteFile is called.

[Buildroot]: submitted to upstream:

- golang#52583
- https://go-review.googlesource.com/c/go/+/400376
- GitHub-Pull-Request: golang#52362

Signed-off-by: Christian Stewart <christian@paral.in>
@paralin
Copy link
Contributor Author

paralin commented May 13, 2022

Updated & confirmed that the build fails w/o the patch & is fixed with it.

Determined why & updated commit message:

  • buildtool.go:198: calls bootstrapRewriteFile(src)
  • bootstrapRewriteFile: buildtool.go:283 calls:
  • isUnneededSSARewriteFile: checks os.Getenv("GOARCH")
  • isUnneededSSARewriteFile: returns "", false
  • bootstrapRewriteFile: calls bootstrapFixImports
  • boostrapFixImports: generates code go1.4 cannot compile

By unsetting GOARCH here we are causing isUnneededSSARewriteFile to return true
instead of false which generates stub functions bypassing the incompatible code.

buildtool.go:272 in isUnneededSSARewriteFile is the only place the GOARCH
environment variable is checked apart from the xinit function.

This patch simply moves the os.Setenv("GOARCH", "") to before the block of code
where bootstrapRewriteFile is called.

paralin added a commit to paralin/go that referenced this issue May 25, 2022
Fix a build failure when bootstrapping the Go compiler with go-bootstrap 1.4
while the environment contains 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)

This is because:

 - buildtool.go:198: calls bootstrapRewriteFile(src)
 - bootstrapRewriteFile: buildtool.go:283 calls:
 - isUnneededSSARewriteFile: checks os.Getenv("GOARCH")
 - isUnneededSSARewriteFile: returns "", false
 - bootstrapRewriteFile: calls bootstrapFixImports
 - boostrapFixImports: generates code go1.4 cannot compile

Instead of checking "GOARCH" in the environment, use the gohostarch variable.

---

[Buildroot]: submitted to upstream:

- golang#52583
- https://go-review.googlesource.com/c/go/+/400376
- GitHub-Pull-Request: golang#52362

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

Change https://go.dev/cl/409974 mentions this issue: cmd/compile: cast riscv64 rewrite shifts to unsigned int

@golang golang locked and limited conversation to collaborators Jun 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants