-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
syscall: (*Proc).Call does not keep arguments live #16035
Comments
OS version: Windows 10 [10.0.10586] The bug also happens in go1.7beta1 |
The basic problem is that there is nothing keeping This doesn't happen when you call Basically, the |
The immediate workaround for your program is to add something like this after the call to
(the I'm not sure what to do about the general case, though. Right now the |
CC @randall77 |
It has always been that way - when you use unsafe package you need to know what you're doing. syscall.Syscall and syscall.(*Proc).Call is how we call external code on windows - there is no way around it. Use of syscall.Syscall and syscall.(*Proc).Call has always been undocumented. From what I understand, we didn't document these rules because we still have not decided what should or should not be allowed. @ianlancetaylor I am surprised you are recommending to use new runtime.KeepAlive function. I specifically asked if runtime.KeepAlive is suitable for such things (see https://groups.google.com/forum/#!searchin/golang-dev/KeepAlive$20runtime/golang-dev/AlMCfgQLkdo/PSYsA9x3DQAJ), but was told that runtime.KeepAlive is to control when finalizers run, not to control garbage collector. I am not sure what you're planing to do on this issue. Especially considering it is marked as go1.7. I am sure we have similar issues already, for example #4318, #6907 Alex |
@alexbrainman I don't see an inconsistency in the use of My suggestion here is not to use It's very much the same kind of thing as finalizers, although there is no actual finalizer. Think of an implicit finalizer being discarding the values. What do you think of https://golang.org/cl/24030? |
CL https://golang.org/cl/24030 mentions this issue. |
Does
I am trying to understand what runtime.KeepAlive does before I can decide on your CL. Lets continue that conversation here. Alex |
No, However, my current thinking is that it is not an issue here, because 1) currently memory never moves anyhow; 2) if we ever do permit memory to move, the So the point of the |
Would it be possible to apply the special behavior for |
Possible? Yes. But a bit of risky move at this stage of the release. The special handling of Of course, although But now that I think about it, because |
The issue does not appear to happen when using |
Thanks. Could somebody test whether https://golang.org/cl/24031 fixes the problem? |
CL https://golang.org/cl/24031 mentions this issue. |
I don't think this is true. If Go object lives on goroutine stack, the stack moves and object moves with the stack. In fact that is what, I think, is happening here. I have changed the program to handle syscall error properly first. So I end up with this:
Then I run the program:
(note how program succeeds if I use -N flag). Then I changed it slightly, so that
This time program succeeds both times. I suspect that compiler is too clever and rearrange this code to keep
It does not fixes the problem as far as I am concerned. The program fails in a similar way as before and only suceeds if I add the Alex |
Thanks for testing the CL. I'm puzzled that we do not see the problem with Your tests do suggest that the problem is with You're right, of course, about the stack moving. cgo works with a moving stack by always ensuring that the arguments escape, which is also what the generated syscall package functions do. I'm not sure how to document this. |
If the fix is only documentation then we can do this at any point before the 1.7 release. Otherwise it will have to wait for 1.8. |
I've taken another tack here. Could somebody please try https://golang.org/cl/24551 to see if it fixes the problem on Windows? Thanks. |
CL https://golang.org/cl/24551 mentions this issue. |
I does not fixes the problem for me. Trying CL 24551 against prorgram listed here #16035 (comment) I get:
If you provide suggestions on how to debug this, I will. Thank you. Alex |
Hmmm, the magic comment may not work on method calls. |
Actually, methods are fine, it's failing because of the |
@alexbrainman When you have time, could you try the latest version of https://golang.org/cl/24551 ? Thanks. |
Still broken:
I am happy to try things, if you tell me what. Thank you. Alex |
@alexbrainman OK, that time I actually did break imports. Please try the latest version of https://golang.org/cl/24551. Thanks. |
It works for me now. Thank you. Can we apply your strategy here https://github.com/golang/sys/blob/master/windows/dll_windows.go#L124 too? Alex |
Thanks for testing it. Yes, if this gets approved, we can do it in the golang.org/x/sys repo as well. Unfortunately it won't work if somebody uses an interface with a |
I am not overly concerned about solving this. This whole issue is a moving target, as GC changes we will break it again and again. We need to find proper solution to this problem. Alex |
This isn't a GC problem, quite. The test case in the CL doesn't run the garbage collector, but it does fail without the compiler change. The problem is that stack growth requires that we precisely identify all pointers on the stack, but when we call a method like One possible proper solution might be to write |
Please, tell more. I don't understand what you are proposing. And why would we need another "proper solution" if you current fix works already? Thank you. Alex |
My solution, which is now submitted, does not work if somebody writes
The fact that the call requires special treatment is lost when the call is made through an interface. This is not a problem for The approach I was suggesting above would be to add variants of
This approach keeps the pointer value as a pointer down to the call to But, of course, it makes the functions harder to create because you have to construct a type. On the plus side, they are easier to call. Handling different numbers of arguments is tedious though straightforward. |
Thanks for explaining. Looks too complicated for a simple task. Hopefully no one stores syscall.Proc or syscall.LazyDLL in an interface variable. Alex |
CL https://golang.org/cl/24870 mentions this issue. |
CL 24551 introduced //go:uintptrescapes comment to make syscall.Proc.Call and syscall.LazyProc.Call parameters escape. Use new comment in this package too. Updates golang/go#16035. Change-Id: I57ec3b4778195ca4a1ce9a8eec331f0f69285926 Reviewed-on: https://go-review.googlesource.com/24870 Reviewed-by: Ian Lance Taylor <[email protected]> Run-TryBot: Ian Lance Taylor <[email protected]> TryBot-Result: Gobot Gobot <[email protected]>
The following test program should give the output of the
ImageState
value for Windows, which for most systems isIMAGE_STATE_COMPLETE
. However, when trying to get the value into a slice it is possible for GC to mess up the slice and not get the updated value from the syscall.Test program:
Expected response:
str: IMAGE_STATE_COMPLETE
Actual response:
str:
If the
regGetValue.Call(...)
line is replace withsyscall.Syscall9(regGetValue.Addr(), ...)
then the code works as intended.Environment:
The text was updated successfully, but these errors were encountered: