the interpreter's [extern] call marshals through an arm64 layout past six arguments, and the SystemV wrapper's float mask and extra-table result class are fixed - #3983
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new src/builtin/ARCHITECTURE.md section contains an inaccurate statement about when tests/dasbind is skipped vs when the probe is built, and it should be corrected to match the actual build/skip conditions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes ABI mismatches in the interpreter path for [extern] calls: arm64 now uses an explicit layout + trampoline for calls that run past the first six arguments, and the x86-64 SystemV wrapper selection/extra-table handling is corrected (including result-class handling).
Changes:
- Add an arm64-specific marshaling path (
Arm64Layout+das_arm64_call) for interpreted[extern]calls with >6 arguments on aarch64 Apple/Linux. - Fix SystemV wrapper selection (mask all six register positions), correct extra-wrapper result class handling, and extend the extra wrapper table for newly-supported shapes.
- Add a dedicated
tests/dasbindprobe shared library + ABI tests (and integrate into CMake and the test/AOT suite selection).
File summaries
| File | Description |
|---|---|
| tests/REVIEW.md | Adds a tests checklist item to prevent weakening the new dasbind ABI coverage. |
| tests/dasbind/test_extern_abi.das | New interpreter/JIT ABI probes validating register/stack argument placement and re-apply behavior. |
| tests/dasbind/probe/dasbind_probe.cpp | C++ probe library implementing deterministic “sum under multipliers” signatures for ABI verification. |
| tests/dasbind/CMakeLists.txt | Builds the probe shared library and places it where dasbind’s loader can find it. |
| tests/dasbind/_reapply_fixture.das | Fixture that re-applies the extern in-process to validate layout lifetime assumptions. |
| tests/aot/CMakeLists.txt | Adds dasbind to the AOT suites list (with the test file itself marked no_aot). |
| tests/.das_test | Gates visiting tests/dasbind based on pointer size to avoid silent skips on 32-bit. |
| src/builtin/systemV_64_extra_wrapper.inc | Fixes extra wrappers’ return types/result-class handling and adds new extra-wrapper shapes. |
| src/builtin/REVIEW.md | Extends review rules to cover any addExtern* binds and requires ARCHITECTURE updates for wrapper/layout changes. |
| src/builtin/module_builtin_dasbind.cpp | Implements arm64 layout/trampoline path and fixes SystemV wrapper mask + extra-table threshold/result-class wiring. |
| src/builtin/generate_x86_64_calls.das | Updates result-class constants and extends the extra wrapper generator inputs. |
| src/builtin/ARCHITECTURE.md | Documents the interpreter [extern] call mechanism and the new arm64 layout/trampoline behavior. |
| CMakeLists.txt | Adds tests/dasbind to the build so the probe library is produced in CI/local builds. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
684bf8e to
05e0c04
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes low-level cross-ABI extern-call mechanics (including inline arm64 assembly) where small mistakes can cause subtle miscalls or crashes across platforms.
Review details
Suppressed comments (2)
tests/dasbind/test_extern_abi.das:69
equaltakes expected first and got second; swap arguments here so failures report the computed extern-call value as "got".
t |> equal(probe_ref_args(fv, 1l, 2l, 3l, 4l, 5l, 6l, 7l, iv), 269l)
tests/dasbind/test_extern_abi.das:80
equaltakes expected first and got second; swap arguments here so failures report the computed extern-call value as "got".
t |> equal(probe_stack14(int8(1), int16(2), 3, 4l, 0.5, 0.25lf, 6, int8(7), int8(8), int16(9), 10, 11l, true, 13), 5576l)
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
05e0c04 to
285d5f6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new arm64 return-path currently relies on an implementation-defined unsigned→signed conversion when preserving raw return bits, which should be made explicitly bit-preserving.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
285d5f6 to
98afff8
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes low-level cross-platform calling-convention behavior (including new inline arm64 assembly), which warrants final human review despite the added test coverage.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
98afff8 to
196d790
Compare
There was a problem hiding this comment.
🟡 Changes recommended
tests/dasbind/_reapply_fixture.das should be marked options no_aot (it can be picked up by the AOT sweep once dasbind is added as a suite, and its [extern] bindings are noAot).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
196d790 to
ac4f2d3
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes low-level cross-platform ABI marshalling and introduces inline arm64 assembly, which warrants final human review despite added tests.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
ac4f2d3 to
6e80bfa
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes low-level cross-platform ABI/assembly call mechanics for interpreted [extern] and should get final human validation despite the added test coverage.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
6e80bfa to
636a55b
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The changes touch cross-ABI call marshalling and introduce inline arm64 assembly, which warrants final human verification despite added probe coverage.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
636a55b to
4b32b90
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes low-level ABI marshalling/trampoline behavior across multiple platforms and warrants a final human review for ABI correctness on all target environments.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
4b32b90 to
4f058ad
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new arm64 layout path misclassifies float&/double& return types as FP results (baseType-only check), which would route the return through d0 instead of x0 and can yield incorrect results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
… six arguments - the generated C wrappers spell every stack argument as an 8-byte slot and mask six float positions, while Apple packs stack arguments at natural size and every arm64 has eight float registers, so LLVMDIBuilderCreateFunction's tail arguments read one slot late on every Mac (the debug-info test red since #3981) and a float in position 6 or 7 rode an integer register on Linux and Android arm64; the layout maps each argument to x0-x7, d0-d7 or a byte offset of a stack image (packed on Apple, slotted on the GNU ABI), an assembly trampoline copies the image and calls, and layouts live for the process beside the bound-function table since a re-applied extern replaces the function object an earlier context's node still names; two table defects fall with it - the wrapper choice read the float mask of the first four SystemV positions instead of six, so a double in position 4 or 5 went through an integer register on every SystemV host, and the extra table's result class was inverted against the main table's, so its hand-listed GL wrappers returned rax where the callee's xmm0 held the value - and a float in position 6 now routes to the extra table instead of being dropped by the six-bit mask; tests/dasbind probes every shape against a library of functions that sum their arguments under distinct multipliers, built into lib/ by its own CMake, gated by the .das_test walk on the library's presence and a 64-bit host, registered in the AOT suites as a no_aot file, with its two SystemV shapes listed in the generator; src/builtin/ARCHITECTURE.md sec.3 carries the mechanism and REVIEW.md the three-box duty Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
4f058ad to
6fbe5ca
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes low-level cross-platform ABI marshaling and introduces new inline arm64 assembly/trampoline behavior, which warrants final human review despite the added probe coverage.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
Behavior change: an interpreted
[extern]with a float in position 6 on x86-64 Linux now refuses to compile unless its shape is listed in the extra wrapper table - it was silently miscalled before; the five dasOpenGL functions with that shape are listed here.Why. The interpreter calls an
[extern]through a generated C prototype that spells every stack argument as an 8-byte slot and masks six float positions, which is wrong on arm64 and was mis-indexed on SystemV: on every MacLLVMDIBuilderCreateFunction's tail arguments read one slot late (the debug-info test red since #3981), a double in position 4 or 5 rode an integer register on every SystemV host, the extra table's hand-listed wrappers readraxwherexmm0held the result, and afloat&ordouble&argument was classed by its base type and passed as a float instead of a pointer. The JIT emits typed calls and was never affected.What changes.
Arm64Layout(packed stack on Apple, 8-byte slots on the GNU ABI) and thedas_arm64_callassembly trampoline; layouts live for the process, since a re-applied extern replaces the function object under a live call node.tests/dasbindbinds a probe library built intolib/; each function sums its arguments under distinct multipliers, so one argument in the wrong register or slot changes the total.Observable behavior.
modules/dasLLVM/tests/llvm_jit_debug_info.dason Apple arm64: red -> green.GLenumon x86-64 Linux: garbage -> the value.float const&argument on Windows or x86-64 Linux: a crash on a garbage pointer -> the value.testswalk on a 64-bit tree without the probe library: fails the suite instead of skipping it.Where to look.
module_builtin_dasbind.cpp: the trampoline andcomputeArm64Layout,getWrapper's mask loop, the tworescomputations;generate_x86_64_calls.dasfor the result-class constants.Validation, claims, ledger
Validation
tests/dasbind11/11 interpreted and JIT,modules/dasLLVM/tests110/110;modules/dasOpenGL/opengl/opengl_func.dascompiles on x86-64 with zero refusals after listing the five shapes. The reference-class defect was found by CI's Linux Debug lane on this PR's own probe and reproduced on x86-64 Release too.1<<7extra-table threshold (the position-6 arm on x86-64), a dropped extra row (compile refusal on x86-64), and master's dasbind sources against the final suite (compile refusal on every ABI).Not done
[extern]does not survive dastest's--ser/--deserround trip: the reader never applies the annotation, so the__dasbind__function it names is missing from thedasbindmodule and the stream cascades. Pre-existing; the suite skips that sweep.