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

[Go] Enable GC checks #38824

Closed
pitrou opened this issue Nov 21, 2023 · 4 comments · Fixed by #38826
Closed

[Go] Enable GC checks #38824

pitrou opened this issue Nov 21, 2023 · 4 comments · Fixed by #38826

Comments

@pitrou
Copy link
Member

pitrou commented Nov 21, 2023

Describe the enhancement requested

Judging by this sporadic failure, it seems we should enable GC checks on unsafe pointers in CGo builds:
https://github.com/apache/arrow/actions/runs/6943203792/job/18887771947?pr=38799#step:8:19738

runtime: marked free object in span 0x7efb82dd4608, elemsize=80 freeindex=8 (bad use of unsafe.Pointer? try -d=checkptr)

Component(s)

Go, Integration

@pitrou
Copy link
Member Author

pitrou commented Nov 21, 2023

@zeroshade What do you think?

@pitrou
Copy link
Member Author

pitrou commented Nov 21, 2023

Well, trying this I'm getting lots of integration failures locally such as:

<class 'RuntimeError'>: Command failed: /opt/go/bin/arrow-json-integration-test -arrow /tmp/tmpqrulbq1o/02bb48c6_generated_map_non_canonical.json_as_file -json /tmp/arrow-integration-tu81iiwl/generated_map_non_canonical.json -mode JSON_TO_ARROW
With output:
--------------
fatal error: checkptr: pointer arithmetic result points to invalid allocation

goroutine 1 [running]:
runtime.throw({0x78b639?, 0xf?})
	/opt/go/src/runtime/panic.go:1047 +0x5d fp=0xc0002149b0 sp=0xc000214980 pc=0x4353dd
runtime.checkptrArithmetic(0x77b926?, {0x0, 0x0, 0x2?})
	/opt/go/src/runtime/checkptr.go:69 +0xaa fp=0xc0002149e0 sp=0xc0002149b0 pc=0x40776a
strings.noescape(...)
	/opt/go/src/strings/builder.go:30
strings.(*Builder).copyCheck(...)
	/opt/go/src/strings/builder.go:40
strings.(*Builder).WriteString(...)
	/opt/go/src/strings/builder.go:123
github.com/apache/arrow/go/v15/arrow.(*MapType).String(0xc00007d1e0?)
	/arrow/go/arrow/datatype_nested.go:549 +0x1ac fp=0xc000214be0 sp=0xc0002149e0 pc=0x5bd56c
fmt.(*pp).handleMethods(0xc0000c1e10, 0xa3a600?)
	/opt/go/src/fmt/print.go:657 +0x30b fp=0xc000214e30 sp=0xc000214be0 pc=0x4ac14b
fmt.(*pp).printArg(0xc0000c1e10, {0x75d740?, 0xc00007d1e0}, 0x76)
	/opt/go/src/fmt/print.go:740 +0x69b fp=0xc000214ed0 sp=0xc000214e30 pc=0x4acd1b
fmt.(*pp).doPrintf(0xc0000c1e10, {0x77c4cd, 0xd}, {0xc0002150a0?, 0x3, 0x3})
	/opt/go/src/fmt/print.go:1057 +0x288 fp=0xc000214fc8 sp=0xc000214ed0 pc=0x4af808
fmt.Fprintf({0x801c60, 0xc00007f2e0}, {0x77c4cd, 0xd}, {0xc0002150a0, 0x3, 0x3})
	/opt/go/src/fmt/print.go:204 +0x75 fp=0xc000215028 sp=0xc000214fc8 pc=0x4a9955
github.com/apache/arrow/go/v15/arrow.Field.String({{0xc0001d4393, 0xf}, {0x8038e0, 0xc00007d1e0}, 0x1, {{0x0, 0x0, 0x0}, {0x0, 0x0, ...}}})
	/arrow/go/arrow/datatype_nested.go:943 +0x128 fp=0xc000215138 sp=0xc000215028 pc=0x5bee28
github.com/apache/arrow/go/v15/arrow.(*Field).String(0x0?)
	<autogenerated>:1 +0x98 fp=0xc0002151f8 sp=0xc000215138 pc=0x5c2a78
fmt.(*pp).handleMethods(0xc0000c1d40, 0xa3a600?)
	/opt/go/src/fmt/print.go:657 +0x30b fp=0xc000215448 sp=0xc0002151f8 pc=0x4ac14b
fmt.(*pp).printArg(0xc0000c1d40, {0x755000?, 0xc0000a4ae0}, 0x76)
	/opt/go/src/fmt/print.go:740 +0x69b fp=0xc0002154e8 sp=0xc000215448 pc=0x4acd1b
fmt.(*pp).doPrintf(0xc0000c1d40, {0x77ad61, 0x8}, {0xc0002156b8?, 0x1, 0x1})
	/opt/go/src/fmt/print.go:1057 +0x288 fp=0xc0002155e0 sp=0xc0002154e8 pc=0x4af808
fmt.Fprintf({0x801c60, 0xc00007f2c0}, {0x77ad61, 0x8}, {0xc0002156b8, 0x1, 0x1})
	/opt/go/src/fmt/print.go:204 +0x75 fp=0xc000215640 sp=0xc0002155e0 pc=0x4a9955
github.com/apache/arrow/go/v15/arrow.(*Schema).String(0xc0000a4900)
	/arrow/go/arrow/schema.go:267 +0x4d1 fp=0xc000215848 sp=0xc000215640 pc=0x5c17d1
fmt.(*pp).handleMethods(0xc0000c0410, 0xa3a600?)
	/opt/go/src/fmt/print.go:657 +0x30b fp=0xc000215a98 sp=0xc000215848 pc=0x4ac14b
fmt.(*pp).printArg(0xc0000c0410, {0x75bd00?, 0xc0000a4900}, 0x76)
	/opt/go/src/fmt/print.go:740 +0x69b fp=0xc000215b38 sp=0xc000215a98 pc=0x4acd1b
fmt.(*pp).doPrintf(0xc0000c0410, {0x77d7de, 0x11}, {0xc000215e70?, 0x1, 0x1})
	/opt/go/src/fmt/print.go:1057 +0x288 fp=0xc000215c30 sp=0xc000215b38 pc=0x4af808
fmt.Sprintf({0x77d7de, 0x11}, {0xc000215e70, 0x1, 0x1})
	/opt/go/src/fmt/print.go:219 +0x59 fp=0xc000215c88 sp=0xc000215c30 pc=0x4a9a59
log.Printf({0x77d7de?, 0xc000014040?}, {0xc000215e70?, 0x0?, 0xd0?})
	/opt/go/src/log/log.go:354 +0x3b fp=0xc000215cc0 sp=0xc000215c88 pc=0x4b9c7b
main.cnvToARROW({0x7ffdb574bc6e, 0x42}, {0x7ffdb574bcb7, 0x40}, 0x1)
	/arrow/go/arrow/ipc/cmd/arrow-json-integration-test/main.go:145 +0x34f fp=0xc000215eb0 sp=0xc000215cc0 pc=0x6fdaaf
main.runCommand({0x7ffdb574bcb7, 0x40}, {0x7ffdb574bc6e, 0x42}, {0x7ffdb574bcfe, 0xd}, 0x49?)
	/arrow/go/arrow/ipc/cmd/arrow-json-integration-test/main.go:67 +0x18c fp=0xc000215f08 sp=0xc000215eb0 pc=0x6fcb2c
main.main()
	/arrow/go/arrow/ipc/cmd/arrow-json-integration-test/main.go:46 +0x187 fp=0xc000215f80 sp=0xc000215f08 pc=0x6fc947
runtime.main()
	/opt/go/src/runtime/proc.go:250 +0x212 fp=0xc000215fe0 sp=0xc000215f80 pc=0x437c72
runtime.goexit()
	/opt/go/src/runtime/asm_amd64.s:1594 +0x1 fp=0xc000215fe8 sp=0xc000215fe0 pc=0x4640e1

@pitrou
Copy link
Member Author

pitrou commented Nov 21, 2023

Ok, it seems to be because of golang/go#42131 (comment)

Hats off to Go for excellent documentation as usual.

@zeroshade
Copy link
Member

In the past, when we've encountered this it has been that we weren't zero'ing out one of the C structs before it being passed to a function and the pointer checker was seeing garbage that happened to look similarly to a Go pointer which triggered the error.

I'm guessing there's another spot where we're failing to zero out the struct and probably need to add another trampoline like we did before.

All that being said, I agree we should probably enable the checks for CGO CI builds and integrations

pitrou added a commit that referenced this issue Nov 27, 2023
### What changes are included in this PR?

Our codebase has many uses of `unsafe.Pointer`. It may be a good idea to add debug checks for these uses in CI builds, especially where data crosses C boundaries.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.

* Closes: #38824

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@pitrou pitrou added this to the 15.0.0 milestone Nov 27, 2023
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
### What changes are included in this PR?

Our codebase has many uses of `unsafe.Pointer`. It may be a good idea to add debug checks for these uses in CI builds, especially where data crosses C boundaries.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.

* Closes: apache#38824

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants