This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-go.git
The following commit(s) were added to refs/heads/main by this push:
new 7f19ac55 fix: Fix arm64 assembly stack frame manipulation (#984)
7f19ac55 is described below
commit 7f19ac5583c11002186caa0a4c518f2533320c72
Author: Nick Ripley <[email protected]>
AuthorDate: Thu Jul 23 16:09:08 2026 -0400
fix: Fix arm64 assembly stack frame manipulation (#984)
### Rationale for this change
The arm64 NEON assembly code in this library incorrectly manipulates the
stack pointer. In the mechanically-translated portion of the code, there
are instructions to set up and restore a call frame for the function,
e.g.
WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]!
But the assembler doesn't decode WORD directives and thus doesn't see
the stack pointer manipulation. As a result, the spdelta table entries
for these functions are incorrect. This table is used by the Go runtime
for stack unwinding, to find the next frame based on the program counter
and current stack pointer. In typical cases this means that the CPU
profiler will just stop unwinding when it hits this function, and in
some cases we can get non-truncated but incorrect tracebacks.
See #983
### What changes are included in this PR?
Fix this by removing the unnecssary frame setup for functions which
don't otherwise use the stack, and by changing functions which do need a
stack frame to include their frame size in the function declaration,
which the assembler takes into account.
Note that this PR removes the `SUB/ADD $16, RSP` instructions added in
#170. Those were added because the functions were overwriting values
saved on the stack, but after this PR most of the modified functions
don't save anything on the stack, and the one that does properly
declares its frame size and makes the fix unnecessary. This does mean
that the modified functions with no frames don't set up a frame pointer,
but in practice that's probably fine. If we really want frame pointers
for all these functions, we can give them a small non-zero frame size
and the assembler will add the appropriate frame pointer setup/tearndown
code.
### Are these changes tested?
Yes, by the existing unit tests, and manually by running the CPU
profiler as described in #983
### Are there any user-facing changes?
No
---
.../internal/kernels/cast_numeric_neon_arm64.s | 9 ---
arrow/math/int64_neon_arm64.s | 14 +---
arrow/math/uint64_neon_arm64.s | 14 +---
arrow/memory/memory_neon_arm64.s | 9 ---
arrow/memory/memory_neon_arm64_test.go | 94 ++++++++++++++++++++++
internal/utils/min_max_neon_arm64.s | 48 -----------
parquet/internal/bmi/bitmap_neon_arm64.s | 9 ---
parquet/internal/utils/bit_packing_neon_arm64.s | 40 ++++-----
parquet/internal/utils/unpack_bool_neon_arm64.s | 10 ---
9 files changed, 113 insertions(+), 134 deletions(-)
diff --git a/arrow/compute/internal/kernels/cast_numeric_neon_arm64.s
b/arrow/compute/internal/kernels/cast_numeric_neon_arm64.s
index 3d56efc5..d6cd2070 100644
--- a/arrow/compute/internal/kernels/cast_numeric_neon_arm64.s
+++ b/arrow/compute/internal/kernels/cast_numeric_neon_arm64.s
@@ -10,13 +10,7 @@ TEXT ·_cast_type_numeric_neon(SB), $0-40
MOVD len+32(FP), R4
- // The Go ABI saves the frame pointer register one word below the
- // caller's frame. Make room so we don't overwrite it. Needs to stay
- // 16-byte aligned
- SUB $16, RSP
- WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]!
WORD $0x7100181f // cmp w0, #6
- WORD $0x910003fd // mov x29, sp
BGT LBB0_17
WORD $0x71000c1f // cmp w0, #3
@@ -4450,9 +4444,6 @@ LBB0_892:
WORD $0xb800452b // str w11, [x9], #4
BNE LBB0_892
LBB0_893:
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
LBB0_894:
WORD $0x927b6909 // and x9, x8, #0xffffffe0
diff --git a/arrow/math/int64_neon_arm64.s b/arrow/math/int64_neon_arm64.s
index c0b639bd..6eafdd18 100644
--- a/arrow/math/int64_neon_arm64.s
+++ b/arrow/math/int64_neon_arm64.s
@@ -10,13 +10,7 @@ TEXT ·_sum_int64_neon(SB), $0-24
MOVD buf+0(FP), R0
MOVD len+8(FP), R1
MOVD res+16(FP), R2
-
- // The Go ABI saves the frame pointer register one word below the
- // caller's frame. Make room so we don't overwrite it. Needs to stay
- // 16-byte aligned
- SUB $16, RSP
- WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]!
- WORD $0x910003fd // mov x29, sp
+
CBZ R1, LBB0_3
WORD $0xf1000c3f // cmp x1, #3
BHI LBB0_4
@@ -26,9 +20,6 @@ TEXT ·_sum_int64_neon(SB), $0-24
LBB0_3:
WORD $0xaa1f03e9 // mov x9, xzr
WORD $0xf9000049 // str x9, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
LBB0_4:
WORD $0x927ef428 // and x8, x1, #0xfffffffffffffffc
@@ -59,8 +50,5 @@ LBB0_8:
BNE LBB0_8
LBB0_9:
WORD $0xf9000049 // str x9, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
diff --git a/arrow/math/uint64_neon_arm64.s b/arrow/math/uint64_neon_arm64.s
index 0f8d66a5..4b869972 100644
--- a/arrow/math/uint64_neon_arm64.s
+++ b/arrow/math/uint64_neon_arm64.s
@@ -10,13 +10,7 @@ TEXT ·_sum_uint64_neon(SB), $0-24
MOVD buf+0(FP), R0
MOVD len+8(FP), R1
MOVD res+16(FP), R2
-
- // The Go ABI saves the frame pointer register one word below the
- // caller's frame. Make room so we don't overwrite it. Needs to stay
- // 16-byte aligned
- SUB $16, RSP
- WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]!
- WORD $0x910003fd // mov x29, sp
+
CBZ R1, LBB0_3
WORD $0xf1000c3f // cmp x1, #3
BHI LBB0_4
@@ -26,9 +20,6 @@ TEXT ·_sum_uint64_neon(SB), $0-24
LBB0_3:
WORD $0xaa1f03e9 // mov x9, xzr
WORD $0xf9000049 // str x9, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
LBB0_4:
WORD $0x927ef428 // and x8, x1, #0xfffffffffffffffc
@@ -59,8 +50,5 @@ LBB0_8:
BNE LBB0_8
LBB0_9:
WORD $0xf9000049 // str x9, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
diff --git a/arrow/memory/memory_neon_arm64.s b/arrow/memory/memory_neon_arm64.s
index 18b0af5c..19dd7db3 100644
--- a/arrow/memory/memory_neon_arm64.s
+++ b/arrow/memory/memory_neon_arm64.s
@@ -11,14 +11,8 @@ TEXT ·_memset_neon(SB), $0-24
MOVD len+8(FP), R1
MOVD c+16(FP), R2
- // The Go ABI saves the frame pointer register one word below the
- // caller's frame. Make room so we don't overwrite it. Needs to stay
- // 16-byte aligned
- SUB $16, RSP
- WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]!
WORD $0x8b010008 // add x8, x0, x1
WORD $0xeb00011f // cmp x8, x0
- WORD $0x910003fd // mov x29, sp
BLS LBB0_7
WORD $0xf100803f // cmp x1, #32
@@ -43,7 +37,4 @@ LBB0_6:
WORD $0xeb09011f // cmp x8, x9
BNE LBB0_6
LBB0_7:
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
diff --git a/arrow/memory/memory_neon_arm64_test.go
b/arrow/memory/memory_neon_arm64_test.go
new file mode 100644
index 00000000..c4db665e
--- /dev/null
+++ b/arrow/memory/memory_neon_arm64_test.go
@@ -0,0 +1,94 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file to you under the
+// Apache License, Version 2.0 (the "License"); you may not use this file
except
+// in compliance with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build arm64 && !noasm
+
+package memory_test
+
+import (
+ "os"
+ "os/exec"
+ "runtime/pprof"
+ "strings"
+ "testing"
+ "time"
+
+ "github.com/apache/arrow-go/v18/arrow/memory"
+)
+
+func TestNEONAssemblyTracebacks(t *testing.T) {
+ // This is a regression test for GH-983. _memset_neon used to modify the
+ // stack pointer in a way that the assembler didn't see, leading to
+ // broken tracebacks in CPU profiler samples when the function is
+ // running.
+
+ buf := make([]byte, 1<<20)
+ var traces []byte
+ duration := time.Second
+ d := t.TempDir()
+ for range 5 {
+ f, err := os.CreateTemp(d, "cpu.prof")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := pprof.StartCPUProfile(f); err != nil {
+ f.Close()
+ t.Skipf("CPU profiling is already enabled: %v", err)
+ }
+
+ deadline := time.Now().Add(duration)
+ for time.Now().Before(deadline) {
+ memory.Set(buf, 0x1f)
+ }
+ pprof.StopCPUProfile()
+ if err := f.Close(); err != nil {
+ t.Fatal(err)
+ }
+
+ traces, err = exec.Command("go", "tool", "pprof",
"-symbolize=none", "-traces", f.Name()).CombinedOutput()
+ if err != nil {
+ t.Fatalf("go tool pprof -traces: %v\n%s", err, traces)
+ }
+ if strings.Contains(string(traces), "memory._memset_neon") {
+ break
+ }
+ // Mitigate potential flakiness on CI runners
+ duration *= 2
+ }
+ if !strings.Contains(string(traces), "memory._memset_neon") {
+ t.Fatal("CPU profile contains no _memset_neon samples after 5
attempts")
+ }
+
+ want := strings.Join([]string{
+ "github.com/apache/arrow-go/v18/arrow/memory._memset_neon",
+
"github.com/apache/arrow-go/v18/arrow/memory.memory_memset_neon",
+ "github.com/apache/arrow-go/v18/arrow/memory.Set",
+
"github.com/apache/arrow-go/v18/arrow/memory_test.TestNEONAssemblyTracebacks",
+ }, "\n")
+
+ for trace := range strings.SplitSeq(string(traces), "-----------") {
+ if !strings.Contains(trace, "memory._memset_neon") {
+ continue
+ }
+
+ // pprof adds whitespace and marks inlined functions, neither
of which is
+ // part of the call stack. Remove both before requiring
adjacent frames.
+ trace = strings.ReplaceAll(trace, "(inline)", "")
+ trace = strings.Join(strings.Fields(trace), "\n")
+ if !strings.Contains(trace, want) {
+ t.Errorf("invalid _memset_neon traceback:\n%s", trace)
+ }
+ }
+}
diff --git a/internal/utils/min_max_neon_arm64.s
b/internal/utils/min_max_neon_arm64.s
index 078971d1..a037057e 100644
--- a/internal/utils/min_max_neon_arm64.s
+++ b/internal/utils/min_max_neon_arm64.s
@@ -12,13 +12,7 @@ TEXT ·_int32_max_min_neon(SB), $0-32
MOVD minout+16(FP), R2
MOVD maxout+24(FP), R3
- // The Go ABI saves the frame pointer register one word below the
- // caller's frame. Make room so we don't overwrite it. Needs to stay
- // 16-byte aligned
- SUB $16, RSP
- WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]!
WORD $0x7100043f // cmp w1, #1
- WORD $0x910003fd // mov x29, sp
BLT int32_early_exit
WORD $0x71001c3f // cmp w1, #7
@@ -34,9 +28,6 @@ int32_early_exit:
WORD $0x52b0000b // mov w11, #-2147483648
WORD $0xb900006b // str w11, [x3]
WORD $0xb900004a // str w10, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
int32_neon:
WORD $0x927d7109 // and x9, x8, #0xfffffff8
@@ -78,9 +69,6 @@ int32_scalar_loop:
int32_done:
WORD $0xb900006b // str w11, [x3]
WORD $0xb900004a // str w10, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
// func _uint32_max_min_neon(values unsafe.Pointer, length int, minout, maxout
unsafe.Pointer)
@@ -91,13 +79,7 @@ TEXT ·_uint32_max_min_neon(SB), $0-32
MOVD minout+16(FP), R2
MOVD maxout+24(FP), R3
- // The Go ABI saves the frame pointer register one word below the
- // caller's frame. Make room so we don't overwrite it. Needs to stay
- // 16-byte aligned
- SUB $16, RSP
- WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]!
WORD $0x7100043f // cmp w1, #1
- WORD $0x910003fd // mov x29, sp
BLT uint32_early_exit
WORD $0x71001c3f // cmp w1, #7
@@ -113,9 +95,6 @@ uint32_early_exit:
WORD $0x1280000b // mov w11, #-1
WORD $0xb900006a // str w10, [x3]
WORD $0xb900004b // str w11, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
uint32_neon:
WORD $0x927d7109 // and x9, x8, #0xfffffff8
@@ -157,9 +136,6 @@ uint32_scalar_loop:
uint32_done:
WORD $0xb900006a // str w10, [x3]
WORD $0xb900004b // str w11, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
// func _int64_max_min_neon(values unsafe.Pointer, length int, minout, maxout
unsafe.Pointer)
@@ -170,13 +146,7 @@ TEXT ·_int64_max_min_neon(SB), $0-32
MOVD minout+16(FP), R2
MOVD maxout+24(FP), R3
- // The Go ABI saves the frame pointer register one word below the
- // caller's frame. Make room so we don't overwrite it. Needs to stay
- // 16-byte aligned
- SUB $16, RSP
- WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]!
WORD $0x7100043f // cmp w1, #1
- WORD $0x910003fd // mov x29, sp
BLT int64_early_exit
WORD $0x2a0103e8 // mov w8, w1
@@ -192,9 +162,6 @@ int64_early_exit:
WORD $0xd2f0000b // mov x11, #-9223372036854775808
WORD $0xf900006b // str x11, [x3]
WORD $0xf900004a // str x10, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
int64_neon:
WORD $0x927e7509 // and x9, x8, #0xfffffffc
@@ -246,9 +213,6 @@ int64_scalar_loop:
int64_done:
WORD $0xf900006b // str x11, [x3]
WORD $0xf900004a // str x10, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
@@ -260,13 +224,7 @@ TEXT ·_uint64_max_min_neon(SB), $0-32
MOVD minout+16(FP), R2
MOVD maxout+24(FP), R3
- // The Go ABI saves the frame pointer register one word below the
- // caller's frame. Make room so we don't overwrite it. Needs to stay
- // 16-byte aligned
- SUB $16, RSP
- WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]!
WORD $0x7100043f // cmp w1, #1
- WORD $0x910003fd // mov x29, sp
BLT uint64_early_exit
WORD $0x71000c3f // cmp w1, #3
@@ -282,9 +240,6 @@ uint64_early_exit:
WORD $0x9280000b // mov x11, #-1
WORD $0xf900006a // str x10, [x3]
WORD $0xf900004b // str x11, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
uint64_neon:
WORD $0x927e7509 // and x9, x8, #0xfffffffc
@@ -336,7 +291,4 @@ uint64_scalar_loop:
uint64_done:
WORD $0xf900006a // str x10, [x3]
WORD $0xf900004b // str x11, [x2]
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
diff --git a/parquet/internal/bmi/bitmap_neon_arm64.s
b/parquet/internal/bmi/bitmap_neon_arm64.s
index f711e490..563548d7 100644
--- a/parquet/internal/bmi/bitmap_neon_arm64.s
+++ b/parquet/internal/bmi/bitmap_neon_arm64.s
@@ -8,13 +8,7 @@ TEXT ·_levels_to_bitmap_neon(SB), $0-32
MOVD numLevels+8(FP), R1
MOVD rhs+16(FP), R2
- // The Go ABI saves the frame pointer register one word below the
- // caller's frame. Make room so we don't overwrite it. Needs to stay
- // 16-byte aligned
- SUB $16, RSP
- WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]!
WORD $0x7100043f // cmp w1, #1
- WORD $0x910003fd // mov x29, sp
BLT LBB1_3
WORD $0x71000c3f // cmp w1, #3
@@ -82,9 +76,6 @@ LBB1_7:
WORD $0xaa080168 // orr x8, x11, x8
BNE LBB1_7
LBB1_8:
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
MOVD R8, res+24(FP)
RET
diff --git a/parquet/internal/utils/bit_packing_neon_arm64.s
b/parquet/internal/utils/bit_packing_neon_arm64.s
index b3a02f4c..af1779f1 100644
--- a/parquet/internal/utils/bit_packing_neon_arm64.s
+++ b/parquet/internal/utils/bit_packing_neon_arm64.s
@@ -272,7 +272,7 @@
#define LCPI0_196 $0xfffffffcfffffffe
#define LCPI0_200 $0xfffffffeffffffff
-TEXT ·_unpack32_neon(SB), $0-40
+TEXT ·_unpack32_neon(SB), $496-40
MOVD in+0(FP), R0
MOVD out+8(FP), R1
@@ -281,19 +281,14 @@ TEXT ·_unpack32_neon(SB), $0-40
// LEAQ LCDATA1<>(SB), BP
// %bb.0:
- // The Go ABI saves the frame pointer register one word below the
- // caller's frame. Make room so we don't overwrite it. Needs to stay
- // 16-byte aligned
- SUB $16, RSP
- WORD $0xa9ba7bfd // stp x29, x30, [sp, #-96]!
- WORD $0xd10643e9 // sub x9, sp, #400
- WORD $0xa9016ffc // stp x28, x27, [sp, #16]
- WORD $0xa90267fa // stp x26, x25, [sp, #32]
- WORD $0x910003fd // mov x29, sp
- WORD $0xa9035ff8 // stp x24, x23, [sp, #48]
- WORD $0xa90457f6 // stp x22, x21, [sp, #64]
- WORD $0xa9054ff4 // stp x20, x19, [sp, #80]
- WORD $0x927df13f // and sp, x9, #0xfffffffffffffff8
+ // TODO: do not use x28, x27, or x18 since they are reserved.
+ // It is probably unnecessary to save the rest of these registers since
+ // the ABI doesn't have callee saved registers
+ WORD $0xa91a6ffc // stp x28, x27, [sp, #416]
+ WORD $0xa91b67fa // stp x26, x25, [sp, #432]
+ WORD $0xa91c5ff8 // stp x24, x23, [sp, #448]
+ WORD $0xa91d57f6 // stp x22, x21, [sp, #464]
+ WORD $0xa91e4ff4 // stp x20, x19, [sp, #480]
WORD $0x11007c48 // add w8, w2, #31
WORD $0x7100005f // cmp w2, #0
WORD $0x1a82b108 // csel w8, w8, w2, lt
@@ -6919,14 +6914,13 @@ LBB0_158:
LBB0_156:
WORD $0x531b6a60 // lsl w0, w19, #5
- WORD $0x910003bf // mov sp, x29
- WORD $0xa9454ff4 // ldp x20, x19, [sp, #80]
- WORD $0xa94457f6 // ldp x22, x21, [sp, #64]
- WORD $0xa9435ff8 // ldp x24, x23, [sp, #48]
- WORD $0xa94267fa // ldp x26, x25, [sp, #32]
- WORD $0xa9416ffc // ldp x28, x27, [sp, #16]
- WORD $0xa8c67bfd // ldp x29, x30, [sp], #96
- // Put the stack pointer back where it was
- ADD $16, RSP
+ WORD $0xa95e4ff4 // ldp x20, x19, [sp, #480]
+ WORD $0xa95d57f6 // ldp x22, x21, [sp, #464]
+ WORD $0xa95c5ff8 // ldp x24, x23, [sp, #448]
+ WORD $0xa95b67fa // ldp x26, x25, [sp, #432]
+ WORD $0xa95a6ffc // ldp x28, x27, [sp, #416]
+ // The raw compiler-generated body uses x30 as a scratch register. The
+ // Go-generated prologue saved the return address at the bottom of the
frame.
+ MOVD 0(RSP), R30
MOVD R0, num+32(FP)
RET
diff --git a/parquet/internal/utils/unpack_bool_neon_arm64.s
b/parquet/internal/utils/unpack_bool_neon_arm64.s
index f4ea581e..f8ef8bf6 100644
--- a/parquet/internal/utils/unpack_bool_neon_arm64.s
+++ b/parquet/internal/utils/unpack_bool_neon_arm64.s
@@ -11,13 +11,6 @@ TEXT ·_bytes_to_bools_neon(SB), $0-32
MOVD out+16(FP), R2
MOVD outlen+24(FP), R3
- // The Go ABI saves the frame pointer register one word below the
- // caller's frame. Make room so we don't overwrite it. Needs to stay
- // 16-byte aligned
- SUB $16, RSP
- WORD $0xa9bf7bfd // stp x29, x30, [sp, #-16]!
- WORD $0x910003fd // mov x29, sp
-
WORD $0x7100043f // cmp w1, #1
BLT done
@@ -145,7 +138,4 @@ scalar_next:
JMP scalar_loop
done:
- WORD $0xa8c17bfd // ldp x29, x30, [sp], #16
- // Put the stack pointer back where it was
- ADD $16, RSP
RET
