cmd/compile: some x/sys versions no longer build due to "go:linkname must refer to declared function or variable" #55889
Comments
|
We can always make this a proposal if needed, but hopefully this is not a controversial idea. If we agree that this is a valid problem, but we're not happy with the solution, I am also happy to re-purpose this issue to simply be a UX bug report. |
Why does it need to do that? Perhaps we should file a proposal for whatever new standard-library APIs would be needed to avoid it. (Looks like it's mostly just variations on
“Too old” should be covered by the “Too new” seems like a violation of the compatibility policy. Since |
I'm not sure; I've never developed that module. Removing the coupling would indeed be best.
I guess you're right. I think we can largely put this concern aside, because most people run into "Go version too new" instead.
I think that would make https://go-review.googlesource.com/c/go/+/333109/ the breaking change that we could reconsider. With it, newer Go versions fail to build older |
Yep, looks like it. Ideally that change in error behavior should have been conditional on the |
dmitshur
added
the
NeedsInvestigation
mvdan
changed the title
|
I have repurposed this issue to be about cmd/compile's apparent regression. |
gopherbot
added
the
compiler/runtime
|
Thinking outloud, if we do resolve this regression somehow, we should probably backport the fix to Go 1.18.x and 1.19.x - otherwise the affected users are stuck on 1.17.x until 1.20 is released. They can always update |
|
I'd argue the opposite. The way x/sys accesses the internal details of the runtime and standard library using unsafe construct and assembly code is not covered by the compatibility guarantee. This makes it very difficult to keep old versions of x/sys work while Go evolves. Ideally x/sys shouldn't do this, but perhaps that is not possible? Maybe the API boundary between x/sys and the Go standard library + toolchain is not ideal. Unfortunately it is hard to co-evolve x/sys and the Go toolchain+std (unlike, say, the compiler and runtime). This is what the original post is trying to resolve, as well as #55092 and #55090. Another example is that in Go 1.17 when we switched to the register ABI, even older versions of x/sys also broke on some platforms, due to the use of the assembly code. Even with our best effort to not break assembly code in our design (e.g. automatic wrapper generation), it is still very hard (if not impossible) to not break, as the assembly code is too sensitive to details like stack layout. (I guess one way not to break is in the compiler to hard-code golang.org/x/sys import path to use old ABI, but that doesn't sound right.) Do we want to back off register ABI, then? Again, as the original post says, I think the problem is the difficulty of co-evolving x/sys and Go toolchain+std, or updating them at ~same time. And the solution discussed in the original post, and/or #55092, #55090, is probably a good direction. Thanks. |
|
Just to be clear, the error message in the title is produced when a package contains a I'd be okay changing the error check to be tied to the go.mod file (only an error for ≥1.18), and also backporting that change to currently supported versions, if that's the direction that makes most sense here. |
|
@gopherbot Please backport to Go 1.18 and 1.19. This was is regression in the Go compiler, which broke building x/sys for GOOS=darwin prior to https://go.dev/cl/274573 (golang/sys@69691e4). |
|
Backport issue(s) opened: #56556 (for 1.18), #56557 (for 1.19). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases. |
|
Change https://go.dev/cl/447755 mentions this issue: |
|
Change https://go.dev/cl/447815 mentions this issue: |
|
Change https://go.dev/cl/447816 mentions this issue: |
…in -lang=go1.17 and older Prior to Go 1.18, ineffectual //go:linkname directives (i.e., directives referring to an undeclared name, or to a declared type or constant) were treated as noops. In Go 1.18, we changed this into a compiler error to mitigate accidental misuse. However, the x/sys repo contained ineffectual //go:linkname directives up until go.dev/cl/274573, which has caused a lot of user confusion. It seems a bit late to worry about now, but to at least prevent further user pain, this CL changes the error message to only apply to modules using "go 1.18" or newer. (The x/sys repo declared "go 1.12" at the time go.dev/cl/274573 was submitted.) For #55889. Fixes #56556. Change-Id: Id762fff96fd13ba0f1e696929a9e276dfcba2620 Reviewed-on: https://go-review.googlesource.com/c/go/+/447755 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-on: https://go-review.googlesource.com/c/go/+/447815
…in -lang=go1.17 and older Prior to Go 1.18, ineffectual //go:linkname directives (i.e., directives referring to an undeclared name, or to a declared type or constant) were treated as noops. In Go 1.18, we changed this into a compiler error to mitigate accidental misuse. However, the x/sys repo contained ineffectual //go:linkname directives up until go.dev/cl/274573, which has caused a lot of user confusion. It seems a bit late to worry about now, but to at least prevent further user pain, this CL changes the error message to only apply to modules using "go 1.18" or newer. (The x/sys repo declared "go 1.12" at the time go.dev/cl/274573 was submitted.) For #55889. Fixes #56557. Change-Id: Id762fff96fd13ba0f1e696929a9e276dfcba2620 Reviewed-on: https://go-review.googlesource.com/c/go/+/447755 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-on: https://go-review.googlesource.com/c/go/+/447816
…in -lang=go1.17 and older Prior to Go 1.18, ineffectual //go:linkname directives (i.e., directives referring to an undeclared name, or to a declared type or constant) were treated as noops. In Go 1.18, we changed this into a compiler error to mitigate accidental misuse. However, the x/sys repo contained ineffectual //go:linkname directives up until go.dev/cl/274573, which has caused a lot of user confusion. It seems a bit late to worry about now, but to at least prevent further user pain, this CL changes the error message to only apply to modules using "go 1.18" or newer. (The x/sys repo declared "go 1.12" at the time go.dev/cl/274573 was submitted.) For golang#55889. Fixes golang#56557. Change-Id: Id762fff96fd13ba0f1e696929a9e276dfcba2620 Reviewed-on: https://go-review.googlesource.com/c/go/+/447755 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-on: https://go-review.googlesource.com/c/go/+/447816
Using an
x/sysversion that is too old for a Go version may result in build errors, and that has clearly confused at least hundreds of Go users in the past year or so:Given that
x/sysuses//go:linknamedirectives into unexported Go APIs, it is fairly tightly bound to a set of Go versions. If the Go version is too old or too new, it's entirely possible that the build may fail like the example above onGOOS=darwin.If the
x/sysversion is too old, the solution is pretty easy:go get golang.org/x/systo update the dependency. That is not obvious given the error message, though. And I bet that a good number of those end users are only indirectly depending onx/sys, so they might not even know what the library does or why it's being built.Similarly, if one tries to use a recent version of
x/syswith an ancient version of Go, I imagine the build may also fail in weird and confusing ways, though this side of the coin doesn't seem to be as common of a problem.I think we should try to make these errors more intuitive. As a strawman idea, I suggest that we force the current master version of
x/systo fail to build on Go 1.17 and older, as well as Go 1.21 and newer, via build tags such as:The string literals would cause a parse error which would show the string to the user, as a way of customizing the error message:
We would not error on Go 1.20, as that is the current tip/master version, and we are continuously supporting and testing it. Erroring on Go 1.21 today is perhaps unnecessary, but it's the only way to prepare the current master versions for a future when Go 1.21 is widely used and may not work with today's versions of
x/sys. When Go 1.20 is released and we start working towards 1.21, the build tags inx/sysmaster would be advanced by one version.A potential alternative would be a
toolchainline ingo.mod, per #55092. However, that might only allow us to set a minimum Go toolchain version, and presumably not a maximum. It's also not yet implemented, nor is it understood by older Go versions. We definitely want our solution to make Go 1.16 fail to build the latestx/sysmaster version, for example.cc @golang/runtime @ianlancetaylor @bradfitz @tklauser per https://dev.golang.org/owners
The text was updated successfully, but these errors were encountered: