The Wayback Machine - https://web.archive.org/web/20210920024714/https://github.com/gopherjs/gopherjs/pull/1043
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

Go 1.17 support #1043

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:

  • Merge #1044 and rebase.
  • Set runtime.Version() dynamically at compile time. This changed from 1.16 to 1.17 upstream, and is now handled by the linker in 1.17, which we don't have, so we need to invent a new solution.
  • Fix or properly document tests in crypto/ed25519 package.
  • Update the hash/maphash implementation to work without pointer arithmetic. Related: golang/go#47342 (comment)
  • Update math/big to avoid constant overflow.
  • Fix reflect (see #1043 (comment) for details)
  • Make new conversion rules from slice to array pointer work
  • Validate that the new //go:build lines are understood properly
  • fmt examples are failing for 1.17
  • module-based building not working
  • Fix golang/go#44830
  • fixedbugs/issue23017.go causes a compiler panic
  • Test against latest released 1.17.x version.
  • Update README and documentation with the new version as applicable.

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 Jul 23, 2021
@flimzy flimzy force-pushed the wip-go1.17 branch 4 times, most recently from f8407a0 to f6f0eef Jul 29, 2021
@flimzy
Copy link
Member Author

@flimzy 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 Jul 29, 2021
Copy link
Member

@nevkontakte nevkontakte left a comment

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 :)

compiler/natives/src/crypto/ed25519/ed25519vectors_test.go Outdated Show resolved Hide resolved
import "testing"

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

@nevkontakte nevkontakte Aug 4, 2021

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 Aug 25, 2021
And slightly refactor CheckGoVersion to require less manual intervention for
each version change.
nevkontakte and others added 27 commits Sep 12, 2021
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 Sep 18, 2021
@flimzy flimzy merged commit daae650 into master Sep 18, 2021
3 checks passed
@flimzy flimzy deleted the wip-go1.17 branch Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.