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

race detector checkptr fails on PtrOffset #124

Closed
rcoreilly opened this issue May 2, 2020 · 5 comments · Fixed by #135
Closed

race detector checkptr fails on PtrOffset #124

rcoreilly opened this issue May 2, 2020 · 5 comments · Fixed by #135

Comments

@rcoreilly
Copy link

as of version 1.14, the race detector enables checkptr https://golang.org/doc/go1.14#compiler which fails on PtrOffset, at least for example in VertexAttribPointer:

gl.VertexAttribPointer(uint32(v.handle), int32(v.typ.Vec), gpu.TheGPU.Type(v.typ.Type), false, int32(str*4), gl.PtrOffset(off*4))

Not sure there is much to be done about this given the logic of checkptr but just in case someone else hits this obstacle, here's how to overcome it:

go build -race -gcflags=all=-d=checkptr=0

@dertseha
Copy link
Contributor

dertseha commented May 20, 2020

Yup, this new checker cascades quite a lot.
The core issue here is the signature of VertexAttribPointer(), which has unsafe.Pointer as the last parameter. The fix would be to change the type to be uintptr_t in the C code - which, for this method, is more fitting as it takes arbitrary offset numbers.

See also: https://golang.org/doc/go1.14#compiler

edit: I believe #80 and the discussion there will become relevant as well now.

neclepsio added a commit to neclepsio/gl that referenced this issue Jun 19, 2020
The difference from [go-gl](https://github.com/go-gl/gl) are:
- WithOffset variants for some functions, so you don't have to pass pointers insteas of offsets (closes go-gl/gl issues [80](go-gl#80) and [124](go-gl#124)). Currently only functions `glDrawElements`, `glVertexAttribPointer`, `glGetVertexAttribPointerv` provide variants: let me know if you need more.
- No need to use cgo under Windows (much faster build times).
@neclepsio
Copy link

In the meantime it gets fixed, you can use my fork.

@rcoreilly
Copy link
Author

@neclepsio could you submit a PR to fix this issue in the main package?

@neclepsio
Copy link

@rcoreilly I did nothing but running what already done by @dertseha for issue #80. There is ongoing discussion in that issue on what and how exactly to fix. I doubt a pull request with only the result their own generator would be useful: they just need the time to think what to do. Since I needed a fix as soon as possible, and one was already available but not so easy to use, I made a fork so that everyone in my position can use it in the meantime.

@dertseha
Copy link
Contributor

For clarification why #135 closes this issue: Instead of calling gl.VertexAttribPointer() the code needs to call the newly available gl.VertexAttribPointerWithOffset() - like this:

gl.VertexAttribPointerWithOffset(uint32(v.handle), int32(v.typ.Vec), gpu.TheGPU.Type(v.typ.Type), false, int32(str*4), uintptr(off*4))

Note the change in type of the last argument.

There are a few more overloads with WithOffset in their name. There might be more functions that need this treatment - see READMEs of go-gl/glow and go-gl/gl for more details on checkptr and overloads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants