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

cmd/compile: fatal error: checkptr: pointer arithmetic computed bad pointer value #64467

Open
gucio321 opened this issue Nov 30, 2023 · 13 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@gucio321
Copy link

Go version

go version go1.21.3 linux/amd64

What operating system and processor architecture are you using (go env)?

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/username/.cache/go-build'
GOENV='/home/username/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/username/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/username/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.3'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2422207597=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I want to cast an unintptr to unsafe.Pointer.
When running with -race it crashes:

fatal error: checkptr: pointer arithmetic computed bad pointer value

Here is the code

func (selfStruct *TextureID) c() (result C.ImTextureID, fin func(
        self := selfStruct.Data

        return (C.ImTextureID)(unsafe.Pointer(self)), func() {} // <- this line causes crash
}

What did you expect to see?

No panic when running with -race

In reference to #64111 as a solution of that issue I was suggested to use uintptr as an alternative to unsafe.pointer.
However I need to cast it (for a while) to unsafe.pointer to be able to pass it to C function taking void*

What did you see instead?

fatal error: checkptr: pointer arithmetic computed bad pointer value
@cuonglm
Copy link
Member

cuonglm commented Nov 30, 2023

Your code violates the 3rd rule: https://pkg.go.dev/unsafe#Pointer

@gucio321
Copy link
Author

gucio321 commented Nov 30, 2023

So maybe it shouldn't apply to packages that use C or something like this?
Or maybe consider fixing AllenDang/giu#730 somehow else...

@seankhliao seankhliao changed the title compiler: fatal error: checkptr: pointer arithmetic computed bad pointer value cmd/compile: fatal error: checkptr: pointer arithmetic computed bad pointer value Nov 30, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 30, 2023
@dmitshur dmitshur added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Nov 30, 2023
@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@gopherbot gopherbot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 30, 2023
@gucio321
Copy link
Author

@gopherbot it shouldn't be closed

CC @dmitshur

@thepudds thepudds reopened this Dec 31, 2023
@thepudds thepudds removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 31, 2023
@thepudds
Copy link
Contributor

Hi @gucio321, can you clarify why this shouldn’t be closed?

Can you also comment more directly on this:

Your code violates the 3rd rule:
https://pkg.go.dev/unsafe#Pointer

Do you agree with that assessment?

Also, note that the Go project does not use the issue tracker for answering questions:

https://go.dev/wiki/Questions

@thepudds thepudds added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 31, 2023
@gucio321
Copy link
Author

gucio321 commented Jan 1, 2024

Hi, thank you for your reply!
Let's look at the origin of this problem: in #64111 the solution suggested was to use uintptr to handle void* C type. In go however the only way to pass something into C code as a void ptr is using an unsafe.pointer. now let's say i have a function in C that returns void*. It could happen that it returns a 0x1 value (because it's C so why not 😊). Now let's say i just want to print this void* value in go using fmt. This is where the problem described in #64111 appears.
A solution is converting the unsafe.pointer to uintptr go type before printing it. But now let's say i need to pass this uintptr back to C (again - as a void*) - it violates 3rd rule of race detector

If we consider solution from #64111 the only possible (and we should because the issue was closed as completed) it seems logical to me that this issue needs separated fix and should not be closed.

In my opinion rhe solution here would be either

I hope I've justifies my point of view. Lmk if something is unclear or i need to provide some more info!

@randall77
Copy link
Contributor

the only way to pass something into C code as a void ptr is using an unsafe.pointer.

You can certainly pass it as a uintptr and do the conversion to void* on the C side.

Similarly for the return value. Wrap (in C) the thing that returns a void* to convert the result to a uintptr and return that to Go.

it violates 3rd rule of race detector

What rule are you referring to here? (A reference link would be great.)

@gucio321
Copy link
Author

gucio321 commented Jan 2, 2024

What rule are you referring to here? (A reference link would be great.)

I'm sorry - I forgot to put it there. I mean the rules mentioned in 1st comment on this thread. Ref link: https://pkg.go.dev/unsafe#Pointer

You can certainly pass it as a uintptr and do the conversion to void* on the C side.

Similarly for the return value. Wrap (in C) the thing that returns a void* to convert the result to a uintptr and return that to Go.

Well, so this is a workaround, however its (in my opinion) a bit stupid that user can't properly handle a normal thing (a void* C pointer) using GO stuff and they need to do the trick on C side... [digression: shouldn't void* be handled as interface{}?].

I still view that this issue needs some fix in GO.

@thepudds
Copy link
Contributor

thepudds commented Jan 2, 2024

Hi @gucio321, "stupid" might be an overly strong word to choose? FWIW, I think cgo is tackling a fairly difficult and nuanced set of problems.

In #64111, you reported the error:

runtime: bad pointer in frame fmt.(*pp).printValue at 0xc00022d758: 0x1

and you mentioned towards the end of that discussion that:

I still think that fmt shouldn't care what exactly is the pointer

In that issue, it's not that the fmt code is complaining, but instead I suspect the Go runtime is complaining when it is adjusting pointer values it finds on the stack:

go/src/runtime/stack.go

Lines 621 to 626 in b25f555

if f.valid() && 0 < p && p < minLegalPointer && debug.invalidptr != 0 {
// Looks like a junk value in a pointer slot.
// Live analysis wrong?
getg().m.traceback = 2
print("runtime: bad pointer in frame ", funcname(f), " at ", pp, ": ", hex(p), "\n")
throw("invalid pointer found on stack")

Given the Go runtime implements growable stacks, it has to adjust pointers in some cases, including so that pointers to stack locations are still valid after the stack has been copied/moved. While adjusting pointers, in your case it seems the runtime found what looks to be an illegal Go pointer, and it complains about that as an important diagnostic to help people avoid "Heisenbugs", intermittent crashes, or subtle data corruptions.

In this issue here, it's also a deliberate diagnostic check that is complaining about the violation of the unsafe rules (in this case, additional pointer checks that are automatically enabled when running with the race detector). When these diagnostics were first introduced, the reaction was extremely positive, such as in this excellent blog entry from then:

I’ve written a fair amount of unsafe code in Go, but even veteran programmers can easily make mistakes. This is why these types of diagnostic tools are so important!

(That write-up is worth a read if you haven't already read it. It's by Matt Layher, a heavy user of unsafe & cgo from the broader community).

In any event, you don't seem to be encountering a bug in Go, but rather automatic enforcement of an important set of rules. As the cgo documentation says:

Go is a garbage collected language, and the garbage collector needs to know the location of every pointer to Go memory. Because of this, there are restrictions on passing pointers between Go and C.

Fortunately, Go does seem to support what you want to do. As Bryan said in #64111 (comment):

If you have a C type that may represent a mix of pointers and integers, you should generally pass it to Go as a uintptr

You mentioned above:

user can't properly handle a normal thing (a void* C pointer) using GO stuff and they need to do the trick on C side

If you don't want to change your actual C code, could you have some wrappers or small conversion utilities in the cgo preamble of one of your .go files, which is a nice convenience offered by cgo for such things? If the Go side never needs to interpret that value as a Go pointer, can it always be a uintptr on the Go side?

@gucio321
Copy link
Author

gucio321 commented Jan 2, 2024

Thank you for this detailed answer!
There are two more things however that are in my opinion important here:

  • why fmt has anything to do with pointer validity?
  • maybe CGO could use uintptr as a default (or alternative) type for C void* pointer as the 0x1 isn't something strange for C code and may be used by C code authors.

@gucio321

This comment was marked as outdated.

@thepudds
Copy link
Contributor

thepudds commented Jan 2, 2024

why fmt has anything to do with pointer validity?

Hi @gucio321, I tried to explain that above in #64467 (comment) in the part about the runtime adjusting pointers it finds on the stack in some cases. It does not require fmt.

Here's a quick example from scratch that doesn't use fmt and fails with the same error your reported:

https://go.dev/play/p/d09eyqUlVV0

func main() {
	stretchStack(16, 0) // use ~16 KiB of stack.
	p := badPointer()   // store unsafe.Pointer with value 0x1 on stack.
	runtime.GC()        // trigger stack shrinking and discovery of bad pointer on stack (usually).
	foo(p)
}

Running that gives:

runtime: bad pointer in frame main.main at 0xc00010ff28: 0x1
fatal error: invalid pointer found on stack

Note that the error stack includes:

runtime.adjustpointers(...)
runtime.adjustframe(...)
runtime.copystack(...)
runtime.shrinkstack(...)

@gucio321
Copy link
Author

gucio321 commented Jan 2, 2024

ok, I see thank you.
What about this?

  • maybe CGO could use uintptr as a default (or alternative) type for C void* pointer as the 0x1 isn't something strange for C code and may be used by C code authors.

gucio321 added a commit to gucio321/cimgui-go that referenced this issue Jan 6, 2024
currently it applies to cimgui functions and typedefs.
What is missing is the struct_accessor that will be reworked in the next commit.

Rason for that is golang/go#64467
@seankhliao seankhliao removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 13, 2024
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 15, 2024
@dmitshur dmitshur added this to the Backlog milestone Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Status: In Progress
Development

No branches or pull requests

7 participants