Repository navigation
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Foreign-ABI ARM64 object translation
Solid, self-contained addition. The go120ld object parser in readNativeObject is notably defensive for a hand-rolled binary decoder: block-offset monotonicity, record-size divisibility (%21/%23), symbol-name range, a symbol-size cap, and monotonic index-table validation all guard the trust boundary. I traced the slice/index paths reachable from TranslateNativeARM64Object (including payload/relocs/target) and found no unguarded out-of-range panic on the malformed and truncated inputs the tests exercise. Symbol names emitted into assembly go through strconv.Quote, which escapes the characters an attacker would need to break out of a quoted operand, so directive injection is not a concern. Test coverage is strong — truncation fuzzing, a tampered-relocation subtest, and a guarded end-to-end clang build.
The findings below are maintainability and edge-case items, not correctness blockers.
Maintainability — magic numbers. The parser is pinned to the go120ld layout via many unexplained literals: field offsets s[13:]/s[17:]/s[11], reloc offsets v[7:]/v[15:]/v[19:], record sizes 21/23, block indices 3..7/11/13/14/16, the reserved package sentinels 0x7ffffffb/0x7ffffffe/0x7ffffffd/0x7fffffff, and flags&16 (NOSPLIT). A future reader can't verify these without the upstream cmd/internal/goobj definitions open alongside, and the format will silently mis-parse if it ever drifts. Named constants (e.g. symRecordSize, symFieldAlign, flagNoSplit, named block indices) referencing the tracked goobj version would be the highest-leverage cleanup.
Minor:
native_arm64_object.go:45-56— the "missing native function" check re-scanslabelsfor every requested name (O(funcs x labels)) and duplicates the matching already done in the first loop. Tracking matched names in the first pass and diffing againstfuncsremoves both the nested scan and the second pass. Immaterial for these tiny inputs; optional cleanup.native_arm64_object_test.go:100,108— the twoos.WriteFilecalls fornative.s/main.cignore their errors, unlike the fixture writer. On a write failure the subsequent clang step reports a confusing error; check andt.Fatalfor consistency.
| } | ||
| return r, nil | ||
| } | ||
| func (r *nativeObject) block(i int) []byte { return r.b[r.blocks[i]:r.blocks[i+1]] } |
There was a problem hiding this comment.
[P2] block(i) accesses blocks[i+1] on a fixed [19]uint32 with no guard
block(i) returns r.b[r.blocks[i]:r.blocks[i+1]]. blocks is [19]uint32 (indices 0..18), and every current caller passes i <= 16, so blocks[i+1] stays in range only by convention. A future block(18) call would index blocks[19] and panic with an out-of-range unrelated to input data. Since the array holds N+1 offsets describing N blocks, consider documenting/asserting that block(i) requires i < 18, or restructuring so the boundary is enforced rather than assumed.
| } else if word&0xfc000000 != 0x14000000 { | ||
| return "", nil, fmt.Errorf("invalid native branch in %s", s.name) | ||
| } | ||
| fmt.Fprintf(&out, "%s %s\n", op, expr) |
There was a problem hiding this comment.
[P2] Nonzero addend on R_CALLARM64 emits likely-invalid bl target+N
The addend is appended to the operand for every relocation kind. For .quad (R_ADDR) an operand like "_sym"+8 is fine, but the R_CALLARM64 branch case can emit bl "_sym"+8 / b "_sym"+8. A branch to a symbol-plus-addend is unusual and many assemblers reject or mishandle it, and the tests only exercise the zero-addend branch path. Consider rejecting rel.add != 0 explicitly for the branch case, or add a test confirming the toolchain accepts it.
| // reconstruct incoming registers. The Go assembler encodes those registers; | ||
| // this bridge only converts its address and branch relocations to Mach-O asm. | ||
| // | ||
| // The input format is cmd/internal/goobj's go120ld format. Only explicitly |
There was a problem hiding this comment.
[P3] Doc comment omits the Darwin/ARM64 header requirement
The comment states the input is "cmd/internal/goobj's go120ld format" but readNativeObject additionally hard-requires the "go object darwin arm64 " header and rejects any other GOOS/GOARCH. A caller reading only this comment might expect any go120ld object to be accepted. The README does say "Darwin/ARM64 Go object"; aligning the doc comment would remove the inconsistency.
2 participants
TypeScript's Darwin FSEvents backend uses raw ARM64 C callbacks and trampolines without Go function declarations. Typed LLVM lowering cannot reconstruct their register ABI: ordinary dynamic calls fail without declarations, while inventing a void signature would lose arguments/results.
Add an explicit native-object path for compiler drivers.
ForeignARM64Functionsrecognizes file-local NOSPLIT stubs with zero Go frames (callbacks containing calls require NOFRAME).TranslateNativeARM64Objectreads the Go assembler's go120ld object, preserves machine instructions, and converts R_ADDR / R_CALLARM64 relocations to Mach-O assembly. Calls must resolve to selected local definitions or driver-supplied dynamic imports. It also returns DATA definitions so drivers can bind their Go globals. Unknown object formats, references and relocations fail explicitly.The driver retains responsibility for invoking the Go assembler and native compiler, choosing the target, and linking libraries; LLGo integration is tracked in xgo-dev/llgo#2623.
Validation on macOS arm64, Go 1.27, LLVM 22:
go test ./ -run 'Test(NativeARM64Object|ForeignARM64Selection|TranslateGoModule)' -count=1passed.tscat c975de5011fb7dfb32a491cf3fcf02d4f811f50e; its emitted JavaScript and type-error diagnostic match standard Go's build.The first supported native-object target is Darwin/ARM64. The API remains available to drivers hosted on other architectures, and native execution checks are host-gated.
Review/CI follow-up: named go120ld layout constants, explicit rejection of nonzero branch addends, checked fixture writes, and malformed-object regression tests. Native object file statement coverage is 99.4% locally and
go test ./...passes. Refreshed the stale lz4 corpus to v4.1.30; all 11 target checks pass.