fix(native): load Result after epilogue ARC releases, not before

EmitFunctionDef loaded the function Result into %rax (or %xmm0) at the top of
the epilogue, then ran the local/param ARC release pass — which calls
_ClassRelease / _StringRelease / _DynArrayRelease.  Those calls clobber %rax, so
any value-returning function that also releases an ARC-managed parameter or
local returned garbage.

Most visibly, a method returning Integer but taking an interface (or class)
value parameter releases that parameter on exit, so `U.Use(I)` returned 3
instead of 55; a String-returning function taking a String parameter returned a
clobbered pointer.

Move the Result load to AFTER every ARC release call, immediately before
leave/ret, so nothing can clobber it.  sret (record-returning) functions are
unaffected — they return via the caller's buffer and load no Result.  The $main
epilogue already sets %eax=0 after its global-release pass, so it was correct.

Two e2e regression tests on both backends: TestRun_Native_RetValSurvivesArcRelease
(value return past a class-param release) and TestRun_Native_IntfArgToMethod
(interface value arg to a method, dispatched in the callee).

Full suite OK (2681), FIXPOINT_OK (native-backend-only change).
This commit is contained in:
Graeme Geldenhuys 2026-06-09 17:25:39 +01:00
parent 971897a411
commit facd18bdca
2 changed files with 86 additions and 9 deletions

View file

