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.

Sign up for GitHub

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

Unsafe pointer arithmetic #30

Closed
pztrn opened this issue Mar 3, 2020 · 26 comments
Closed

Unsafe pointer arithmetic #30

pztrn opened this issue Mar 3, 2020 · 26 comments
Labels
help wanted Extra attention is needed

Comments

@pztrn
Copy link

pztrn commented Mar 3, 2020

I've compiled application with -race and it crashed right before showing up GUI.

fatal error: checkptr: unsafe pointer arithmetic

goroutine 1 [running, locked to thread]:
runtime.throw(0xce6324, 0x23)
        /usr/lib/go/src/runtime/panic.go:1112 +0x72 fp=0xc00011fa18 sp=0xc00011f9e8 pc=0x483d12
runtime.checkptrArithmetic(0x1, 0x0, 0x0, 0x0)
        /usr/lib/go/src/runtime/checkptr.go:24 +0xce fp=0xc00011fa48 sp=0xc00011fa18 pc=0x456fde
github.com/AllenDang/giu/imgui.TextureID.handle(...)
        /home/pztrn/projects/go/pkg/mod/github.com/!allen!dang/giu@v0.0.0-20200210105846-9cbca4cde9f2/imgui/TextureID.go:24
github.com/AllenDang/giu/imgui.FontAtlas.SetTextureID.func1(0x27280e0, 0x1)
        /home/pztrn/projects/go/pkg/mod/github.com/!allen!dang/giu@v0.0.0-20200210105846-9cbca4cde9f2/imgui/FontAtlas.go:174 +0x78 fp=0xc00011fa88 sp=0xc00011fa48 pc=0xa3c3c8
github.com/AllenDang/giu/imgui.FontAtlas.SetTextureID(0x27280e0, 0x1)
        /home/pztrn/projects/go/pkg/mod/github.com/!allen!dang/giu@v0.0.0-20200210105846-9cbca4cde9f2/imgui/FontAtlas.go:174 +0x43 fp=0xc00011faa8 sp=0xc00011fa88 pc=0xa35683
github.com/AllenDang/giu/imgui.(*OpenGL3).createFontsTexture(0xc000074230)
        /home/pztrn/projects/go/pkg/mod/github.com/!allen!dang/giu@v0.0.0-20200210105846-9cbca4cde9f2/imgui/RendererOpenGL3.go:338 +0x3e5 fp=0xc00011fb50 sp=0xc00011faa8 pc=0xa289f5
github.com/AllenDang/giu/imgui.(*OpenGL3).createDeviceObjects(0xc000074230)
        /home/pztrn/projects/go/pkg/mod/github.com/!allen!dang/giu@v0.0.0-20200210105846-9cbca4cde9f2/imgui/RendererOpenGL3.go:292 +0xd26 fp=0xc00011fcc8 sp=0xc00011fb50 pc=0xa28256
github.com/AllenDang/giu/imgui.NewOpenGL3(0x2724368, 0x3fe00000, 0xb, 0x258, 0x1f4)
        /home/pztrn/projects/go/pkg/mod/github.com/!allen!dang/giu@v0.0.0-20200210105846-9cbca4cde9f2/imgui/RendererOpenGL3.go:46 +0x1f5 fp=0xc00011fd50 sp=0xc00011fcc8 pc=0xa25485
github.com/AllenDang/giu.NewMasterWindowWithBgColor(0xcd52c1, 0xb, 0x258, 0x1f4, 0x1, 0xc00011fec0, 0x0, 0x7fd2ff3d8108)
        /home/pztrn/projects/go/pkg/mod/github.com/!allen!dang/giu@v0.0.0-20200210105846-9cbca4cde9f2/MasterWindow.go:54 +0x263 fp=0xc00011fe30 sp=0xc00011fd50 pc=0xa53723
github.com/AllenDang/giu.NewMasterWindow(...)
        /home/pztrn/projects/go/pkg/mod/github.com/!allen!dang/giu@v0.0.0-20200210105846-9cbca4cde9f2/MasterWindow.go:25
@AllenDang
Copy link
Owner

It's a wrapper for a C pointer. I need to dig a little bit more about what -race does. BTW, it works fine in normal mode.

@pztrn
Copy link
Author

pztrn commented Mar 4, 2020

Yep, I agree, but this will prevent running application with -race for race detection which is not good IMO.

@AllenDang
Copy link
Owner

@pztrn Yes, you got the point. I will dig it out.

@AllenDang
Copy link
Owner

This issue caused by converting uintptr to a C void* pointer. Anyone knows how to avoid this crash?

@AllenDang AllenDang added the help wanted Extra attention is needed label Mar 7, 2020
@ryn1x
Copy link
Contributor

ryn1x commented Jun 18, 2020

I am getting this same error. This prevents me from using the -race tool. Seems like a pretty big issues since there is a lot of concurrency going on in a typical GUI application and -race is really needed.

@AllenDang
Copy link
Owner

@ryn1x I'll take a look on this deeply this time. This is truly a big issue.

@yarcat
Copy link
Contributor

yarcat commented Aug 12, 2020 •
edited
Loading

The trick here is that there should be no unsafe.Pointer. All void* should be replaced with uintptr_t from <stdint.h> (or <cstdint>). If uintptr -to- void* conversion is required, it should be done on C side, not on Go.

Check this example commit, which allows to bypass first of the failures demonstrated by using -race option passed to go build/run.

It's quite tedious and non-rewarding work. But I'll do my best to help with it.

UPD: Forgot to mention, that I was trying to run go run -race ./examples/helloworld.

@yarcat
Copy link
Contributor

yarcat commented Aug 12, 2020

Unfortunately they also have this in go-gl which makes it harder to fix as the rabbit hole is much deeper than I hoped.

@AllenDang
Copy link
Owner

@yarcat Yeah, I met the same upstream problem last time when I try to fix this.

@kettek kettek mentioned this issue Oct 6, 2020
Merged
@kettek
Copy link
Contributor

kettek commented Oct 7, 2020 •
edited
Loading

To note, the fork @ https://github.com/neclepsio/gl has some variant function calls that, afaik, should resolve problems stemming from unsafe.Pointer use. I've experimentally tried using it (along with changing some of the imgui C interface) and it seemed to fix the pointer arithmetic problems.

@AllenDang
Copy link
Owner

@kettek Wow! Can you make a PR?

@kettek
Copy link
Contributor

kettek commented Oct 8, 2020

Sure. It ended up being part of my attempts to do per-texture filtering, so I'll separate it from that first.

@runrc
Copy link

runrc commented Nov 4, 2020

Unfortunately this issue is a bit of show stopper for a concurrent GIU application, where using -race to catch data race issue is very important. Will this be resolved soon?

@AllenDang
Copy link
Owner

@ravicheema Cannot promise when, but I'm keep trying to fix it everyday.

@kettek
Copy link
Contributor

kettek commented Nov 6, 2020

I'll attempt to make a PR this weekend integrating the changes I had experimented with last month.

@kettek
Copy link
Contributor

kettek commented Nov 10, 2020

See #83

There are still issues with actual race conditions but this PR fixes the unsafe pointer arithmetic problem.

@runrc
Copy link

runrc commented Nov 11, 2020 •
edited
Loading

WORK AROUND - Go +1.14 has added -d=checkptr which adds instrumentation to check that Go code is following unsafe.Pointer safety rules when -race is specified. This can be disabled with gcflags=all=-d=checkptr=0

Use the following to perform data race condition checks and disable unsafe.Pointer safety rules,
go run -race -gcflags=all=-d=checkptr=0 .

Not ideal but it is a work around.

@dertseha
Copy link

Hello there!
I bring greetings from go-gl/gl, where a PR ( go-gl/gl#135 ) has landed that allows to handle checkptr issues - for the most common GL calls.

For example, instead of calling gl.VertexAttribPointer() the code needs to call the newly available gl.VertexAttribPointerWithOffset() - 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 in case more function overloads are needed.

In general, any use of gl.PtrOffset() is discouraged - a future update has it marked as deprecated.

I have not reviewed the giu code, my proposal is to remove any use of PtrOffset() and ensure direct use of uintptr type - and in case overloads are needed, use those or please report them missing.

Also, as the maintainer of imgui-go, I can report that the original issue about TextureID has also long been fixed.

@kettek
Copy link
Contributor

kettek commented Apr 30, 2021

Great to hear! I will rework the changes in PR #83, as that used a fork of go-gl/gl that implemented WithOffset, to use the updated go-gl/gl version as well as to reintegrate imgui-go's TextureID fixes.

Thanks for the update!

@AllenDang
Copy link
Owner

@dertseha Wow! This is a great news! @kettek Thanks for your great work! Looking forward for your PR.

@gucio321
Copy link
Collaborator

gucio321 commented Oct 5, 2021 •
edited
Loading

@AllenDang it seems like upstream imgui-go doesn't have this issue anymore. I tried to run inkyblackness/imgui-go-examples example and it went clean.

@AllenDang
Copy link
Owner

@gucio321 I'll check it out and fix it soon.

@runrc
Copy link

runrc commented Jan 31, 2022 •
edited
Loading

@AllenDang and @gucio321
Any updates on this issue? This is an important issue for multiple-threaded applications.

@gucio321 gucio321 mentioned this issue Feb 25, 2022
Closed
@gucio321
Copy link
Collaborator

well so like I said any example from inkyblackness/imgui-g-examples works well with -race so i suppose that @AllenDang needs to change something inside of AllenDang/imgui-go however idk what is the point there.

@AllenDang
Copy link
Owner

@gucio321 Yes, it's the root cause. I'm thinking about to use cimgui (as it is auto-generated and easy to sync the changes from imgui) to recreate the binding, might need more time to evaluate.
And sorry for the delay, :P

AllenDang added a commit that referenced this issue Mar 22, 2022
@AllenDang
Solve #30
106202c
@AllenDang
Copy link
Owner

@pztrn @gucio321 @runrc I got it fixed, finally...
@dertseha Thanks for your code, they guide me step by step to right point.

@gucio321 gucio321 mentioned this issue Dec 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@AllenDang @yarcat @pztrn @dertseha @runrc @kettek @ryn1x @gucio321