[go] runtime,runtime/cgo: port to Go part of the runtime init lock
Quim Muntal (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Cherry Mui
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 9 comments![]( "Open in Gerrit")
arg->context = ++contextCount;
Let's keep our C code roughly Go like.
contextCount++;
arg->context = contextCount
fmt.Fprintf(fm, "size_t _cgo_wait_runtime_init_done(void) { return 0; }\n")We should change _cgo_wait_runtime_init_done to not return a value.
uintptr_ts/uintptr_t/void/
and drop the "return 0;" at the end.
func cgocallbackg(fn, frame unsafe.Pointer, ctxt uintptr) {I think ctxt will always be zero. We should add a TODO to remove it (which will unfortunately mean editing assembly code for each platform).
if !mainInitDone.Load() {Update for CL 746581.
if ctxt == 0 && cgoContext != nil {It seems to me that ctxt will always be 0 at this point. When would it be otherwise?
// Decrease the length of the slice by one, safely.Since we could get a SIGPROF at any time, we need to shrink the slice _before_ we release the context argument.
Related details
- Cherry Mui
- Quim Muntal
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Cherry Mui
- Quim Muntal
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal voted and added 9 comments![]( "Open in Gerrit")
Votes added by Quim Muntal
| Commit-Queue | +1 |
9 comments
Let's keep our C code roughly Go like.
contextCount++;
arg->context = contextCount
Done
fmt.Fprintf(fm, "size_t _cgo_wait_runtime_init_done(void) { return 0; }\n")We should change _cgo_wait_runtime_init_done to not return a value.
Done
s/uintptr_t/void/
and drop the "return 0;" at the end.
Done
func cgocallbackg(fn, frame unsafe.Pointer, ctxt uintptr) {I think ctxt will always be zero. We should add a TODO to remove it (which will unfortunately mean editing assembly code for each platform).
Good catch. Will add the TODO and implement it in a follow-up CL.
if !mainInitDone.Load() {Quim MuntalUpdate for CL 746581.
Done
It seems to me that ctxt will always be 0 at this point. When would it be otherwise?
Yep. It can only be zero. Removing the check.
// Decrease the length of the slice by one, safely.Since we could get a SIGPROF at any time, we need to shrink the slice _before_ we release the context argument.
- Cherry Mui
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal uploaded new patchset![]( "Open in Gerrit")
- Cherry Mui
- Ian Lance Taylor
- Quim Muntal
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Cherry Mui
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Cherry Mui
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal uploaded new patchset![]( "Open in Gerrit")
Related details
- Cherry Mui
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Cherry Mui
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Lance Taylor
| Code-Review | +2 |
- Cherry Mui
- Quim Muntal
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cherry Mui
| Code-Review | +1 |
1 comment
Looks good overall.
For extra testing, I'm running it through Google's internal tests, which have substantial uses of cgo and cgo context. I'll report back once the testing is done.
Thanks!
Related details
- Quim Muntal
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Quim Muntal
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Quim Muntal
| Commit-Queue | +1 |
Looks good overall.
For extra testing, I'm running it through Google's internal tests, which have substantial uses of cgo and cgo context. I'll report back once the testing is done.Thanks!
Thanks for running those additional tests! This rewrite is kind of tricky...
Related details
- Cherry Mui
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal added 1 comment![]( "Open in Gerrit")
Quim MuntalLooks good overall.
For extra testing, I'm running it through Google's internal tests, which have substantial uses of cgo and cgo context. I'll report back once the testing is done.Thanks!
Thanks for running those additional tests! This rewrite is kind of tricky...
Any update from the internal Google tests?
Related details
- Cherry Mui
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cherry Mui
| Code-Review | +1 |
Quim MuntalLooks good overall.
For extra testing, I'm running it through Google's internal tests, which have substantial uses of cgo and cgo context. I'll report back once the testing is done.Thanks!
Quim MuntalThanks for running those additional tests! This rewrite is kind of tricky...
Any update from the internal Google tests?
Sorry for the delay. I got the results. There are some tests failing that are related to this CL, but I haven't looked them in detail, and identify whether it is this CL, or the code in Google made assumptions depending on the implementation details on the old behavior. My home lost internet yesterday due to weather, so I was not able to looked at them.
There are some data races found, when the C code is compiled with TSAN. It is possible that CL 747441 would address it. I haven't looked into it.
I'll look into more today. Thanks.
Related details
- Quim Muntal
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui voted and added 1 comment![]( "Open in Gerrit")
Votes added by Cherry Mui
| Code-Review | +0 |
Quim MuntalLooks good overall.
For extra testing, I'm running it through Google's internal tests, which have substantial uses of cgo and cgo context. I'll report back once the testing is done.Thanks!
Quim MuntalThanks for running those additional tests! This rewrite is kind of tricky...
Cherry MuiAny update from the internal Google tests?
Sorry for the delay. I got the results. There are some tests failing that are related to this CL, but I haven't looked them in detail, and identify whether it is this CL, or the code in Google made assumptions depending on the implementation details on the old behavior. My home lost internet yesterday due to weather, so I was not able to looked at them.
There are some data races found, when the C code is compiled with TSAN. It is possible that CL 747441 would address it. I haven't looked into it.
I'll look into more today. Thanks.
There are two kinds of failures:
1. As mentioned above, when the C code is compiled with thread sanitizer (not the Go code with race detector), TSAN reports races in accessing the field in the context. It might not be real race, but missed annotation? It is possible that CL 747441 would address it. (I tried this CL alone.)
2. The place where the context function is called changed. Previously it is on the C stack before calling into Go. Now it is in cgocallbackg1, called via cgocall. Now there are two extra stack transitions, C->Go then Go->C. This may make a difference in context collection. For example, some tests in Google collect a stack trace in the context function. Now with the stack transitions it is not able to trace back through them, and so the C stack is lost. In runtime.SetCgoTraceback documentation (https://pkg.go.dev/runtime#SetCgoTraceback),
While it would be correct for the context function to record a complete a stack trace whenever it is called, and simply copy that out in the traceback function, in a typical program the context function will be called many times without ever recording a traceback for that context. Recording a complete stack trace in a call to the context function is likely to be inefficient.
collecting a stack trace in the context function is discouraged but still deemed "correct". This change would break it. Even collecting just the information to identify the execution point for traceback later, like "the stack pointer and PC" as mentioned in the documentation, the stack transitions still make it hard to do full traceback later.
Given that, I think we may want to keep the property that the context function is called on the C stack without stack transition. Thanks.
Related details
- Quim Muntal
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
Quim MuntalLooks good overall.
For extra testing, I'm running it through Google's internal tests, which have substantial uses of cgo and cgo context. I'll report back once the testing is done.Thanks!
Quim MuntalThanks for running those additional tests! This rewrite is kind of tricky...
Cherry MuiAny update from the internal Google tests?
Cherry MuiSorry for the delay. I got the results. There are some tests failing that are related to this CL, but I haven't looked them in detail, and identify whether it is this CL, or the code in Google made assumptions depending on the implementation details on the old behavior. My home lost internet yesterday due to weather, so I was not able to looked at them.
There are some data races found, when the C code is compiled with TSAN. It is possible that CL 747441 would address it. I haven't looked into it.
I'll look into more today. Thanks.
There are two kinds of failures:
1. As mentioned above, when the C code is compiled with thread sanitizer (not the Go code with race detector), TSAN reports races in accessing the field in the context. It might not be real race, but missed annotation? It is possible that CL 747441 would address it. (I tried this CL alone.)
2. The place where the context function is called changed. Previously it is on the C stack before calling into Go. Now it is in cgocallbackg1, called via cgocall. Now there are two extra stack transitions, C->Go then Go->C. This may make a difference in context collection. For example, some tests in Google collect a stack trace in the context function. Now with the stack transitions it is not able to trace back through them, and so the C stack is lost. In runtime.SetCgoTraceback documentation (https://pkg.go.dev/runtime#SetCgoTraceback),
While it would be correct for the context function to record a complete a stack trace whenever it is called, and simply copy that out in the traceback function, in a typical program the context function will be called many times without ever recording a traceback for that context. Recording a complete stack trace in a call to the context function is likely to be inefficient.
collecting a stack trace in the context function is discouraged but still deemed "correct". This change would break it. Even collecting just the information to identify the execution point for traceback later, like "the stack pointer and PC" as mentioned in the documentation, the stack transitions still make it hard to do full traceback later.Given that, I think we may want to keep the property that the context function is called on the C stack without stack transition. Thanks.
Well, the comment is formatted weirdly. Hopefully you can still read it. Reported a bug internally.
Related details
- Quim Muntal
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Lance Taylor
| Code-Review | +0 |
Quim MuntalLooks good overall.
For extra testing, I'm running it through Google's internal tests, which have substantial uses of cgo and cgo context. I'll report back once the testing is done.Thanks!
Quim MuntalThanks for running those additional tests! This rewrite is kind of tricky...
Cherry MuiAny update from the internal Google tests?
Cherry MuiSorry for the delay. I got the results. There are some tests failing that are related to this CL, but I haven't looked them in detail, and identify whether it is this CL, or the code in Google made assumptions depending on the implementation details on the old behavior. My home lost internet yesterday due to weather, so I was not able to looked at them.
There are some data races found, when the C code is compiled with TSAN. It is possible that CL 747441 would address it. I haven't looked into it.
I'll look into more today. Thanks.
Cherry MuiThere are two kinds of failures:
1. As mentioned above, when the C code is compiled with thread sanitizer (not the Go code with race detector), TSAN reports races in accessing the field in the context. It might not be real race, but missed annotation? It is possible that CL 747441 would address it. (I tried this CL alone.)
2. The place where the context function is called changed. Previously it is on the C stack before calling into Go. Now it is in cgocallbackg1, called via cgocall. Now there are two extra stack transitions, C->Go then Go->C. This may make a difference in context collection. For example, some tests in Google collect a stack trace in the context function. Now with the stack transitions it is not able to trace back through them, and so the C stack is lost. In runtime.SetCgoTraceback documentation (https://pkg.go.dev/runtime#SetCgoTraceback),
While it would be correct for the context function to record a complete a stack trace whenever it is called, and simply copy that out in the traceback function, in a typical program the context function will be called many times without ever recording a traceback for that context. Recording a complete stack trace in a call to the context function is likely to be inefficient.
collecting a stack trace in the context function is discouraged but still deemed "correct". This change would break it. Even collecting just the information to identify the execution point for traceback later, like "the stack pointer and PC" as mentioned in the documentation, the stack transitions still make it hard to do full traceback later.Given that, I think we may want to keep the property that the context function is called on the C stack without stack transition. Thanks.
Well, the comment is formatted weirdly. Hopefully you can still read it. Reported a bug internally.
Cherry, thanks for pointing that out. You're right, of course: the context function needs to be called at the same point in the call stack. It unfortunately can't be called from Go code. I don't see how that could work.
Related details
- Quim Muntal
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal added 1 comment![]( "Open in Gerrit")
Quim MuntalLooks good overall.
For extra testing, I'm running it through Google's internal tests, which have substantial uses of cgo and cgo context. I'll report back once the testing is done.Thanks!
Quim MuntalThanks for running those additional tests! This rewrite is kind of tricky...
Cherry MuiAny update from the internal Google tests?
Cherry MuiSorry for the delay. I got the results. There are some tests failing that are related to this CL, but I haven't looked them in detail, and identify whether it is this CL, or the code in Google made assumptions depending on the implementation details on the old behavior. My home lost internet yesterday due to weather, so I was not able to looked at them.
There are some data races found, when the C code is compiled with TSAN. It is possible that CL 747441 would address it. I haven't looked into it.
I'll look into more today. Thanks.
Cherry MuiThere are two kinds of failures:
1. As mentioned above, when the C code is compiled with thread sanitizer (not the Go code with race detector), TSAN reports races in accessing the field in the context. It might not be real race, but missed annotation? It is possible that CL 747441 would address it. (I tried this CL alone.)
2. The place where the context function is called changed. Previously it is on the C stack before calling into Go. Now it is in cgocallbackg1, called via cgocall. Now there are two extra stack transitions, C->Go then Go->C. This may make a difference in context collection. For example, some tests in Google collect a stack trace in the context function. Now with the stack transitions it is not able to trace back through them, and so the C stack is lost. In runtime.SetCgoTraceback documentation (https://pkg.go.dev/runtime#SetCgoTraceback),
While it would be correct for the context function to record a complete a stack trace whenever it is called, and simply copy that out in the traceback function, in a typical program the context function will be called many times without ever recording a traceback for that context. Recording a complete stack trace in a call to the context function is likely to be inefficient.
collecting a stack trace in the context function is discouraged but still deemed "correct". This change would break it. Even collecting just the information to identify the execution point for traceback later, like "the stack pointer and PC" as mentioned in the documentation, the stack transitions still make it hard to do full traceback later.Given that, I think we may want to keep the property that the context function is called on the C stack without stack transition. Thanks.
Ian Lance TaylorWell, the comment is formatted weirdly. Hopefully you can still read it. Reported a bug internally.
Cherry, thanks for pointing that out. You're right, of course: the context function needs to be called at the same point in the call stack. It unfortunately can't be called from Go code. I don't see how that could work.
Thank for the review and for running the tests. It is a pity that we can't do this. I'll abandone this CL and some of the following in this chain.
Related details
- Cherry Mui
- Ian Lance Taylor
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quim Muntal (Gerrit)
Quim Muntal abandoned this change.
Quim Muntal abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |