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: add "maymorestack" hook for testing stack growth prologues #48297

Closed
aclements opened this issue Sep 9, 2021 · 16 comments
Closed

runtime: add "maymorestack" hook for testing stack growth prologues #48297

aclements opened this issue Sep 9, 2021 · 16 comments

Comments

@aclements
Copy link
Member

@aclements aclements commented Sep 9, 2021

I plan to add a debugging hook for internal use that will be invoked in the prologue of any function that may call morestack (the stack growth function). This is inspired by the preemption bugs that led to #47302, #47304, and #47441.

In general, testing around morestack is difficult because almost all Go functions can potentially call morestack, but very few do at run time, and which ones do are highly unpredictable. As a result, if a particular function is incorrectly sensitive to whether morestack gets invoked (either to grow the stack or to preempt), it's highly unlikely that general testing will actually trigger the morestack path in that function.

The hook function will be called unconditionally before the stack bound check in any function that could call morestack. This particular placement means the hook can inject preemption or stack growth by simply setting the stack bound to a poisoned value and returning, since the stack bound check will then fail and enter morestack.

This hook is also useful for more precise lock order checking because it lets us model functions that "may" acquire any of the locks acquired by morestack. These are lock edges we only see in the rare cases where morestack is actually called, but lie latent in any function that could call it.

@aclements aclements added this to the Unplanned milestone Sep 9, 2021
@aclements aclements self-assigned this Sep 9, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 9, 2021

The hook function will be called unconditionally before the stack bound check in any function that could call morestack.

How do you plan to make this happen? Presumably we don't want to always call the hook in every function. Perhaps I misunderstand.

@aclements
Copy link
Member Author

@aclements aclements commented Sep 9, 2021

I plan to always call the hook in every function. The hook call will only be added if the debug option is supplied, so the performance isn't really an issue. And, in fact, I've already implemented this and it has shockingly little performance impact. Unless you were thinking of some other problem?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 9, 2021

How is the debug option supplied?

@aclements
Copy link
Member Author

@aclements aclements commented Sep 9, 2021

In my current prototype, the compiler takes it as a -d=maymorestack= argument and the assembler takes it as -maymorestack= (because it doesn't have an equivalent of -d). So, e.g., you can supply the preemption-testing hook as

GOFLAGS="-gcflags=runtime=-d=maymorestack=runtime.mayMoreStackPreempt -asmflags=runtime=-maymorestack=runtime.mayMoreStackPreempt"

But I'm happy to change this.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 9, 2021

That seems fine. Thanks. I just didn't find the original description very clear. We have various different kinds of debugging hooks (e.g., GODEBUG).

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 30, 2021

Change https://golang.org/cl/359956 mentions this issue: cmd/compile,cmd/internal/objabi: move -d flag parser to objabi

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 30, 2021

Change https://golang.org/cl/359795 mentions this issue: cmd/{asm,compile,internal/obj}: add "maymorestack" support

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 30, 2021

Change https://golang.org/cl/359797 mentions this issue: runtime: add a maymorestack hook that moves the stack

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 30, 2021

Change https://golang.org/cl/359796 mentions this issue: runtime: add always-preempt maymorestack hook

gopherbot pushed a commit that referenced this issue Nov 5, 2021
This moves and slightly generalizes the -d debug flag parser from
cmd/compile/internal/base to cmd/internal/objabi so that we can use
the same debug flag syntax in other tools.

This makes a few minor tweaks to implementation details. The flag
itself is now just a flag.Value that gets constructed explicitly,
rather than at init time, and we've cleaned up the implementation a
little (e.g., using a map instead of a linear search of a slice). The
help text is now automatically alphabetized. Rather than describing
the values of some flags in the help text footer, we simply include it
in the flags' help text and make sure multi-line help text renders
sensibly.

For #48297.

Change-Id: Id373ee3b767e456be483fb28c110d025149be532
Reviewed-on: https://go-review.googlesource.com/c/go/+/359956
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Nov 5, 2021
This adds a maymorestack hook that forces a preemption at every
possible cooperative preemption point. This would have helped us catch
several recent preemption-related bugs earlier, including #47302,
 #47304, and #47441.

For #48297.

Change-Id: Ib82c973589c8a7223900e1842913b8591938fb9f
Reviewed-on: https://go-review.googlesource.com/c/go/+/359796
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: David Chase <drchase@google.com>
gopherbot pushed a commit that referenced this issue Nov 5, 2021
This adds a maymorestack hook that moves the stack at every
cooperative preemption point.

For #48297.

Change-Id: Ic15f9bcbc163345e6422586302d57fda4744caec
Reviewed-on: https://go-review.googlesource.com/c/go/+/359797
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 5, 2021

Change https://golang.org/cl/361212 mentions this issue: cmd/dist: add maymorestack tests

@go101
Copy link

@go101 go101 commented Nov 5, 2021

How to use use this option in go commands?
I didn't get the answer by reading the code.
-gcflags=-d=maymorestack?
Or another way?

@aclements
Copy link
Member Author

@aclements aclements commented Nov 5, 2021

@go101, see the comments added in CL 359796. Your usage is close, but you have to also specify which maymorestack hook function to use: -gcflags=-d=maymorestack=X where X is a function that can be used as a maymorestack hook, of which there are now two in runtime/debug.go.

@go101
Copy link

@go101 go101 commented Nov 5, 2021

Thanks. Got it and make it work for a custom function. Cool feature!

@go101
Copy link

@go101 go101 commented Nov 5, 2021

Er, it looks I misunderstood this option. I thought the hook will be called when stacks grow. In fact, it is not.

@aclements
Copy link
Member Author

@aclements aclements commented Nov 5, 2021

@go101, right, it's called whenever they might grow. It's really intended as a way for us to amplify the rare things that happen when that path is (fairly unpredictably) taken. Hence the hooks to "always preempt" and "always move the stack". I'd also like to hook this up with the static lock rank checker so we can mark locks that could be taken by morestack if the conditions were right, but haven't gotten to that.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 25, 2022

Change https://go.dev/cl/402157 mentions this issue: runtime: fix stack-move sensitivity in some tests

gopherbot pushed a commit that referenced this issue Apr 25, 2022
There are a few tests of the scheduler run queue API that allocate a
local []g and test using those G's. However, the run queue API
frequently converts between *g and guintptr, which is safe for "real"
Gs because they're heap-allocated and hence don't move, but if these
tests get a stack movement while holding one of these local *g's as a
guintptr, it won't get updated and the test will fail.

Updates #48297.

Change-Id: Ifd424147ce1a1b53732ff0cf55a81df1a9beeb3b
Reviewed-on: https://go-review.googlesource.com/c/go/+/402157
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Apr 25, 2022
These tests run the runtime, reflect, and sync package tests with the
two maymorestack hooks we have.

These tests only run on the longtest builders (or with
GO_TEST_SHORT=false) because we're running the runtime test two
additional times and the mayMoreStackMove hook makes it about twice as
slow (~230 seconds).

To run just these tests by hand, do

  GO_TEST_SHORT=false go tool dist test -run mayMoreStack

Updates #48297.

This detected #49354, which was found as a flake on the dashboard, but
was reliably reproducible with these tests; and #49395.

Change-Id: If785a8b8d6e1b9ad4d2ae67493b54055ab6cbc85
Reviewed-on: https://go-review.googlesource.com/c/go/+/361212
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Austin Clements <austin@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants