Merge branch 'flow/sx-plan-arch/fix-0074' into arch-refactor
This commit is contained in:
84
issues/0074-ffi-arg-type-void-fallback.md
Normal file
84
issues/0074-ffi-arg-type-void-fallback.md
Normal file
@@ -0,0 +1,84 @@
|
||||
# 0074 — silent `getRefIRType(arg_ref) orelse .void` fallback in FFI call-arg lowering
|
||||
|
||||
> **✅ RESOLVED.** Root cause: four FFI call-arg lowering loops resolved an
|
||||
> argument's IR type via `getRefIRType(arg_ref) orelse .void` — a silent fallback
|
||||
> to the load-bearing real type `.void`, which downstream `toLLVMType` →
|
||||
> `abiCoerceParamType` → `coerceArg` treat as a legitimate (void-typed) foreign
|
||||
> argument, corrupting the call ABI with no diagnostic. Fix: one shared resolver
|
||||
> `LLVMEmitter.argIRTypeOrFail` ([src/ir/emit_llvm.zig]) returns the dedicated
|
||||
> `.unresolved` sentinel on a failed lookup — never `.void`/`.s64` — so the failure
|
||||
> cannot masquerade as a real type and trips `toLLVMType`'s existing hard `@panic`
|
||||
> tripwire at the call site. All four sites
|
||||
> ([src/ir/emit_llvm.zig] JNI constructor; [src/backend/llvm/ops.zig] objc_msgSend,
|
||||
> JNI non-virtual, JNI `Call<Type>Method`) now route through the helper. Happy path
|
||||
> is byte-identical (every real arg already has a resolved type) — FFI examples stay
|
||||
> green with zero snapshot churn. Regression test (fail-before/pass-after):
|
||||
> `src/ir/emit_llvm.test.zig` — "argIRTypeOrFail surfaces .unresolved for an
|
||||
> unresolvable FFI arg ref (issue 0074)".
|
||||
|
||||
## Symptom
|
||||
**One-line:** Four FFI call-arg lowering sites silently default a failed
|
||||
argument-type lookup to `.void` — the forbidden silent-type-fallback anti-pattern
|
||||
(`.void` as a failed-type-lookup sentinel), which would produce a void-typed
|
||||
foreign-call argument (wrong LLVM param type → silent ABI corruption) with no
|
||||
diagnostic if the lookup ever fails.
|
||||
|
||||
**Observed:** `self.getRefIRType(arg_ref) orelse .void` at:
|
||||
- `src/ir/emit_llvm.zig:2463`
|
||||
- `src/backend/llvm/ops.zig:517` (Obj-C `objc_msgSend` arg loop)
|
||||
- `src/backend/llvm/ops.zig:731` (JNI non-virtual call arg loop)
|
||||
- `src/backend/llvm/ops.zig:761` (JNI `Call<Type>Method` arg loop)
|
||||
|
||||
Each then does `toLLVMType(raw_ty)` → `abiCoerceParamType` → `coerceArg`, so a
|
||||
`.void` fallback silently mis-types the foreign-call argument.
|
||||
|
||||
**Expected:** `getRefIRType` returning null for a real call argument is a
|
||||
"must-succeed lookup" failure (every arg is a valid param/instruction ref). Per
|
||||
`CLAUDE.md` REJECTED PATTERNS — *"`.void` is an UNACCEPTABLE sentinel for a failed
|
||||
type lookup"* — the lookup failure must surface as a diagnostic / hard tripwire, not
|
||||
a silently-corrupted argument type.
|
||||
|
||||
## Provenance / scope
|
||||
Pre-existing pattern (the `emit_llvm.zig` site is original; the three `ops.zig` sites
|
||||
were relocated **verbatim, behavior-preserving** by step A7.4c of the arch-refactor —
|
||||
the flow reviewer/observer correctly approved the relocations as equivalence-preserving).
|
||||
Surfaced by the **A9.3 final fallback-audit** of the arch-refactor stream. Not
|
||||
introduced by the refactor; filed per the IMPASSIBLE RULE (*"If you find an existing
|
||||
default-return in the compiler that swallows a lookup failure, treat it as a
|
||||
discovered bug — file an issue, do not just delete the default in place"*).
|
||||
|
||||
## Reproduction
|
||||
This is a **latent / static** finding: there is no known sx program that drives
|
||||
`getRefIRType` to `null` for a valid foreign-call argument (well-formed IR always
|
||||
has a type for every arg ref), so it cannot currently be triggered at runtime — which
|
||||
is exactly why it is dangerous (a future IR change that breaks the invariant would
|
||||
corrupt FFI ABI silently). The code paths are exercised (and must stay green after
|
||||
the fix) by the existing FFI examples, e.g.:
|
||||
|
||||
```
|
||||
examples/13xx-ffi-objc-* # objc_msgSend arg lowering (ops.zig:517)
|
||||
examples/14xx-ffi-jni-* # JNI Call<Type>Method / non-virtual (ops.zig:731/761)
|
||||
```
|
||||
|
||||
(No new minimal repro `.sx` is meaningful for a latent defensive fallback; the fix is
|
||||
verified by (a) the FFI suite staying green and (b) a unit test that asserts the new
|
||||
loud-failure path, see below.)
|
||||
|
||||
## Investigation prompt (ready to paste into a fresh session)
|
||||
> In `/Users/agra/projects/sx`, four FFI call-arg lowering sites use the forbidden
|
||||
> silent type-fallback `self.getRefIRType(arg_ref) orelse .void`
|
||||
> (`src/ir/emit_llvm.zig:2463`; `src/backend/llvm/ops.zig:517`, `:731`, `:761`).
|
||||
> `getRefIRType` (`src/ir/emit_llvm.zig:2229`, returns `?TypeId`) yields `null` only
|
||||
> when a ref is neither a function param nor a block instruction result — a
|
||||
> must-not-happen case for a real call argument. Replace the silent `.void` default
|
||||
> with a loud failure that cannot be mistaken for a real type, per `CLAUDE.md`
|
||||
> REJECTED PATTERNS: emit a diagnostic via `self.diagnostics.addFmt(.err, span,
|
||||
> "...", .{...})` and/or a hard tripwire (`@panic`/`bailDetail`-style) naming the op
|
||||
> and the bad ref — do NOT substitute another real type. Prefer a single shared
|
||||
> helper (e.g. `argIRTypeOrFail(arg_ref, span)`) used by all four sites so the policy
|
||||
> lives in one place. Then: (1) `/Users/agra/.zvm/bin/zig build && /Users/agra/.zvm/bin/zig
|
||||
> build test && bash tests/run_examples.sh` must stay 361/0 with the FFI examples
|
||||
> green (the happy path is unchanged); (2) add a `*.test.zig` unit test that
|
||||
> constructs an FFI call op with a bogus arg ref and asserts the loud failure fires
|
||||
> (not a `.void` silent default). Expected new behavior: an unresolved FFI arg type
|
||||
> produces a clear compiler error / panic, never a void-typed foreign argument.
|
||||
77
issues/0075-reflection-builtin-s64-type-fallback.md
Normal file
77
issues/0075-reflection-builtin-s64-type-fallback.md
Normal file
@@ -0,0 +1,77 @@
|
||||
# 0075 — silent `getRefIRType(...) orelse TypeId.s64` fallback in reflection builtins
|
||||
|
||||
## Symptom
|
||||
**One-line:** The `type_name` and `type_eq` reflection builtins resolve their Type
|
||||
argument's IR type via `getRefIRType(...) orelse TypeId.s64` — the forbidden
|
||||
silent-type-lookup fallback (`.s64` is the exact issue-0042 sentinel the project
|
||||
rules name) — so a failed must-succeed lookup silently decides "not boxed (`!= .any`)"
|
||||
and mis-handles the value with no diagnostic.
|
||||
|
||||
**Observed (primary — must fix):** `self.e.getRefIRType(...) orelse TypeId.s64` at:
|
||||
- `src/backend/llvm/ops.zig:1023` (`.type_name` builtin — `arg_ir_ty`, gates the
|
||||
`== .any` boxed-extract vs bare-i64 decision)
|
||||
- `src/backend/llvm/ops.zig:1049` (`.type_eq` builtin — first operand)
|
||||
- `src/backend/llvm/ops.zig:1055` (`.type_eq` builtin — second operand)
|
||||
|
||||
`getRefIRType` (`src/ir/emit_llvm.zig:2229`, `?TypeId`) returns `null` only when a ref
|
||||
is neither a function param nor a block instruction result — a must-not-happen case
|
||||
for a real builtin argument. On `null` the code defaults to `.s64`, then tests
|
||||
`arg_ir_ty == .any`; the `.s64` default silently means "treat as a bare TypeId index,
|
||||
not a boxed `Any`", so a genuinely-boxed arg whose lookup failed would skip the
|
||||
`ExtractValue` and use the wrong value — silent miscompile, no diagnostic.
|
||||
|
||||
**Expected:** per `CLAUDE.md` REJECTED PATTERNS, a failed must-succeed type lookup
|
||||
surfaces a diagnostic / hard tripwire (e.g. the `.unresolved` sentinel introduced for
|
||||
issue 0074), never a real-type default.
|
||||
|
||||
## Secondary (confirm — borderline)
|
||||
- `src/ir/lower.zig:2527` — `.null_literal => constNull(self.target_type orelse .void)`
|
||||
- `src/ir/lower.zig:2528` — `.undef_literal => constUndef(self.target_type orelse .void)`
|
||||
`target_type` is a context hint that may be legitimately absent for a bare
|
||||
`null`/`undef` with no expected type — this may be an INTENTIONAL default rather
|
||||
than a lookup-swallow. The fix session should confirm: if a `null`/`undef` literal
|
||||
reaching here without a `target_type` is actually a must-not-happen case, make it
|
||||
loud; if a typeless null/undef is legitimate, leave it and add a one-line comment
|
||||
stating the invariant.
|
||||
|
||||
## Audited — intentional language defaults (NO action; documented so they aren't re-flagged)
|
||||
- `src/ir/lower.zig:4855` — `int_literal => constInt(lit.value, info.ty orelse .s64)`:
|
||||
an untyped integer literal defaulting to `s64` is standard language semantics, not a
|
||||
lookup failure.
|
||||
- `src/ir/lower.zig:4856` — `float_literal => constFloat(..., info.ty orelse .f64)`:
|
||||
untyped float literal defaults to `f64` — language semantics.
|
||||
- `src/ir/type_bridge.zig:334` — `.tag_type = tag_type orelse .s64`: documented
|
||||
("enum unions are always tagged (default i64)") — an intentional default tag type,
|
||||
not a swallowed lookup.
|
||||
|
||||
## Provenance / scope
|
||||
Pre-existing, NOT introduced by the arch-refactor. Discovered during the **issue-0074
|
||||
fix** (the fix worker surfaced the reflection `.s64` fallbacks as a separate pattern
|
||||
outside 0074's FFI-arg scope) and confirmed by a manager sweep
|
||||
(`rg "orelse \.(s64|void|...)" src`). Filed per the IMPASSIBLE RULE (existing
|
||||
default-returns that swallow a lookup failure → file, don't fix in place).
|
||||
|
||||
## Reproduction
|
||||
Latent / static (same nature as 0074): well-formed IR always gives a builtin arg a
|
||||
resolvable type, so the `.s64` default can't be driven at runtime today — which is why
|
||||
it's dangerous (a future IR change would silently miscompile `type_name`/`type_eq`).
|
||||
Exercised by the comptime/reflection examples; the fix must keep the suite at 361/0.
|
||||
|
||||
## Investigation prompt (ready to paste into a fresh session)
|
||||
> In `/Users/agra/projects/sx`, the `.type_name` and `.type_eq` reflection builtins in
|
||||
> `src/backend/llvm/ops.zig` (lines 1023, 1049, 1055) resolve a Type argument's IR type
|
||||
> with the forbidden silent fallback `getRefIRType(...) orelse TypeId.s64`, used to gate
|
||||
> a `== .any` boxed-vs-bare decision. Issue 0074 already added the shared resolver
|
||||
> `LLVMEmitter.argIRTypeOrFail` (`src/ir/emit_llvm.zig`) returning the dedicated
|
||||
> `.unresolved` sentinel on a failed lookup. Route these three sites through that helper
|
||||
> (or a sibling) so a failed lookup yields `.unresolved` — never `.s64`; then `==.any`
|
||||
> is false for `.unresolved` AND you must make the unresolved case loud (diagnostic via
|
||||
> `self.diagnostics.addFmt(.err, span, ...)` or a hard tripwire), not silently "bare
|
||||
> i64". Also resolve the borderline `lower.zig:2527/2528` `target_type orelse .void`
|
||||
> (confirm intentional vs make-loud; comment the invariant either way). Leave the
|
||||
> audited intentional defaults (`lower.zig:4855/4856`, `type_bridge.zig:334`) untouched.
|
||||
> Verify: `/Users/agra/.zvm/bin/zig build && /Users/agra/.zvm/bin/zig build test &&
|
||||
> bash tests/run_examples.sh` stays 361/0; add a `*.test.zig` regression test asserting
|
||||
> the loud `.unresolved` path for a `type_name`/`type_eq` arg with an unresolvable ref
|
||||
> (fail-before/pass-after). Expected new behavior: an unresolved reflection-builtin arg
|
||||
> type surfaces loudly, never silently defaults to `.s64`.
|
||||
@@ -514,7 +514,7 @@ pub const Ops = struct {
|
||||
// coercion applied so structs / strings decay the
|
||||
// same way they do for any C foreign call.
|
||||
for (msg.args, 0..) |arg_ref, i| {
|
||||
const raw_ty = self.e.getRefIRType(arg_ref) orelse .void;
|
||||
const raw_ty = self.e.argIRTypeOrFail(arg_ref);
|
||||
const raw_llvm = self.e.toLLVMType(raw_ty);
|
||||
const coerced_ty = self.e.abiCoerceParamType(raw_ty, raw_llvm);
|
||||
param_types[i + 2 + sret_off] = coerced_ty;
|
||||
@@ -728,7 +728,7 @@ pub const Ops = struct {
|
||||
call_args_nv[2] = cls;
|
||||
call_args_nv[3] = mid_val;
|
||||
for (msg.args, 0..) |arg_ref, i| {
|
||||
const raw_ty = self.e.getRefIRType(arg_ref) orelse .void;
|
||||
const raw_ty = self.e.argIRTypeOrFail(arg_ref);
|
||||
const raw_llvm = self.e.toLLVMType(raw_ty);
|
||||
const coerced_ty = self.e.abiCoerceParamType(raw_ty, raw_llvm);
|
||||
call_param_types_nv[i + 4] = coerced_ty;
|
||||
@@ -758,7 +758,7 @@ pub const Ops = struct {
|
||||
call_args[1] = target;
|
||||
call_args[2] = mid;
|
||||
for (msg.args, 0..) |arg_ref, i| {
|
||||
const raw_ty = self.e.getRefIRType(arg_ref) orelse .void;
|
||||
const raw_ty = self.e.argIRTypeOrFail(arg_ref);
|
||||
const raw_llvm = self.e.toLLVMType(raw_ty);
|
||||
const coerced_ty = self.e.abiCoerceParamType(raw_ty, raw_llvm);
|
||||
call_param_types[i + 3] = coerced_ty;
|
||||
|
||||
@@ -1097,3 +1097,50 @@ test "emit: ERR E3.0 — no DWARF without a debug context (unit-test default)" {
|
||||
try std.testing.expect(std.mem.indexOf(u8, ir_str, "DICompileUnit") == null);
|
||||
try std.testing.expect(std.mem.indexOf(u8, ir_str, "!dbg") == null);
|
||||
}
|
||||
|
||||
// ── issue 0074: FFI arg-type lookup must fail loudly, never silently `.void` ──
|
||||
// `argIRTypeOrFail` backs the four FFI call-arg lowering sites (objc_msgSend,
|
||||
// JNI Call<Type>Method / non-virtual / constructor). A ref it cannot resolve is
|
||||
// a codegen invariant violation; it must surface the dedicated `.unresolved`
|
||||
// tripwire sentinel (which `toLLVMType` hard-panics on) rather than the old
|
||||
// silent `.void` default that would emit a void-typed foreign-call argument.
|
||||
test "emit: argIRTypeOrFail surfaces .unresolved for an unresolvable FFI arg ref (issue 0074)" {
|
||||
const alloc = std.testing.allocator;
|
||||
var module = Module.init(alloc);
|
||||
defer module.deinit();
|
||||
|
||||
var b = Builder.init(&module);
|
||||
|
||||
// func ffifn(a: s64, b: f64) -> void { <entry> }
|
||||
const fid = b.beginFunction(str(&module, "ffifn"), &[_]Function.Param{
|
||||
.{ .name = str(&module, "a"), .ty = .s64 },
|
||||
.{ .name = str(&module, "b"), .ty = .f64 },
|
||||
}, .void);
|
||||
const entry = b.appendBlock(str(&module, "entry"), &.{});
|
||||
b.switchToBlock(entry);
|
||||
b.retVoid();
|
||||
b.finalize();
|
||||
|
||||
var emitter = LLVMEmitter.init(alloc, &module, "test_ffi_argty", .{});
|
||||
defer emitter.deinit();
|
||||
emitter.current_func_idx = fid.index();
|
||||
|
||||
// Happy path: a real arg ref (param 0 / param 1) resolves byte-identically
|
||||
// to its declared IR type — the FFI fast path is unchanged.
|
||||
try std.testing.expectEqual(TypeId.s64, emitter.argIRTypeOrFail(Ref.fromIndex(0)));
|
||||
try std.testing.expectEqual(TypeId.f64, emitter.argIRTypeOrFail(Ref.fromIndex(1)));
|
||||
|
||||
// A ref past every param and instruction is unresolvable.
|
||||
const bogus = Ref.fromIndex(100_000);
|
||||
try std.testing.expectEqual(@as(?TypeId, null), emitter.getRefIRType(bogus));
|
||||
|
||||
// Fail-before: the old `getRefIRType(arg) orelse .void` would silently
|
||||
// yield `.void` here — a real, load-bearing type that downstream ABI
|
||||
// coercion treats as a legitimate (void-typed) foreign argument.
|
||||
try std.testing.expectEqual(TypeId.void, emitter.getRefIRType(bogus) orelse TypeId.void);
|
||||
|
||||
// Pass-after: the helper returns the dedicated `.unresolved` sentinel,
|
||||
// never `.void`, so the failure cannot masquerade as a real type.
|
||||
try std.testing.expectEqual(TypeId.unresolved, emitter.argIRTypeOrFail(bogus));
|
||||
try std.testing.expect(emitter.argIRTypeOrFail(bogus) != .void);
|
||||
}
|
||||
|
||||
@@ -2239,6 +2239,16 @@ pub const LLVMEmitter = struct {
|
||||
return null;
|
||||
}
|
||||
|
||||
/// Resolve the IR type of a foreign-call argument ref. Every FFI arg ref is
|
||||
/// a real function param or block instruction result, so a `null` here is a
|
||||
/// codegen invariant violation, not a recoverable case: return the dedicated
|
||||
/// `.unresolved` sentinel — never `.void`/`.s64` — so the failure cannot be
|
||||
/// mistaken for a real type and trips `toLLVMType`'s hard tripwire at the call
|
||||
/// site instead of silently emitting a void-typed foreign argument.
|
||||
pub fn argIRTypeOrFail(self: *LLVMEmitter, arg_ref: Ref) TypeId {
|
||||
return self.getRefIRType(arg_ref) orelse .unresolved;
|
||||
}
|
||||
|
||||
|
||||
/// Coerce both binary operands to match the instruction's result type.
|
||||
/// E.g. if result is i64 but one operand is i32, sext it.
|
||||
@@ -2460,7 +2470,7 @@ pub const LLVMEmitter = struct {
|
||||
call_args[1] = cls;
|
||||
call_args[2] = mid;
|
||||
for (msg.args, 0..) |arg_ref, i| {
|
||||
const raw_ty = self.getRefIRType(arg_ref) orelse .void;
|
||||
const raw_ty = self.argIRTypeOrFail(arg_ref);
|
||||
const raw_llvm = self.toLLVMType(raw_ty);
|
||||
const coerced_ty = self.abiCoerceParamType(raw_ty, raw_llvm);
|
||||
call_param_types[i + 3] = coerced_ty;
|
||||
|
||||
Reference in New Issue
Block a user