@ -6802,16 +6802,13 @@ begin
if ADecl.Body <> nil then if ADecl.Body <> nil then
for I := 0 to ADecl.Body.Stmts.Count - 1 do for I := 0 to ADecl.Body.Stmts.Count - 1 do
Self.EmitStmt(TASTStmt(ADecl.Body.Stmts.Items[I])); Self.EmitStmt(TASTStmt(ADecl.Body.Stmts.Items[I]));
{ Epilogue: Exit lands here; load Result into %rax (int) or %xmm0 (float). { Epilogue: Exit lands here. The Result is loaded into %rax/%xmm0 only AFTER
For sret functions the caller's buffer already holds the result — just ret. } the ARC release passes below — those call _ClassRelease/_StringRelease, which
clobber %rax (and may touch the XMM regs), so loading Result first would
return garbage from a value-returning function that also releases ARC
params/locals. For sret functions the caller's buffer already holds the
result, so no load is needed. }
Self.Emit(FExitLabel + ':'); Self.Emit(FExitLabel + ':');
if (ADecl.ResolvedReturnType <> nil) and not FSretFunc then
begin
if IsFloatFamily(ADecl.ResolvedReturnType) then
Self.EmitLoadFloat(Self.VarOperand('Result'), ADecl.ResolvedReturnType)
else
Self.EmitLoadVar(Self.VarOperand('Result'), ADecl.ResolvedReturnType);
end;
{ Release ARC-managed locals (not params, not Result). { Release ARC-managed locals (not params, not Result).
Result is returned to the caller who owns it. } Result is returned to the caller who owns it. }
if ADecl.Body <> nil then if ADecl.Body <> nil then
@ -6892,6 +6889,16 @@ begin
Self.Emit(#9'callq _ClassRelease'); Self.Emit(#9'callq _ClassRelease');
end; end;
end; end;
{ Now that every ARC release call is done (none can clobber the result), load
Result into %rax (int) or %xmm0 (float). sret functions return via the
caller's buffer and need no load. }
if (ADecl.ResolvedReturnType <> nil) and not FSretFunc then
begin
if IsFloatFamily(ADecl.ResolvedReturnType) then
Self.EmitLoadFloat(Self.VarOperand('Result'), ADecl.ResolvedReturnType)
else
Self.EmitLoadVar(Self.VarOperand('Result'), ADecl.ResolvedReturnType);
end;
Self.Emit(#9'movq %rbp, %rsp'); Self.Emit(#9'movq %rbp, %rsp');
Self.Emit(#9'popq %rbp'); Self.Emit(#9'popq %rbp');
Self.Emit(#9'ret'); Self.Emit(#9'ret');

View file

@ -181,6 +181,8 @@ type
procedure TestRun_Native_ArcInterfaceField_AssignAndDispatch; procedure TestRun_Native_ArcInterfaceField_AssignAndDispatch;
procedure TestRun_Native_IntfFieldDispatch; procedure TestRun_Native_IntfFieldDispatch;
procedure TestRun_Native_IntfFieldReadIntoLocal; procedure TestRun_Native_IntfFieldReadIntoLocal;
procedure TestRun_Native_RetValSurvivesArcRelease;
procedure TestRun_Native_IntfArgToMethod;
procedure TestRun_Native_ArcNestedRecordField_FullCleanup; procedure TestRun_Native_ArcNestedRecordField_FullCleanup;
procedure TestRun_Native_ArcStringReturnToField_NoDoubleRetain; procedure TestRun_Native_ArcStringReturnToField_NoDoubleRetain;
procedure TestRun_Native_ArcImplicitSelfStringField_Reassign; procedure TestRun_Native_ArcImplicitSelfStringField_Reassign;
@ -2589,6 +2591,62 @@ const
end. end.
'''; ''';
{ Value-returning function whose return value must survive the epilogue ARC
release pass. A function returning Integer but taking an ARC (class) value
param releases that param on exit (_ClassRelease clobbers %rax); the Result
must be loaded into %rax AFTER that release, not before. Previously the
native backend loaded Result first and returned garbage (e.g. 3 not 55). }
SrcRetValSurvivesArcRelease = '''
program P;
type
TGreeter = class
function Greet: Integer;
end;
TUser = class
function Use(O: TGreeter): Integer;
end;
function TGreeter.Greet: Integer; begin Result := 55 end;
function TUser.Use(O: TGreeter): Integer;
begin
Result := O.Greet()
end;
var T: TGreeter; U: TUser;
begin
T := TGreeter.Create();
U := TUser.Create();
WriteLn(U.Use(T))
end.
''';
{ Interface value argument passed to a method, dispatched inside the callee.
Exercises the fat-pointer arg ABI at a method call site AND the
return-value-survives-release fix (the callee releases the interface param). }
SrcIntfArgToMethod = '''
program P;
type
IGreeter = interface
function Greet: Integer;
end;
TGreeter = class(TObject, IGreeter)
function Greet: Integer;
end;
TUser = class
function Use(G: IGreeter): Integer;
end;
function TGreeter.Greet: Integer; begin Result := 55 end;
function TUser.Use(G: IGreeter): Integer;
begin
Result := G.Greet()
end;
var T: TGreeter; I: IGreeter; U: TUser;
begin
T := TGreeter.Create();
I := T;
U := TUser.Create();
WriteLn(U.Use(I))
end.
''';
{ Dyn-array field inside a class: the field must be ARC-refcounted on store { Dyn-array field inside a class: the field must be ARC-refcounted on store
and released when the holder is destroyed (f74e5cc). Observed by reading an and released when the holder is destroyed (f74e5cc). Observed by reading an
element back after the field assignment — a dropped/garbled buffer would element back after the field assignment — a dropped/garbled buffer would
@ -3165,6 +3223,18 @@ begin
AssertRunsOnBoth(SrcIntfFieldReadIntoLocal, '40' + LE, 0); AssertRunsOnBoth(SrcIntfFieldReadIntoLocal, '40' + LE, 0);
end; end;
procedure TE2ENativeTests.TestRun_Native_RetValSurvivesArcRelease;
begin
if not ToolchainAvailable() then begin Ignore('toolchain unavailable'); Exit; end;
AssertRunsOnBoth(SrcRetValSurvivesArcRelease, '55' + LE, 0);
end;
procedure TE2ENativeTests.TestRun_Native_IntfArgToMethod;
begin
if not ToolchainAvailable() then begin Ignore('toolchain unavailable'); Exit; end;
AssertRunsOnBoth(SrcIntfArgToMethod, '55' + LE, 0);
end;
procedure TE2ENativeTests.TestRun_Native_ArcNestedRecordField_FullCleanup; procedure TE2ENativeTests.TestRun_Native_ArcNestedRecordField_FullCleanup;
begin begin
if not ToolchainAvailable() then begin Ignore('toolchain unavailable'); Exit; end; if not ToolchainAvailable() then begin Ignore('toolchain unavailable'); Exit; end;