-
Notifications
You must be signed in to change notification settings - Fork 18k
runtime: do not crash in lastcontinuehandler when running as DLL #32574
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: do not crash in lastcontinuehandler when running as DLL #32574
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Note: I need help to setup a test covering this, as it won't be trivial:
For now, I only have a manual test-case for this, but I assume this needs to be part of the Go automated test suite. |
I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
This PR (HEAD: 8b4e9c1) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/181839 to see it. Tip: You can toggle comments from me using the |
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Alex Brainman: Patch Set 1: (2 comments) Sorry, but I don't see why this change is needed. Please, create an issue https://golang.org/issue/new that shows how to reproduce this problem. And change like this needs a test. Alex Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Simon Ferquel: Patch Set 1:
I will post more details in a GitHub issue. It only occurs when using cgo, because it happens when you are doing interop with other languages that might introduce their continue handlers after the go runtime.
If you don't attach the .net debugger, no breakpoint exception is injected, and everything runs fine. Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
8b4e9c1
to
cf0b236
Compare
This PR (HEAD: cf0b236) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/181839 to see it. Tip: You can toggle comments from me using the |
Message from Simon Ferquel: Patch Set 2:
I added a test in a separated commit exhibiting the behavior, and rebased this PR on it. If you run the introduced test on a Windows machine, you'll see the test panic with:
Applying the second commit fixes the test, but there is a test relying on the broken behavior somewhere else:
Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Simon Ferquel: Patch Set 2: I wonder if it would make sense to allow only a subset of exception types to be propagated to another continue handler. I guess BREAKPOINT_EXCEPTION makes sense for CGO enabled programs, so I could modify the PR to return CONTINUE_SEARCH only if the exception type is BREAKPOINT_EXCEPTION and it happened in non-go code. Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Simon Ferquel: Patch Set 2: Also, I can think of another approach to fix the issue: the main reason for this bug, is that the runtime wrongly assumes that lastcontinuehandler is always the last handler in the continue handler vector. When using non-go debuggers, that might be wrong, but we can correct this: Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Alex Brainman: Patch Set 2: (3 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Simon Ferquel: Patch Set 2: (2 comments) Answered the comments in code, and also added a repro with a walkthrough video here: #32648 I understand that this change of behavior is pretty big, and I would be happy to prototype other approaches to fix the problem with your guidance. It is pretty frustrating because c-shared works fine on Windows, except you can't debug anything but go code (and until this is solved, I don't want to leverage it in production workloads) Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Alex Brainman: Patch Set 2: (3 comments) I am travelling for the next month, so I might not have computers and Internet at times. And we cannot submit this change until after go 1.13 is released, and Go tree will open again (some time in August). So, hopefully, we will be ready by then. Thank you. Alex Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
cf0b236
to
efb9f1b
Compare
This PR (HEAD: efb9f1b) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/181839 to see it. Tip: You can toggle comments from me using the |
efb9f1b
to
017a542
Compare
This PR (HEAD: 017a542) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/181839 to see it. Tip: You can toggle comments from me using the |
017a542
to
78b33c1
Compare
Message from Simon Ferquel: Patch Set 4: (4 comments) I've basically rewritten the whole test as an integration test more resembling to a real world scenario. Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
This PR (HEAD: 78b33c1) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/181839 to see it. Tip: You can toggle comments from me using the |
Message from Alex Brainman: Patch Set 5: Run-TryBot+1 (13 comments) Some comments, thank you very much. Alex Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Gobot Gobot: Patch Set 5: TryBots beginning. Status page: https://farmer.golang.org/try?commit=cbe47382 Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Gobot Gobot: Patch Set 5: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
78b33c1
to
e0fd37e
Compare
This PR (HEAD: e0fd37e) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/181839 to see it. Tip: You can toggle comments from me using the |
Message from Simon Ferquel: Patch Set 5: (13 comments) Just updated the PR with changes requested by Alex Brainman. Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
This test reproduces a golang dll run from within a C program with a debugger attached. It emulates the behavior of a debugger handling a breakpoint in C code called back by GO code. Signed-off-by: Simon Ferquel <[email protected]>
If Go DLL is used by external C program, and lastcontinuehandler is reached, lastcontinuehandler will crash the process it is running in. But it should not be up to Go runtime to decide if process to be crashed or not - it should be up to C runtime. This CL adjusts lastcontinuehandler to not to crash when running as DLL. Fixes golang#32648.
e0fd37e
to
a5f71ea
Compare
This PR (HEAD: a5f71ea) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/181839 to see it. Tip: You can toggle comments from me using the |
Message from Simon Ferquel: Patch Set 7:
Also just rebased for having an up-to-date base Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Simon Ferquel: Patch Set 8: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Simon Ferquel: Patch Set 10: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Simon Ferquel: Patch Set 11: (14 comments) Struggling with the commit message. Otherwise I think it is pretty much done. Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Alex Brainman: Patch Set 11: (1 comment) I am still reviewing your code, but decided to reply to your commit message question first. So you can update CL without waiting for me to finish review. Thank you. Alex Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Agniva De Sarker: Patch Set 11: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Alex Brainman: Patch Set 11: Run-TryBot+1 (2 comments) Just one comment. And I am waiting to see your final commit message. Thank you. Alex Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Gobot Gobot: Patch Set 11: TryBots beginning. Status page: https://farmer.golang.org/try?commit=8635b7ea Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Gobot Gobot: Patch Set 11: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Signed-off-by: Simon Ferquel <[email protected]>
This PR (HEAD: dbdffcb) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/181839 to see it. Tip: You can toggle comments from me using the |
Message from Simon Ferquel: Patch Set 13: (18 comments) New patch upcoming with typo fix Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Alex Brainman: Patch Set 14: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Gobot Gobot: Patch Set 14: TryBots beginning. Status page: https://farmer.golang.org/try?commit=7d5d7049 Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Gobot Gobot: Patch Set 14: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
Message from Alex Brainman: Patch Set 14: This CL LGTM. Thank you very much. I will submit this CL once Go tree opens for submission as per https://groups.google.com/d/msg/golang-dev/UQOSRilopzw/Xtth3MQmAgAJ Feel free to ping me here, so I don't forget. Alex Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
If Go DLL is used by external C program, and lastcontinuehandler is reached, lastcontinuehandler will crash the process it is running in. But it should not be up to Go runtime to decide if process to be crashed or not - it should be up to C runtime. This CL adjusts lastcontinuehandler to not to crash when running as DLL. Fixes #32648. Change-Id: Ia455e69b8dde2a6f42f06b90e8af4aa322ca269a GitHub-Last-Rev: dbdffcb GitHub-Pull-Request: #32574 Reviewed-on: https://go-review.googlesource.com/c/go/+/181839 Run-TryBot: Alex Brainman <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Alex Brainman <[email protected]>
Message from Alex Brainman: Patch Set 14: Code-Review+2 Please don’t reply on this GitHub thread. Visit golang.org/cl/181839. |
This PR is being closed because golang.org/cl/181839 has been merged. |
If Go DLL is used by external C program, and lastcontinuehandler is reached, lastcontinuehandler will crash the process it is running in. But it should not be up to Go runtime to decide if process to be crashed or not - it should be up to C runtime. This CL adjusts lastcontinuehandler to not to crash when running as DLL. Fixes golang#32648. Change-Id: Ia455e69b8dde2a6f42f06b90e8af4aa322ca269a GitHub-Last-Rev: dbdffcb GitHub-Pull-Request: golang#32574 Reviewed-on: https://go-review.googlesource.com/c/go/+/181839 Run-TryBot: Alex Brainman <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Alex Brainman <[email protected]>
If Go DLL is used by external C program, and lastcontinuehandler is reached, lastcontinuehandler will crash the process it is running in. But it should not be up to Go runtime to decide if process to be crashed or not - it should be up to C runtime. This CL adjusts lastcontinuehandler to not to crash when running as DLL. Fixes golang#32648. Change-Id: Ia455e69b8dde2a6f42f06b90e8af4aa322ca269a GitHub-Last-Rev: dbdffcb GitHub-Pull-Request: golang#32574 Reviewed-on: https://go-review.googlesource.com/c/go/+/181839 Run-TryBot: Alex Brainman <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Alex Brainman <[email protected]>
If Go DLL is used by external C program, and lastcontinuehandler
is reached, lastcontinuehandler will crash the process it is
running in.
But it should not be up to Go runtime to decide if process to be
crashed or not - it should be up to C runtime. This CL adjusts
lastcontinuehandler to not to crash when running as DLL.
Fixes #32648.