Skip to content

Go 1.17 support #1043

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

Merged
merged 50 commits into from
Sep 18, 2021
Merged

Go 1.17 support #1043

merged 50 commits into from
Sep 18, 2021

Conversation

flimzy
Copy link
Member

@flimzy flimzy commented Jul 22, 2021

Go 1.17 is on the way!

This PR tracks progress toward adding Go 1.17 support to GopherJS.

TODO:

Out of scope for this PR (but may be considered as part of future work):

@flimzy flimzy force-pushed the wip-go1.17 branch 13 times, most recently from e857b8a to f6f0eef Compare July 23, 2021 07:29
@flimzy flimzy force-pushed the wip-go1.17 branch 4 times, most recently from f8407a0 to f6f0eef Compare July 29, 2021 09:41
@flimzy
Copy link
Member Author

flimzy commented Jul 29, 2021

Recent changes in the reflect package have broken GopherJS. Still needs investigation.

To reproduce:

$ gopherjs test crypto/ed25519/internal/edwards25519/field --run=TestSetBytesRoundTrip
--- FAIL: TestSetBytesRoundTrip (0.01s)

/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:1450
          throw new Error(msg);
                ^
Error: reflect: New of type that may not be allocated in heap (possibly undefined cgo C type)
    at $callDeferred (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:1450:17)
    at $panic (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:1493:3)
    at $b (/testing/testing.go:1203:5)
    at $b (/testing/testing.go:1206:5)
    at $callDeferred (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:1462:23)
    at $panic (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:1493:3)
    at Object.New (/reflect/value.go:2745:4)
    at sizedValue (/testing/quick/quick.go:71:3)
    at Value (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:42629:10)
    at arbitraryValues (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:43029:11)
    at Object.Check (/testing/quick/quick.go:285:4)
    at TestSetBytesRoundTrip (/crypto/ed25519/internal/edwards25519/field/fe_test.go:159:6)
    at tRunner (/testing/testing.go:1253:3)
    at $goroutine (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:1513:19)
    at $runScheduled (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:1553:7)
    at $schedule (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:1569:5)
    at $go (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:1545:3)
    at Object.<anonymous> (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:51764:1)
    at Object.<anonymous> (/home/jonhall/go/src/github.com/gopherjs/gopherjs/test.2657973286:51767:4)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47
FAIL    crypto/ed25519/internal/edwards25519/field      0.259s

This can be reproduces with a simple test case:

func main() {
	x := []int{}
	t := reflect.TypeOf(x)
	_ = reflect.New(t)
}

@flimzy flimzy force-pushed the wip-go1.17 branch 5 times, most recently from 4c3ccf5 to d767a4d Compare July 29, 2021 14:36
Copy link
Member

@nevkontakte nevkontakte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! 👍 I still haven't found time to look into the slice-to-array conversion, but I intend to do so this weekend, unless you get there first :)

import "testing"

func TestScalarMultDistributesOverAdd(t *testing.T) {
t.Skip("slow") // Times out, takes ~13 minutes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR per se, but thinking out loud. One reason I heard for people using GopherJS was access to the crypto library, and our crypto in incredibly slow. I did a little profiling and it seems to all boil down to math/big being really slow with our implementation of slices. I couldn't think of an easy way to improve the situation, but these tests shall serve as additional motivation to try and optimize it further.

flimzy added 3 commits August 25, 2021 21:31
And slightly refactor CheckGoVersion to require less manual intervention for
each version change.
nevkontakte and others added 27 commits September 12, 2021 19:20
Array pointers are the only kind of pointer considered a "wrapped", so
they need to be handled appropriately. At runtime an array pointer is
represented by a reference to the JavaScript native array object, so we
only need to copy the reference and couple it with the desired Go type.
It will be boxed into the new Go type if/when the compiler needs it down
the road.
Upstream uses a conversion to unsafeheader.Slice, which isn't supported
by GopherJS. Using Value.Len() is preferrable, since we already override
it to provide compatibility. If/when
golang/go#48346 gets into the stable release,
this patch will no longer be necessary.
Complete support for slice-to-array conversion.
Since JavaScript runtime is single-threaded, there isn't much special
logic to be done. However, it is important that all these functions
never call anything potentially blocking to make sure the goroutine is
not interrupted in a middle of an atomic operation.
sync/atomic: Implement new Swap and CompareAndSwap methods of Value.
This pulls in a few select functions from Go 1.16, which were optimized and thus made unsafe in Go 1.17.

golang/go#47342 should render this change obsolete if/when implemented.
Get `hash/maphash` to work again
This package seems to be related to the new register-based ABI in Go
1.17, which is completely irrelevant for GopherJS.
The fix is two-fold:

 1. `disableSplice` is initialized with `(*bool)(nil)` to work around
    #1060.
 2. Skipped `TestSplicePipePool`, which relies in runtime features
    GopherJS doesn't support.
Fix standard library test failures in `internal` packages.
In the following example type T is *types.Named, not *types.Pointer, but
the underlying type is:

```go
type T *struct{ y int }
var q T = &struct{ y int }{}
q.y = 7
```

The panic was detected by gorepo test `fixedbugs/issue23017.go`.
GopherJS breaks lhs expression evaluation order defined by the spec.
Unfortunately, it's not easy to fix and would likely generate a lot more
code for multi-assignments.

An efficient fix would require writing some sort of analysis to
determine if assignments are likely to influence each other, and at the
moment I can't think of how such analysis would work. Without it we
would have to create a whole bunch of extra temporary variables for any
multi-assignment, which will likely lead to a significant increase in
the artifact size. Considering nobody reported this issue so far, I'm
inclined to punt on this issue for now.

#1063 tracks this bug.
Resolve the two remaining gorepo test failures.
This change improves Go version detection for the provided GOROOT by
falling back to `go version` command output if VERSION file doesn't
exist (fixes #1018).

If GOROOT-based version detection failed, it falls back further to
the Go version from GopherJS release or a minor version in "go1.x"
format.

The detected value is then injected by the compiler into the generated
JS file and picked up by the `runtime` package.
runtime: Version() returns Go version provided by the compiler.
This isn't really sufficient to test the implementation correctness
perfectly, but it is a sufficient smoke test for GopherJS, and it cuts
the test runtime substantially.
Un-skip a few tests from crypto libraries.
@flimzy flimzy marked this pull request as ready for review September 18, 2021 18:29
@flimzy flimzy merged commit daae650 into master Sep 18, 2021
@flimzy flimzy deleted the wip-go1.17 branch September 18, 2021 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants