fix(arc): release static-array-of-interface elements at scope exit
A local `array[0..N] of IFoo` stored interface elements but never released them when the array went out of scope, leaking one obj reference per element on BOTH backends (the scope-exit ARC sweep had no static-array case). Both backends now release each interface element of a static-array-of- interface local at scope teardown, via shared helpers EmitStaticArrayReleaseElems / EmitManagedReleaseAt (per-backend). The QBE exception-path cleanup zeroes each released slot so an outer handler's cleanup stays a no-op. Scope is restricted to INTERFACE elements on purpose. Static-array-of- class/string/record locals are NOT released here: the element store retains unconditionally while owning code manages some such arrays manually (the ELF writer frees each `RelaBuf: array[0..5] of TByteBuf` element with `.Free`), so a blanket scope-exit release double-frees and corrupts the allocator — it crashed the native self-hosted compiler (caught by fixpoint-warmcache.sh). EmitRecordReleaseFields likewise still skips static-array fields, to stay symmetric with the retain/copy paths. Both gaps are documented for a later, symmetric fix. E2E regression (both backends): TE2ELeakCheckTests.TestDebug_StaticArrayOfInterface_NoLeak.
This commit is contained in:
Graeme Geldenhuys
parent
1e3f3ccc5d
commit
4bd52cd96a
|
|
@ -253,6 +253,19 @@ type
|
|||
scope-exit cleanup. }
|
||||
procedure EmitRecordFieldReleases(ART: TRecordTypeDesc;
|
||||
const ABaseReg: string);
|
||||
{ Release one ARC-managed value of type AType whose storage is at the address
|
||||
in callee-saved ABaseReg. Scalars (string/class/intf/dynarray) are
|
||||
released directly; records recurse via EmitRecordFieldReleases; static
|
||||
arrays recurse element-by-element via EmitStaticArrayReleaseElems. When
|
||||
AZero is set the scalar managed slot is zeroed after release (exception
|
||||
path). ABaseReg must survive callq. }
|
||||
procedure EmitManagedReleaseAt(AType: TTypeDesc; const ABaseReg: string;
|
||||
AZero: Boolean);
|
||||
{ Release every managed element of a static array whose inline storage starts
|
||||
at the address in callee-saved ABaseReg. Loops the element count,
|
||||
releasing each via EmitManagedReleaseAt. }
|
||||
procedure EmitStaticArrayReleaseElems(AType: TStaticArrayTypeDesc;
|
||||
const ABaseReg: string; AZero: Boolean);
|
||||
procedure EmitRecordFieldRetains(ART: TRecordTypeDesc;
|
||||
const ABaseReg: string);
|
||||
{ Emit all class/record method definitions for the given type decls plus
|
||||
|
|
@ -1868,6 +1881,13 @@ begin
|
|||
Self.Emit(#9'popq %r14');
|
||||
Continue;
|
||||
end;
|
||||
{ NOTE: a static-array-of-managed FIELD is intentionally NOT released here.
|
||||
EmitRecordFieldReleases must stay symmetric with EmitRecordFieldRetains /
|
||||
EmitRecordCopy, neither of which retains static-array elements; releasing
|
||||
them here without a matching retain over-releases on every record copy /
|
||||
by-value param pass and corrupts the heap. Static-array element ARC is
|
||||
handled only for scope-exit LOCALS (the bug-#4 case). Records with such
|
||||
fields remain a separate, latent concern. }
|
||||
if not (F.TypeDesc.IsString() or (F.TypeDesc.Kind = tyClass)
|
||||
or (F.TypeDesc.Kind = tyDynArray)
|
||||
or (F.TypeDesc.Kind = tyInterface)) then
|
||||
|
|
@ -1903,6 +1923,64 @@ begin
|
|||
end;
|
||||
end;
|
||||
|
||||
procedure TX86_64Backend.EmitManagedReleaseAt(AType: TTypeDesc;
|
||||
const ABaseReg: string; AZero: Boolean);
|
||||
begin
|
||||
if AType = nil then Exit;
|
||||
if AType.Kind = tyRecord then
|
||||
begin
|
||||
Self.EmitRecordFieldReleases(TRecordTypeDesc(AType), ABaseReg);
|
||||
Exit;
|
||||
end;
|
||||
if AType.Kind = tyStaticArray then
|
||||
begin
|
||||
Self.EmitStaticArrayReleaseElems(TStaticArrayTypeDesc(AType), ABaseReg, AZero);
|
||||
Exit;
|
||||
end;
|
||||
if not (AType.IsString() or (AType.Kind = tyClass)
|
||||
or (AType.Kind = tyDynArray) or (AType.Kind = tyInterface)) then
|
||||
Exit;
|
||||
{ Load the element's obj/data pointer (interface: obj slot at +0). }
|
||||
Self.Emit(Format(#9'movq (%s), %%rdi', [ABaseReg]));
|
||||
if AType.IsString() then
|
||||
Self.Emit(#9'callq _StringRelease')
|
||||
else if AType.Kind = tyDynArray then
|
||||
Self.Emit(#9'callq _DynArrayRelease')
|
||||
else
|
||||
{ tyClass and tyInterface both release the obj slot via _ClassRelease; an
|
||||
interface's itab slot at +8 is static rodata and needs no release. }
|
||||
Self.Emit(#9'callq _ClassRelease');
|
||||
if AZero then
|
||||
Self.Emit(Format(#9'movq $0, (%s)', [ABaseReg]));
|
||||
end;
|
||||
|
||||
procedure TX86_64Backend.EmitStaticArrayReleaseElems(AType: TStaticArrayTypeDesc;
|
||||
const ABaseReg: string; AZero: Boolean);
|
||||
var
|
||||
I, ElemSize: Integer;
|
||||
begin
|
||||
if (AType = nil) or (AType.ElementType = nil) then Exit;
|
||||
ElemSize := AType.ElementType.RawSize();
|
||||
{ Hold the array base in callee-saved %r15 and recompute each element address
|
||||
into %r14. Both are saved/restored as a PAIR (even push count) so the
|
||||
stack stays 16-byte aligned at the per-element release callq. Keeping the
|
||||
base in %r15 (not advancing %r14 in place) keeps the addressing correct
|
||||
under nested static arrays, whose recursive call reuses %r14. }
|
||||
Self.Emit(#9'pushq %r15');
|
||||
Self.Emit(#9'pushq %r14');
|
||||
Self.Emit(Format(#9'movq %s, %%r15', [ABaseReg]));
|
||||
for I := 0 to AType.HighBound - AType.LowBound do
|
||||
begin
|
||||
if I * ElemSize > 0 then
|
||||
Self.Emit(Format(#9'leaq %d(%%r15), %%r14', [I * ElemSize]))
|
||||
else
|
||||
Self.Emit(#9'movq %r15, %r14');
|
||||
Self.EmitManagedReleaseAt(AType.ElementType, '%r14', AZero);
|
||||
end;
|
||||
Self.Emit(#9'popq %r14');
|
||||
Self.Emit(#9'popq %r15');
|
||||
end;
|
||||
|
||||
procedure TX86_64Backend.EmitRecordFieldRetains(ART: TRecordTypeDesc;
|
||||
const ABaseReg: string);
|
||||
var
|
||||
|
|
@ -16464,6 +16542,32 @@ begin
|
|||
Self.EmitRecordFieldReleases(
|
||||
TRecordTypeDesc(TVarDecl(ADecl.Body.Decls.Items[I]).ResolvedType), '%rbx');
|
||||
Self.Emit(#9'popq %rbx');
|
||||
end
|
||||
{ Static-array-of-INTERFACE locals (array[0..N] of IFoo): release each
|
||||
fat-pointer element's obj slot at scope exit. The inline storage base
|
||||
goes into %rbx (callee-saved).
|
||||
|
||||
Scope: ONLY interface elements. The interface element store routes
|
||||
through EmitInterfaceToFieldSlotsAt, whose retain/release is balanced by
|
||||
this scope-exit release. Static-array-of-CLASS/STRING/record locals are
|
||||
deliberately excluded: the existing element store retains unconditionally
|
||||
while `.Free`/aliasing in the owning code (e.g. the ELF writer's
|
||||
`RelaBuf: array[0..5] of TByteBuf`) already manages those lifetimes, so a
|
||||
blanket scope-exit release double-frees and corrupts the heap. Closing
|
||||
that gap requires reconciling the element store's ARC with the manual
|
||||
management first; it is tracked separately. }
|
||||
else if (TVarDecl(ADecl.Body.Decls.Items[I]).ResolvedType.Kind = tyStaticArray)
|
||||
and (TStaticArrayTypeDesc(TVarDecl(ADecl.Body.Decls.Items[I]).ResolvedType).ElementType <> nil)
|
||||
and (TStaticArrayTypeDesc(TVarDecl(ADecl.Body.Decls.Items[I]).ResolvedType).ElementType.Kind = tyInterface) then
|
||||
for J := 0 to TVarDecl(ADecl.Body.Decls.Items[I]).Names.Count - 1 do
|
||||
begin
|
||||
Self.Emit(#9'pushq %rbx');
|
||||
Self.Emit(Format(#9'leaq %s, %%rbx',
|
||||
[Self.VarOperand(TVarDecl(ADecl.Body.Decls.Items[I]).Names.Strings[J])]));
|
||||
Self.EmitStaticArrayReleaseElems(
|
||||
TStaticArrayTypeDesc(TVarDecl(ADecl.Body.Decls.Items[I]).ResolvedType),
|
||||
'%rbx', False);
|
||||
Self.Emit(#9'popq %rbx');
|
||||
end;
|
||||
end;
|
||||
end;
|
||||
|
|
|
|||
|
|
@ -460,6 +460,19 @@ type
|
|||
{ Release every ARC-managed field of a record at AAddr in-line (no copy).
|
||||
Used before overwriting a record slot to prevent reference leaks. }
|
||||
procedure EmitRecordReleaseFields(ARec: TRecordTypeDesc; const AAddr: string);
|
||||
{ Release one ARC-managed value of type AType whose storage is at AAddr.
|
||||
AAddr points AT the slot (string/class/intf/dynarray) or AT the inline
|
||||
aggregate (record/static array). Recurses for records and nested static
|
||||
arrays so every managed leaf is released exactly once. }
|
||||
procedure EmitManagedReleaseAt(AType: TTypeDesc; const AAddr: string;
|
||||
AZero: Boolean);
|
||||
{ Release every managed element of a static array whose inline storage
|
||||
starts at AAddr. Loops AType.LowBound..HighBound, releasing each element
|
||||
via EmitManagedReleaseAt. When AZero is set (exception path) each scalar
|
||||
managed slot is zeroed after release so an outer handler's cleanup is a
|
||||
safe no-op. }
|
||||
procedure EmitStaticArrayReleaseElems(AType: TStaticArrayTypeDesc;
|
||||
const AAddr: string; AZero: Boolean);
|
||||
{ Mirror of EmitRecordReleaseFields: AddRef every ARC-managed field of a
|
||||
record at AAddr. Used in the by-value record param prologue so the
|
||||
callee owns retains on managed-field contents, balancing the matching
|
||||
|
|
@ -2390,6 +2403,27 @@ begin
|
|||
end;
|
||||
Continue;
|
||||
end
|
||||
else if (Decl.ResolvedType.Kind = tyStaticArray)
|
||||
and (TStaticArrayTypeDesc(Decl.ResolvedType).ElementType <> nil)
|
||||
and (TStaticArrayTypeDesc(Decl.ResolvedType).ElementType.Kind = tyInterface) then
|
||||
begin
|
||||
{ Static-array-of-INTERFACE local (array[0..N] of IFoo): release each
|
||||
fat-pointer element's obj slot at scope exit. VarRef gives the inline
|
||||
storage base.
|
||||
|
||||
Scope: ONLY interface elements — kept in lockstep with the native
|
||||
backend. Static-array-of-class/string/record locals are excluded
|
||||
because the element store's unconditional retain plus manual `.Free`/
|
||||
aliasing in the owning code would double-free; reconciling that is
|
||||
tracked separately. }
|
||||
for J := 0 to Decl.Names.Count - 1 do
|
||||
begin
|
||||
VarName := Decl.Names.Strings[J];
|
||||
EmitStaticArrayReleaseElems(TStaticArrayTypeDesc(Decl.ResolvedType),
|
||||
VarRef(VarName, Decl.IsGlobal), False);
|
||||
end;
|
||||
Continue;
|
||||
end
|
||||
else
|
||||
Continue;
|
||||
for J := 0 to Decl.Names.Count - 1 do
|
||||
|
|
@ -2473,6 +2507,21 @@ begin
|
|||
end;
|
||||
Continue;
|
||||
end
|
||||
else if (Decl.ResolvedType.Kind = tyStaticArray)
|
||||
and (TStaticArrayTypeDesc(Decl.ResolvedType).ElementType <> nil)
|
||||
and (TStaticArrayTypeDesc(Decl.ResolvedType).ElementType.Kind = tyInterface) then
|
||||
begin
|
||||
{ Static-array-of-interface local on exception path: release each element's
|
||||
obj slot and zero it so an outer handler's cleanup is a safe no-op.
|
||||
(Same interface-only scope as the normal path.) }
|
||||
for J := 0 to Decl.Names.Count - 1 do
|
||||
begin
|
||||
VarName := Decl.Names.Strings[J];
|
||||
EmitStaticArrayReleaseElems(TStaticArrayTypeDesc(Decl.ResolvedType),
|
||||
VarRef(VarName, Decl.IsGlobal), True);
|
||||
end;
|
||||
Continue;
|
||||
end
|
||||
else
|
||||
Continue;
|
||||
for J := 0 to Decl.Names.Count - 1 do
|
||||
|
|
@ -5805,6 +5854,13 @@ begin
|
|||
EmitRecordReleaseFields(TRecordTypeDesc(F.TypeDesc), FldAddr);
|
||||
Continue;
|
||||
end;
|
||||
{ NOTE: a static-array-of-managed FIELD is intentionally NOT released here.
|
||||
EmitRecordReleaseFields must stay symmetric with EmitRecordAddRefFields /
|
||||
EmitRecordCopy, neither of which retains static-array elements; releasing
|
||||
them here without a matching retain over-releases on every record copy /
|
||||
by-value param pass and corrupts the heap. Static-array element ARC is
|
||||
handled only for scope-exit LOCALS (the bug-#4 case). Records with such
|
||||
fields remain a separate, latent concern. }
|
||||
if not (F.TypeDesc.IsString() or (F.TypeDesc.Kind = tyClass)
|
||||
or (F.TypeDesc.Kind = tyDynArray)
|
||||
or (F.TypeDesc.Kind = tyInterface)) then Continue;
|
||||
|
|
@ -5829,6 +5885,63 @@ begin
|
|||
end;
|
||||
end;
|
||||
|
||||
procedure TCodeGenQBE.EmitManagedReleaseAt(AType: TTypeDesc; const AAddr: string;
|
||||
AZero: Boolean);
|
||||
var
|
||||
ValT: string;
|
||||
begin
|
||||
if AType = nil then Exit;
|
||||
if AType.Kind = tyRecord then
|
||||
begin
|
||||
{ Records are released field-by-field; the per-field scalar zeroing inside
|
||||
EmitRecordReleaseFields is not parameterised, so the AZero contract here
|
||||
is satisfied for the leaf scalars it touches. }
|
||||
EmitRecordReleaseFields(TRecordTypeDesc(AType), AAddr);
|
||||
Exit;
|
||||
end;
|
||||
if AType.Kind = tyStaticArray then
|
||||
begin
|
||||
EmitStaticArrayReleaseElems(TStaticArrayTypeDesc(AType), AAddr, AZero);
|
||||
Exit;
|
||||
end;
|
||||
if not (AType.IsString() or (AType.Kind = tyClass)
|
||||
or (AType.Kind = tyDynArray) or (AType.Kind = tyInterface)) then
|
||||
Exit;
|
||||
ValT := AllocTemp();
|
||||
EmitLine(Format(' %s =l loadl %s', [ValT, AAddr]));
|
||||
if AType.IsString() then
|
||||
EmitLine(Format(' call $_StringRelease(l %s)', [ValT]))
|
||||
else if AType.Kind = tyDynArray then
|
||||
EmitLine(Format(' call $_DynArrayRelease(l %s)', [ValT]))
|
||||
else
|
||||
{ tyClass and tyInterface both release the obj slot via _ClassRelease;
|
||||
an interface's itab slot at +8 is static rodata, no release needed. }
|
||||
EmitLine(Format(' call $_ClassRelease(l %s)', [ValT]));
|
||||
if AZero then
|
||||
EmitLine(Format(' storel 0, %s', [AAddr]));
|
||||
end;
|
||||
|
||||
procedure TCodeGenQBE.EmitStaticArrayReleaseElems(AType: TStaticArrayTypeDesc;
|
||||
const AAddr: string; AZero: Boolean);
|
||||
var
|
||||
I, ElemSize: Integer;
|
||||
ElemAddr: string;
|
||||
begin
|
||||
if (AType = nil) or (AType.ElementType = nil) then Exit;
|
||||
ElemSize := AType.ElementType.RawSize();
|
||||
for I := 0 to AType.HighBound - AType.LowBound do
|
||||
begin
|
||||
if I = 0 then
|
||||
ElemAddr := AAddr
|
||||
else
|
||||
begin
|
||||
ElemAddr := AllocTemp();
|
||||
EmitLine(Format(' %s =l add %s, %d', [ElemAddr, AAddr, I * ElemSize]));
|
||||
end;
|
||||
EmitManagedReleaseAt(AType.ElementType, ElemAddr, AZero);
|
||||
end;
|
||||
end;
|
||||
|
||||
procedure TCodeGenQBE.EmitRecordAddRefFields(ARec: TRecordTypeDesc;
|
||||
const AAddr: string);
|
||||
var
|
||||
|
|
|
|||
|
|
@ -63,6 +63,11 @@ type
|
|||
after the field load. L[I].HitPoints — the TList<T>.Get getter returns
|
||||
+1, the field is read, then the transient base must be released. }
|
||||
procedure TestDebug_ReceiverFieldAccess_NoLeak;
|
||||
{ A static array of interfaces (array[0..N] of IFoo): the elements are
|
||||
ARC-managed but the scope-exit cleanup previously skipped tyStaticArray
|
||||
locals entirely, so every stored element leaked on BOTH backends. The
|
||||
fix releases each element at scope exit. }
|
||||
procedure TestDebug_StaticArrayOfInterface_NoLeak;
|
||||
end;
|
||||
|
||||
implementation
|
||||
|
|
@ -705,6 +710,48 @@ const
|
|||
end.
|
||||
''';
|
||||
|
||||
{ Static array of interfaces: three IGreet elements stored into a local
|
||||
array[0..2]. Scope-exit cleanup must release each element. Prints the
|
||||
three greetings, then exits with no leak report. }
|
||||
SrcStaticArrayOfInterface = '''
|
||||
program P;
|
||||
type
|
||||
IGreet = interface
|
||||
function Name(): string;
|
||||
end;
|
||||
TGreet = class(IGreet)
|
||||
FName: string;
|
||||
function Name(): string;
|
||||
end;
|
||||
function TGreet.Name(): string;
|
||||
begin
|
||||
Result := Self.FName;
|
||||
end;
|
||||
procedure MakeInto(var G: IGreet; const N: string);
|
||||
var
|
||||
T: TGreet;
|
||||
begin
|
||||
T := TGreet.Create();
|
||||
T.FName := N;
|
||||
G := T;
|
||||
end;
|
||||
procedure Run();
|
||||
var
|
||||
Arr: array[0..2] of IGreet;
|
||||
I: Integer;
|
||||
begin
|
||||
MakeInto(Arr[0], 'a');
|
||||
MakeInto(Arr[1], 'b');
|
||||
MakeInto(Arr[2], 'c');
|
||||
for I := 0 to 2 do
|
||||
Write(Arr[I].Name());
|
||||
WriteLn('');
|
||||
end;
|
||||
begin
|
||||
Run();
|
||||
end.
|
||||
''';
|
||||
|
||||
procedure TE2ELeakCheckTests.TestDebug_ReceiverFieldAccess_NoLeak;
|
||||
var
|
||||
Output: string;
|
||||
|
|
@ -723,6 +770,24 @@ begin
|
|||
AssertTrue('no leak report (native), got: ' + Output, Pos('leak', Output) < 0);
|
||||
end;
|
||||
|
||||
procedure TE2ELeakCheckTests.TestDebug_StaticArrayOfInterface_NoLeak;
|
||||
var
|
||||
Output: string;
|
||||
ExitCode: Integer;
|
||||
begin
|
||||
if not ToolchainAvailable() then begin Ignore('toolchain unavailable'); Exit end;
|
||||
AssertTrue('compile+run (qbe)',
|
||||
CompileAndRunWithRTLDebugOn(beQBE, SrcStaticArrayOfInterface, Output, ExitCode, True));
|
||||
AssertEquals('exit 0 (qbe)', 0, ExitCode);
|
||||
AssertEquals('stdout (qbe)', 'abc' + LE, Output);
|
||||
AssertTrue('no leak report (qbe), got: ' + Output, Pos('leak', Output) < 0);
|
||||
AssertTrue('compile+run (native)',
|
||||
CompileAndRunWithRTLDebugOn(beNative, SrcStaticArrayOfInterface, Output, ExitCode, True));
|
||||
AssertEquals('exit 0 (native)', 0, ExitCode);
|
||||
AssertEquals('stdout (native)', 'abc' + LE, Output);
|
||||
AssertTrue('no leak report (native), got: ' + Output, Pos('leak', Output) < 0);
|
||||
end;
|
||||
|
||||
initialization
|
||||
RegisterTest(TE2ELeakCheckTests);
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue