fix: go's atomic operations require 64bit alignment in structs on ARM#323
Conversation
|
arrow/ipc/reader.go
Outdated
| // refCount needs to be 64-bit aligned | ||
| // https://pkg.go.dev/sync/atomic#pkg-note-BUG | ||
| refCount int64 |
There was a problem hiding this comment.
does this actually ensure alignment? Or should we use atomic.Int64 ?
There was a problem hiding this comment.
It does because it's now at the start of the struct. The issue was that schema *arrow.Schema is a pointer, which is 32-bit on arm (without it being arm64).
Using atomic.Int64 would be indeed a safer option since it does alignment automatically . Good point! 👍
There was a problem hiding this comment.
I swapped with atomic.Int64 now. I think this would need to be done on the complete code base though. If I have some time next week I can offer to do this. Along with that I could also double check that we don't have more alignment issues I did not stumble into yet.
Well, that's because it's not arm64, but "regular" 32-bit arm in our case. |
Ah! sigh yea, i guess it would be best to ensure we default to the pure go impls then |
Added in this PR. |
zeroshade
left a comment
There was a problem hiding this comment.
Looks good! Thanks! will merge once CI passes
### Rationale for this change This is a follow-up to #323. ARM 32-bit requires atomic operations to be 32-bit aligned. This has not been the case in a rather large amount of cases in the code base. Failing to align causes crashes during runtime. ### What changes are included in this PR? I replaced all uses of `atomic.LoadInt64`, `atomic.StoreInt64` and `atomic.AddInt64` with `atomic.Int64{}` and its methods. This type has built-in alignment, so it does not matter in which order struct fields appear, which makes it generally harder to screw up alignment during changes. ### Are these changes tested? Briefly on the 32-bit architecture with the same program mentioned in #323. The best way to move forward here would be to also add a 32bit CI environment to make sure `arrow` builds and tests there. But I guess this is out of scope for this PR. I've noticed that `go vet ./...` produces many warnings over the project, most of them already there before this PR. The new ones are all of the kind: ``` arrow/array/dictionary.go:614:42: literal copies lock value from bldr: github.com/apache/arrow-go/v18/arrow/array.dictionaryBuilder contains github.com/apache/arrow-go/v18/arrow/array.builder contains sync/atomic.Int64 contains sync/atomic.noCopy ``` I.e. false positives due to initialization in another struct and then copying it in a constructor function. ### Are there any user-facing changes? In the hope that everything was done right: no.
Rationale for this change
I've tried to run a pqarrow based writer/reader on a RaspberryPi. This lead to an almost immediate crash however, due to an unaligned struct member in
ipc.Reader.I did not check yet if there are other structs that might have similar issues. It would probably be nice to
What changes are included in this PR?
Swapping the struct field order is enough to fix the issue. See also the link in the diff.
Are these changes tested?
Yes, no additional unit tests due to hardware constraints, but my program runs through now.
Are there any user-facing changes?
Less crashing?
Other note
I noticed that this will also crash if I do not pass the
noasmbuild tag, which is probably also a little unfortunate. This seems to be because thefunclistinparquet/internal/bmi/bmi.gois empty. If you want I can add a default, so the funclist is always initialized with the fallback go implementation, unless overwritten.