diff --git a/examples/issue-0044.sx b/examples/issue-0044.sx new file mode 100644 index 0000000..0190dbd --- /dev/null +++ b/examples/issue-0044.sx @@ -0,0 +1,110 @@ +// issue-0044 — `xx this` inside a BOOL-returning #objc_class method +// truncated to i8 when used as an argument to a foreign-class method +// call. Root cause was resolveCallParamTypes returning an empty slice +// for foreign-class method receivers (the type was a `#foreign +// #objc_class` alias), so the per-arg target_type defaulted to the +// enclosing function's return type (BOOL → i8) and the `xx ptr` cast +// silently truncated the pointer to its low byte. +// +// Verification: every probe must round-trip the receiver pointer. A +// pre-fix compiler reads the LOW BYTE of `xx this` at the +// `addObserver:selector:name:object:` call site, so the observer +// would be e.g. 0xC0/0x20 instead of the real pointer. +#import "modules/std.sx"; +#import "modules/std/objc.sx"; +#import "modules/compiler.sx"; + +g_observer : *void = null; + +// Stand-in for NSNotificationCenter — we just need a foreign-class +// method with several *void args so the call site's arg-target-type +// resolution exercises the same path as uikit.sx's keyboard observer. +SxIssue44Bus :: #foreign #objc_class("NSNotificationCenter") { + defaultCenter :: () -> *SxIssue44Bus; + addObserver_selector_name_object :: (self: *Self, observer: *void, sel: *void, name: *void, obj: *void); +} + +SxIssue44Foo :: #objc_class("SxIssue44Foo") { + counter: s32; + sentinel: *void; + + alloc :: () -> *SxIssue44Foo; + + bump :: (this: *Self) { + this.counter += 1; + } + + get :: (this: *Self) -> s32 { + return this.counter; + } + + // Return the receiver — direct `xx this` round-trip. + me :: (this: *Self) -> *void { + return xx this; + } + + // SxAppDelegate-shape: BOOL return + 2 extra *void args. Pre-fix, + // the call to addObserver:... would receive `xx this` truncated to + // its low byte (because resolveCallParamTypes returned `&.{}` for + // foreign-class receivers and `self.target_type` leaked the BOOL + // return type into the call's args). + appDelegate_options :: (this: *Self, app: *void, opts: *void) -> BOOL { + bus := SxIssue44Bus.defaultCenter(); + bus.addObserver_selector_name_object( + xx this, + xx 0, + xx 0, + null); + return 1; + } + + // Same shape but captures the observer-equivalent value to a global + // so we can read it back without going through NSNotificationCenter + // (which would crash with a real observer != NSObject subclass). + captureSelf_options :: (this: *Self, app: *void, opts: *void) -> BOOL { + capture_observer(xx this); + return 1; + } +} + +capture_observer :: (p: *void) { + g_observer = p; +} + +main :: () -> s32 { + inline if OS == .macos { + f := SxIssue44Foo.alloc(); + if f == null { print("FAIL: alloc returned null\n"); return 1; } + + f.bump(); + f.bump(); + f.bump(); + print("counter: {}\n", f.get()); + + // Direct `xx this` round-trip (worked pre-fix). + f_void : *void = xx f; + if f.me() == f_void { + print("me: ok\n"); + } else { + print("me: WRONG\n"); + } + + // The actual repro: BOOL return + foreign-class method call. + // Pre-fix: `xx this` truncated to i8, capture_observer receives + // (low_byte_of_f) cast back to *void, which won't equal f_void. + g_observer = null; + _ = f.captureSelf_options(xx 0, xx 0); + if g_observer == f_void { + print("captureSelf-from-BOOL: ok\n"); + } else { + print("captureSelf-from-BOOL: WRONG\n"); + } + + // Also exercise the compile-only path — appDelegate_options' IR + // must pass `ptr` args to objc_msgSend (not `i8`). We don't + // dispatch this for real (would crash inside NSNotificationCenter), + // but the build pulling cleanly through is half the point. + } + inline if OS != .macos { print("skipped (not macos)\n"); } + 0; +} diff --git a/issues/0044-sx-defined-objc-class-method-self-param-must-be-named-self.md b/issues/0044-sx-defined-objc-class-method-self-param-must-be-named-self.md new file mode 100644 index 0000000..92d289d --- /dev/null +++ b/issues/0044-sx-defined-objc-class-method-self-param-must-be-named-self.md @@ -0,0 +1,184 @@ +**FIXED.** Root cause was NOT the parameter name; the original `this` +rename surfaced an unrelated `target_type` leak in +`resolveCallParamTypes`. See "Root cause + fix" below. + +## Symptom + +In an `#objc_class` method body, the first `*Self` parameter MUST be +named `self`. Renaming to anything else (e.g. `this`) compiles cleanly +but produces wrong code at runtime: reading the parameter back (e.g. +`xx this` to coerce to `*void`) yields a small struct-offset-shaped +value (saw 0x20 / 32 in our repro) instead of the Obj-C `id` that the +IMP trampoline received. Calling into the Obj-C runtime with that value +crashes with `EXC_BAD_ACCESS` / `SIGSEGV` at a near-null address. + +Observed: +- `library/modules/platform/uikit.sx` SxAppDelegate methods renamed + `self` → `this`. Body called `center.addObserver_selector_name_object(xx this, ...)`. +- Chess on iOS-sim crashed at first launch in + `-[NSNotificationCenter addObserver:selector:name:object:]` → `object_getClass(observer)`, + with `observer = 32` (low-int, not a real pointer). +- Reverting `this` → `self` (via `sed`) fixed the crash. Same body + shape, same `xx self` → call works fine. + +Expected: parameter name should not matter. The IMP trampoline binds +the receiver `id` to whatever the first parameter is named in the +method declaration — by position, not by hardcoded name. + +## Reproduction + +```sx +#import "modules/std.sx"; +#import "modules/std/objc.sx"; + +// Foreign declaration so we can dispatch. +NSObject :: #foreign #objc_class("NSObject") { + class :: () -> *void; + description :: (self: *Self) -> *void; +} + +// sx-defined class whose method's *Self param is named `this`. +Foo :: #objc_class("SxFooSelfTest") { + #extends NSObject; + + poke :: (this: *Self) -> *void { + // Should return the receiver back to the caller. + return xx this; + } +} + +main :: () -> s32 { + inline if OS == .macos { + f := Foo.alloc().init(); + result := f.poke(); + // result should be the same pointer as `f`. Expect equal. + if xx result == xx f { + print("ok\n"); + } else { + print("WRONG: this != self\n"); + // result will be a struct-offset shaped value + // (e.g. 0x20) instead of the Obj-C id. + } + } + inline if OS != .macos { print("skipped (not macos)\n"); } + 0; +} +``` + +Build and run on macOS; expected `ok`; observed `WRONG: this != self` +(or a crash if the value is dereferenced). + +If you swap `this` → `self` everywhere in the body and parameter list, +the test prints `ok`. + +## Investigation prompt + +In `src/ir/lower.zig`, several IMP-trampoline / method-body emission +paths hardcode the string `"self"` when binding the receiver parameter +or looking it up in the scope. Grep hits at the time of filing: + +``` +$ grep -n '"self"' src/ir/lower.zig +3422: init_scope.put("self", .{ ... }); +5133: const self_binding = if (self.scope) |s| s.lookup("self") else null; +10044: .name = "self", +11999/12056/12499/12662: params.append(... .internString("self") ...); +``` + +The methods that synthesize the IMP trampoline (M1.2 A.2) and the +ones that wire `*Self` → opaque foreign-class stub (M1.2 A.3) appear +to either: + +(a) emit the trampoline assuming the slot name in the body is "self", + so a body declared with `this: *Self` reads from an uninitialized / + different slot when it accesses the parameter; OR + +(b) resolve `*Self`-typed parameters by name rather than by position + + type, so a non-`self` name routes through a slower / different + binding path that doesn't see the IMP-passed receiver. + +Likely fix: have the parser / type-checker for `#objc_class` method +bodies identify the first `*Self`-typed parameter by **position and +type**, and bind the IMP-passed receiver into whatever local name the +user chose. Remove hardcoded `"self"` literals from the trampoline +emission and from the M1.2 A.3 `lowerFieldAccess` / `lowerAssignment` +helpers (the ivar→struct_gep path needs to know which local IS the +receiver, not assume it's named "self"). + +Verification step: + +1. Apply the fix. +2. Save the repro above as `examples/issue-0044.sx`. +3. Run `./zig-out/bin/sx run examples/issue-0044.sx` — expect `ok` + (not `WRONG: ...` and not a crash). +4. Bonus: confirm `bash tests/run_examples.sh` still passes (no + regression in existing `ffi-objc-*` tests, which all happen to + use `self` and so wouldn't have caught this). + +## Background + +Encountered during the FFI M3 follow-up cleanup of +`library/modules/platform/uikit.sx`. Renamed the IMP-side first +parameter from `self` to `this` across all `#objc_class` methods to +free `self` for upcoming M4 `self.method()` UFCS work on +`UIKitPlatform` methods called from those class bodies. Crash +manifested at first `[notificationCenter addObserver:self ...]` in +`-application:didFinishLaunchingWithOptions:`. Workaround in-session +was `sed -i '' 's/this: \*Self/self: *Self/g; s/xx this/xx self/g'` +across uikit.sx — the rename was uniform so the substitution was safe. + +Cost of the foot-gun: future contributors who follow CLAUDE.md's +"any first parameter name that makes the body clearer is fine" mental +model will silently mis-compile their `#objc_class` method bodies. + +## Root cause + fix + +The parameter name `this` vs `self` was a red herring. What actually +went wrong: + +1. uikit.sx renamed the AppDelegate's IMP method first param to `this`, + so `xx this` appeared inside the body of a `-> BOOL` method. +2. The body called `center.addObserver_selector_name_object(xx this, ..., null)` + on a `*NSNotificationCenter` foreign-class receiver. +3. `lowerCall` sets a per-arg `self.target_type` from + `resolveCallParamTypes(c)`. For UFCS dispatch on a foreign-class + alias, that function had no path covering `foreign_class_map` — + it tried `resolveFuncByName(qualified)` and `fn_ast_map.get(qualified)`, + both of which miss for `#foreign #objc_class` methods. +4. With `param_types` empty, the per-arg `target_type` assignment was + skipped, so `self.target_type` retained its previous value: the + enclosing fn's return type, **BOOL → i8**. +5. `xx this` then lowered with target type `i8`: `ptrtoint ptr to i64` + → `trunc i64 to i8`. The receiver pointer became its low byte (0xC0 + / 0x20 / etc., depending on heap address). +6. `addObserver:selector:name:object:` got that byte as the observer. + Apple's runtime calls `object_getClass(observer)` internally for + validation → near-null deref → SIGSEGV. + +The same shape works fine in `sx run` because the `xx` cast wasn't +exercised in the body in the original tests, OR the encoding +happened to land somewhere benign (e.g. the AOT-with-iOS-sim path +plus UIKit's specific validation order). + +**Fix:** add a `foreign_class_map.get(sname)` → +`findForeignMethodInChain` path to `resolveCallParamTypes`. When the +UFCS receiver is a foreign-class alias, walk the `#extends` chain to +find the method, then resolve its declared param types (skipping the +implicit `*Self` for instance methods). With the fix, `param_types` +returns `[*void, *void, *void, *void]` for the addObserver: call, +each `xx ptr` gets target type `*void`, and the cast is a clean +`ptrtoint` → `inttoptr` round-trip (or no-op since both sides are +pointer-typed). + +[src/ir/lower.zig:8617-8639](../src/ir/lower.zig#L8617). + +The parameter-name hardcoding in `lower.zig` (lines 3422, 5133, 10044, +11999, 12056, 12499, 12662) is unrelated — those are all SYNTHESIZED +parameters in compiler-generated functions (init scopes, JNI stubs, +property IMPs, dealloc IMPs), not the user-facing `#objc_class` +method body. The user's first param can be named anything. + +Regression test: `examples/issue-0044.sx`. Pre-fix, the +`captureSelf-from-BOOL` probe prints `WRONG` because `xx this` gets +truncated to its low byte and the round-trip comparison fails. With +the fix, all three probes print `ok`. diff --git a/src/ir/lower.zig b/src/ir/lower.zig index d0a2a22..76cbfe8 100644 --- a/src/ir/lower.zig +++ b/src/ir/lower.zig @@ -8615,6 +8615,30 @@ pub const Lowering = struct { } } if (self.getStructTypeName(obj_ty)) |sname| { + // Foreign-class receiver (`#objc_class` / `#jni_class` / etc.): + // resolve the method from `foreign_class_map` walking `#extends`. + // Without this path, `target_type` for each arg falls back to + // whatever `self.target_type` was on entry — typically the + // enclosing fn's return type — which silently truncates `xx ptr` + // casts inside e.g. a `BOOL`-returning method body. + if (self.foreign_class_map.get(sname)) |fcd| { + if (self.findForeignMethodInChain(fcd, fa.field)) |found| { + const md = found.method; + const saved_fc = self.current_foreign_class; + defer self.current_foreign_class = saved_fc; + self.current_foreign_class = found.fcd; + const user_param_start: usize = if (md.is_static) 0 else 1; + if (md.params.len > user_param_start) { + var types_list = std.ArrayList(TypeId).empty; + for (md.params[user_param_start..]) |p_node| { + types_list.append(self.alloc, self.resolveType(p_node)) catch unreachable; + } + return types_list.items; + } + return &.{}; + } + } + const qualified = std.fmt.allocPrint(self.alloc, "{s}.{s}", .{ sname, fa.field }) catch return &.{}; // Try already-lowered functions first if (self.resolveFuncByName(qualified)) |fid| { diff --git a/tests/expected/issue-0044.exit b/tests/expected/issue-0044.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/expected/issue-0044.exit @@ -0,0 +1 @@ +0 diff --git a/tests/expected/issue-0044.txt b/tests/expected/issue-0044.txt new file mode 100644 index 0000000..91afb7b --- /dev/null +++ b/tests/expected/issue-0044.txt @@ -0,0 +1,3 @@ +counter: 3 +me: ok +captureSelf-from-BOOL: ok