From 4bd52cd96a96b9bda03a4143cc5451d33e208807 Mon Sep 17 00:00:00 2001 From: Graeme Geldenhuys Date: Sat, 27 Jun 2026 21:23:37 +0100 Subject: [PATCH] fix(arc): release static-array-of-interface elements at scope exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../pascal/blaise.codegen.native.x86_64.pas | 104 ++++++++++++++++ .../src/main/pascal/blaise.codegen.qbe.pas | 113 ++++++++++++++++++ .../src/test/pascal/cp.test.e2e.leakcheck.pas | 65 ++++++++++ 3 files changed, 282 insertions(+) diff --git a/compiler/src/main/pascal/blaise.codegen.native.x86_64.pas b/compiler/src/main/pascal/blaise.codegen.native.x86_64.pas index c1fe61a..b23ed31 100644 --- a/compiler/src/main/pascal/blaise.codegen.native.x86_64.pas +++ b/compiler/src/main/pascal/blaise.codegen.native.x86_64.pas @@ -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; diff --git a/compiler/src/main/pascal/blaise.codegen.qbe.pas b/compiler/src/main/pascal/blaise.codegen.qbe.pas index 14e7353..df527bb 100644 --- a/compiler/src/main/pascal/blaise.codegen.qbe.pas +++ b/compiler/src/main/pascal/blaise.codegen.qbe.pas @@ -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 diff --git a/compiler/src/test/pascal/cp.test.e2e.leakcheck.pas b/compiler/src/test/pascal/cp.test.e2e.leakcheck.pas index 0b124d2..13162c5 100644 --- a/compiler/src/test/pascal/cp.test.e2e.leakcheck.pas +++ b/compiler/src/test/pascal/cp.test.e2e.leakcheck.pas @@ -63,6 +63,11 @@ type after the field load. L[I].HitPoints — the TList.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);