search

go-interpreter / ssainterp

A Golang interpreter, built upon golang.org/x/tools/go/ssa/interp
MIT License
58 stars 9 forks source link

fix externals for go1.7. #4

Closed glycerine closed 7 years ago

glycerine commented 7 years ago

@elliott5 please have a look when you can.

This adds sync.runtime_notifyListCheck and runtime.NumGoroutine stubs.

I tried calling the native runtime.NumGoroutine() in the runtime.NumGoroutine() thunk, but the goprint.go test apparently expects only one goroutine ever. Yep, crazy but true. Likely vestigial from earlier go version. Not sure how to best handle this. Perhaps the goprint.go test should just be skipped. By the way, is it legit to call the native runtime.NumGoroutine() directly like that? (see commented out line https://github.com/glycerine/ssainterp/commit/ab205b607431f5d3de898d6f31900dd5e34f91c3#diff-0cb90af343598265d6db5f9e8e4db65cR183).

At any rate, on my go1.7 osx, all the tests now pass.

Things to review/check before merging:

a) Wasn't sure if I was adding the //Elliott annotations at the rights points. b) Wasn't sure if I should or could call the actual runtime.NumGoroutines() in the thunk. c) Wasn't sure if the sync.runtime_notifyListCheck() thunk could or should try to do the same check that the native routine does. Is some critical consistency check being skipped now?

Fixes #3

sbinet commented 7 years ago

looking at "upstream", it seems a better fix is: https://github.com/golang/tools/commit/56005b4126fe4c3f7454f0a4c63a31a28491cb92

glycerine commented 7 years ago

@sbinet agreed and incorporated. Thanks!

elliott5 commented 7 years ago

@glycerine surely your additions should be "// Jason" or "// glycerine" rather than "// elliott" ;)

...we probably need some better way to mark what diverges from the "upstream"...