search

google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.27k stars 204 forks source link

starlark: disable int32 optimization on openbsd #383

Closed adonovan closed 2 years ago

adonovan commented 2 years ago

This PR disables the int32 optimization on openbsd because its default ulimit is only about 1GB.

Fixes #382

[Note: comments by reviewers refer to PR's description of original approach, which was abandoned, as explained in the code comment.]

jayconrod commented 2 years ago

I like this idea; V8 did something very similar. The test panic looks hard to overcome though.

(Incidentally, I've been told we'll need to switch away from Travis CI soon. I'll start looking at that soon, but let me know if you have opinions on how that should work).

falsifian commented 2 years ago

Would making intImpl a uintptr instead of an unsafe.Pointer make the test happy?

adonovan commented 2 years ago

The test panic looks hard to overcome though.

Yeah; I wondered whether the runtime would care about or get confused by manufactured pointers. (Historically its conservative GC allowed such things, but it has been getting stricter every year.) This crash clearly answers the question in the negative. This particular error means that a pointer contained a value that points into the zero page of memory, which is reserved as a nomansland: https://github.com/golang/go/blob/master/src/runtime/stack.go#L614

But more generally, it appears that user programs are not supposed to put bad pointers in the heap. The comments here make clear that pointers into mmap'd regions are ok, but pointers into non-stack spans are forbidden: https://github.com/golang/go/blob/master/src/runtime/mbitmap.go#L387-L411

Would making intImpl a uintptr instead of an unsafe.Pointer make the test happy?

No. The union must appear to the GC to be a pointer so that it keeps the big.Int variables live. In the old scheme, all its values were valid pointers, and this change broke that.

Reverting to the conservative strategy of disabling the optimization on OpenBSD.

adonovan commented 2 years ago

I updated the PR description to reflect the new implementation, which of course renders all the subsequent comments meaningless. I won't be doing that again.

adonovan commented 2 years ago

(Incidentally, I've been told we'll need to switch away from Travis CI soon. I'll start looking at that soon, but let me know if you have opinions on how that should work).

Sorry, I haven't even thought about it, though I have been vaguely aware of it looming. (Presumably it is this: https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing.) Is Google paying the TravisCI bill one of the options? :)

jayconrod commented 2 years ago

Paying the bill would probably be fine, but there's more I can't say publicly.