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

doc: second return value of syscall.Syscall needs to be documented #29842

Open
hallazzang opened this issue Jan 20, 2019 · 5 comments
Open

doc: second return value of syscall.Syscall needs to be documented #29842

hallazzang opened this issue Jan 20, 2019 · 5 comments

Comments

@hallazzang
Copy link
Contributor

@hallazzang hallazzang commented Jan 20, 2019

I'm using Go 1.11.2 on 32 bit Windows.

Current documentation of Syscall* doesn't give any sense about second return value r2. And in most cases, this value seems not needed at all. But Go codebase internally uses it:

m1, m2, _ = VerSetConditionMask.Call(m1, m2, VER_MAJORVERSION, VER_GREATER_EQUAL)
m1, m2, _ = VerSetConditionMask.Call(m1, m2, VER_MINORVERSION, VER_GREATER_EQUAL)
m1, m2, _ = VerSetConditionMask.Call(m1, m2, VER_SERVICEPACKMAJOR, VER_GREATER_EQUAL)
m1, m2, _ = VerSetConditionMask.Call(m1, m2, VER_SERVICEPACKMINOR, VER_GREATER_EQUAL)
vi := OSVersionInfoEx{
MajorVersion: 5,
MinorVersion: 1,
ServicePackMajor: 2,
ServicePackMinor: 0,
}
vi.OSVersionInfoSize = uint32(unsafe.Sizeof(vi))
r, _, e2 := d.Proc("VerifyVersionInfoW").Call(
uintptr(unsafe.Pointer(&vi)),
VER_MAJORVERSION|VER_MINORVERSION|VER_SERVICEPACKMAJOR|VER_SERVICEPACKMINOR,
m1, m2)

And if I understood correctly, the second value is needed only when dealing with APIs that use 64 bit values(e.g. VerSetConditionMask). Since uintptr is 32 bit integer and can't hold those APIs' result, the second return value r2 is used. But there isn't any documentation about return values from syscall.Syscall* that tells which one holds highest 32 bits, etc.

Also the arguments for those APIs seems to be merged(see code above) when dealing with 64 bit values. Without knowing that, I met a horrible panic that doesn't tell any meaningful reason. So it'd be good if these behaviors are well documented.

@FiloSottile FiloSottile modified the milestones: Gccgo, Go1.12 Jan 24, 2019
@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@agnivade agnivade added the Suggested label Jul 7, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@Piyushhbhutoria
Copy link

@Piyushhbhutoria Piyushhbhutoria commented Oct 1, 2019

I guess it is already fixed. please check current documentation

@hallazzang
Copy link
Contributor Author

@hallazzang hallazzang commented Oct 2, 2019

@Piyushhbhutoria Still I don't see any documentation about r2. What do you mean by 'it is already fixed'?

@Piyushhbhutoria
Copy link

@Piyushhbhutoria Piyushhbhutoria commented Oct 2, 2019

how do i get started on this? can you explain how to recreate your panic and where to edit the docs?

@hallazzang
Copy link
Contributor Author

@hallazzang hallazzang commented Oct 3, 2019

@Piyushhbhutoria I don't have 32 bit Windows machine now so I'm afraid I cannot show you my code that created panic. Could you try yourself passing only one uintptr argument for VerSetConditionMask's first parameter on 32 bit Windows?

@Piyushhbhutoria
Copy link

@Piyushhbhutoria Piyushhbhutoria commented Oct 4, 2019

I don't have a 32-bit operating system either, I have a mac.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.