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

Jump to bottom

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
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Milestone
Backlog

Comments

@hallazzang
Copy link
Contributor

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:

go/src/runtime/syscall_windows_test.go

Lines 103 to 118 in 5538a9a

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
@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 24, 2019
@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@agnivade agnivade added the Suggested Issues that may be good for new contributors looking for work to do. label Jul 7, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@Piyushhbhutoria
Copy link

I guess it is already fixed. please check current documentation

@hallazzang
Copy link
Contributor Author

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

@Piyushhbhutoria
Copy link

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

@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

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
Assignees
No one assigned
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@rsc @andybons @FiloSottile @agnivade @hallazzang @gopherbot @Piyushhbhutoria