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

runtime: nowritebarrierrec doesn't cross systemstack #22384

Closed
aclements opened this issue Oct 22, 2017 · 10 comments

Comments

Projects
None yet
3 participants
@aclements
Copy link
Member

commented Oct 22, 2017

When a function is marked go:nowritebarrierrec in the runtime, the compiler recursively disallows write barriers in functions called from it. However, this static check doesn't know that systemstack calls its argument. Since this is quite common in the runtime, we're actually failing to catch several write barriers in prohibited places (I found this the hard way when I made the write barrier a little more picky). I believe all of these are technically benign, but for subtle reasons. I don't like relying on subtle reasons.

We should fix this analysis to understand systemstack and fix the bad write barriers in the runtime. I have most of this done already, but some of the write barriers are proving difficult to remove, so this issue is to track this work.

@aclements aclements added this to the Go1.10 milestone Oct 22, 2017

@aclements aclements self-assigned this Oct 22, 2017

@aclements

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 2017

/cc @RLH

@gopherbot

This comment has been minimized.

Copy link

commented Oct 23, 2017

Change https://golang.org/cl/72772 mentions this issue: runtime: allow write barriers in gchelper

@gopherbot

This comment has been minimized.

Copy link

commented Oct 23, 2017

Change https://golang.org/cl/72770 mentions this issue: runtime: allow write barriers in startpanic_m

@gopherbot

This comment has been minimized.

Copy link

commented Oct 23, 2017

Change https://golang.org/cl/72771 mentions this issue: runtime: eliminate write barriers from persistentalloc

@gopherbot

This comment has been minimized.

Copy link

commented Oct 23, 2017

Change https://golang.org/cl/72773 mentions this issue: cmd/compile: check nowritebarrierrec across systemstack

@mdempsky

This comment has been minimized.

Copy link
Member

commented Oct 23, 2017

@aclements Do you have any thoughts on moving the interprocedural checks to a go/types-based checker? I'm wondering if that would be easier to maintain since the go/types API is better documented and also more stable.

The intraprocedural stuff seems easy enough to keep in the compiler (e.g., nothing escaping to the heap, disallowing write barriers in nowritebarrier functions), but the interprocedural stuff seems to cause us trouble.

@aclements

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2017

@mdempsky, I'm not sure how that would work since it needs to know exactly where the compiler inserted write barriers, which go/types wouldn't know.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Oct 23, 2017

@aclements Ah, right, I was thinking for a second that we explicitly mark "//go:nowritebarrier" on all functions that can't use write barriers, but I forgot the entire point of "//go:nowritebarrierrec" is to avoid that.

I suppose the go/types checker could use compile -m (or whichever flag) output to identify which functions have write barriers, and feed that into its analysis.

@aclements

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2017

I thought of another reason doing this in go/types would be difficult: we want to see the implicit runtime calls that operations like copy, panic, etc. get lowered to.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Oct 26, 2017

Yeah, I'm not thinking an exclusively go/types-based checker, but one that runs "compile -m" to first determine which functions contain write barriers (or whatever other interesting properties), and then go/types is used for understanding and validating the call graph.

gopherbot pushed a commit that referenced this issue Oct 29, 2017

runtime: allow write barriers in startpanic_m
We're about to start tracking nowritebarrierrec through systemstack
calls, which will reveal write barriers in startpanic_m prohibited by
various callers.

We actually can allow write barriers here because the write barrier is
a no-op when we're panicking. Let the compiler know.

Updates #22384.
For #22460.

Change-Id: Ifb3a38d3dd9a4125c278c3680f8648f987a5b0b8
Reviewed-on: https://go-review.googlesource.com/72770
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>

gopherbot pushed a commit that referenced this issue Oct 29, 2017

runtime: eliminate write barriers from persistentalloc
We're about to start tracking nowritebarrierrec through systemstack
calls, which will reveal write barriers in persistentalloc prohibited
by various callers.

The pointers manipulated by persistentalloc are always to off-heap
memory, so this removes these write barriers statically by introducing
a new go:notinheap type to represent generic off-heap memory.

Updates #22384.
For #22460.

Change-Id: Id449d9ebf145b14d55476a833e7f076b0d261d57
Reviewed-on: https://go-review.googlesource.com/72771
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>

gopherbot pushed a commit that referenced this issue Oct 29, 2017

runtime: allow write barriers in gchelper
We're about to start tracking nowritebarrierrec through systemstack
calls, which detects that we're calling markroot (which has write
barriers) from gchelper, which is called from the scheduler during STW
apparently without a P.

But it turns out that func helpgc, which wakes up blocked Ms to run
gchelper, installs a P for gchelper to use. This means there *is* a P
when gchelper runs, so it is allowed to have write barriers. Tell the
compiler this by marking gchelper go:yeswritebarrierrec. Also,
document the call to gchelper so I don't have to spend another half a
day puzzling over how on earth this could possibly work before
discovering the spooky action-at-a-distance in helpgc.

Updates #22384.
For #22460.

Change-Id: I7394c9b4871745575f87a2d4fbbc5b8e54d669f7
Reviewed-on: https://go-review.googlesource.com/72772
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>

@gopherbot gopherbot closed this in 5a4b6bc Oct 29, 2017

@golang golang locked and limited conversation to collaborators Oct 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.