From 1e0b7fc46a780b8cac31487a9f68630dadc3d8d2 Mon Sep 17 00:00:00 2001 From: Graeme Geldenhuys Date: Sat, 27 Jun 2026 11:57:28 +0100 Subject: [PATCH] refactor(codegen): hoist ARC ownership predicate to blaise.codegen (F2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ExprOwnsRef (QBE backend) and NativeExprOwnsRef (native backend) were byte-identical twins: a pure walk over the AST node + its resolved type deciding whether an r-value leaves an ARC-managed value at refcount +1 that the consuming site must not AddRef again. Two drift-prone copies of an ARC decision is exactly the failure mode that leaks/UAFs when one side is updated and the other is not. Hoist the single source of truth into blaise.codegen as ArcExprOwnsRef (distinct name so an unqualified call inside a method cannot bind to a same-named method — the F4 gotcha). Both backends keep their existing free function as a one-line delegator, so every call site is unchanged. Cross-backend dedup, no behaviour change: native .s AND QBE IR both byte-identical on an ARC-heavy program (function-returned class, string property read via getter, method call returning a class, dynamic-array return). All four fixpoints and the full suite (QBE-built and native-built test runners) unchanged. --- .../pascal/blaise.codegen.native.x86_64.pas | 56 +------------- compiler/src/main/pascal/blaise.codegen.pas | 74 +++++++++++++++++++ .../src/main/pascal/blaise.codegen.qbe.pas | 63 +--------------- 3 files changed, 81 insertions(+), 112 deletions(-) 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 7c9374a..c4421e7 100644 --- a/compiler/src/main/pascal/blaise.codegen.native.x86_64.pas +++ b/compiler/src/main/pascal/blaise.codegen.native.x86_64.pas @@ -27,7 +27,7 @@ interface uses SysUtils, Classes, contnrs, Generics.Collections, uAST, uSymbolTable, uStrCompat, - blaise.codegen.arcshapes, uDebugFacts, + blaise.codegen, blaise.codegen.arcshapes, uDebugFacts, blaise.codegen.native.backend, blaise.codegen.target; const @@ -1778,58 +1778,10 @@ begin end; function NativeExprOwnsRef(AExpr: TASTExpr): Boolean; -var - FA: TFieldAccessExpr; - MC: TMethodCallExpr; - IE: TIdentExpr; begin - Result := False; - if AExpr = nil then Exit; - if AExpr.ResolvedType = nil then Exit; - { Ownership transfer applies to every ARC-managed return value, not just - classes: a function/method returning a String or dynamic array leaves its - Result at refcount +1 (the callee AddRef'd on `Result := x` and did not - release Result at scope exit). The caller's assignment site must therefore - NOT AddRef again — it consumes that transferred reference. Without covering - tyString/tyDynArray here the assignment branches below emit a spurious - _StringAddRef/_DynArrayAddRef on the call result, which is never balanced - and leaks one buffer per call. } - if not (AExpr.ResolvedType.Kind in [tyClass, tyDynArray]) - and not AExpr.ResolvedType.IsString() then Exit; - if AExpr is TIdentExpr then - begin - IE := TIdentExpr(AExpr); - if IE.IsImplicitSelfMethod then - Exit(True); - end; - if AExpr is TFieldAccessExpr then - begin - FA := TFieldAccessExpr(AExpr); - if FA.IsConstructorCall then Exit; - if FA.IsMethodCall then begin Result := True; Exit end; - if (FA.PropRead <> nil) and (FA.PropRead.ReadMethod <> '') then - Exit(True); - end; - if AExpr is TMethodCallExpr then - begin - MC := TMethodCallExpr(AExpr); - if not MC.IsConstructorCall then Result := True; - Exit; - end; - if AExpr is TFuncCallExpr then - begin - if (TFuncCallExpr(AExpr).ResolvedDecl <> nil) or - TFuncCallExpr(AExpr).IsIndirectCall then - Result := True; - Exit; - end; - if AExpr is TStringSubscriptExpr then - begin - if (TStringSubscriptExpr(AExpr).StrExpr is TFieldAccessExpr) and - (TFieldAccessExpr(TStringSubscriptExpr(AExpr).StrExpr).PropRead <> nil) and - (TFieldAccessExpr(TStringSubscriptExpr(AExpr).StrExpr).PropRead.ReadMethod <> '') then - Result := True; - end; + { Delegates to the shared backend-neutral predicate in blaise.codegen + (formerly a byte-identical twin of the QBE backend's ExprOwnsRef). } + Result := ArcExprOwnsRef(AExpr); end; { Emit an immortal class-name string blob and return the label+12 reference diff --git a/compiler/src/main/pascal/blaise.codegen.pas b/compiler/src/main/pascal/blaise.codegen.pas index 40b3d3d..72c57d2 100644 --- a/compiler/src/main/pascal/blaise.codegen.pas +++ b/compiler/src/main/pascal/blaise.codegen.pas @@ -121,6 +121,18 @@ function RecretEightbyteIsSSE(ARec: TRecordTypeDesc; AStartByte: Integer): Boole function RecretClassify(ARec: TRecordTypeDesc; const ATarget: TTargetDesc): TRecReturnClass; +{ ---------------------------------------------------------------------- + Shared ARC ownership-transfer predicate. + + True when AExpr, used as an r-value, leaves an ARC-managed value at + refcount +1 that the consuming site must NOT AddRef again (it consumes the + transferred reference). Covers function/method calls and method-backed + property reads returning a class, string, or dynamic array. The decision is + a pure walk over the AST node + its resolved type — no backend state — so it + is single-sourced here and both backends call it (formerly the byte-identical + twins ExprOwnsRef / NativeExprOwnsRef). } +function ArcExprOwnsRef(AExpr: TASTExpr): Boolean; + implementation function RecretManagedClean(ARec: TRecordTypeDesc): Boolean; @@ -283,4 +295,66 @@ begin end; end; +function ArcExprOwnsRef(AExpr: TASTExpr): Boolean; +var + FA: TFieldAccessExpr; + MC: TMethodCallExpr; + IE: TIdentExpr; +begin + Result := False; + if AExpr = nil then Exit; + if AExpr.ResolvedType = nil then Exit; + { Ownership transfer applies to every ARC-managed return value, not just + classes: a function/method returning a String or dynamic array leaves + its Result at refcount +1 (the callee AddRef'd on `Result := x` and did + not release Result at scope exit). The caller's assignment site must + therefore NOT AddRef again — it consumes that transferred reference. + Without covering tyString/tyDynArray here the assignment branches emit a + spurious _StringAddRef/_DynArrayAddRef on the call result, which is never + balanced and leaks one buffer per call. } + if not (AExpr.ResolvedType.Kind in [tyClass, tyDynArray]) + and not AExpr.ResolvedType.IsString() then Exit; + if AExpr is TIdentExpr then + begin + IE := TIdentExpr(AExpr); + if IE.IsImplicitSelfMethod then + Exit(True); + end; + { Constructor calls via TFieldAccessExpr (TFoo.Create) — do NOT own. } + if AExpr is TFieldAccessExpr then + begin + FA := TFieldAccessExpr(AExpr); + if FA.IsConstructorCall then Exit; + if FA.IsMethodCall then begin Result := True; Exit end; + { Method-backed property read (read GetX): the getter returns +1. + Field-backed reads (read FX) emit a plain load and do NOT own. } + if (FA.PropRead <> nil) and (FA.PropRead.ReadMethod <> '') then + Exit(True); + end; + { TMethodCallExpr: constructor calls do NOT own; all other method calls DO. } + if AExpr is TMethodCallExpr then + begin + MC := TMethodCallExpr(AExpr); + if not MC.IsConstructorCall then Result := True; + Exit; + end; + if AExpr is TFuncCallExpr then + begin + if (TFuncCallExpr(AExpr).ResolvedDecl <> nil) or + TFuncCallExpr(AExpr).IsIndirectCall then + Result := True; + Exit; + end; + { Indexed property subscript: L[I] desugars to Subscript(FieldAccess(Items)) + where Items has a ReadMethod. The subscript emitter delegates to the + getter — the result inherits the +1. } + if AExpr is TStringSubscriptExpr then + begin + if (TStringSubscriptExpr(AExpr).StrExpr is TFieldAccessExpr) and + (TFieldAccessExpr(TStringSubscriptExpr(AExpr).StrExpr).PropRead <> nil) and + (TFieldAccessExpr(TStringSubscriptExpr(AExpr).StrExpr).PropRead.ReadMethod <> '') then + Result := True; + end; +end; + end. diff --git a/compiler/src/main/pascal/blaise.codegen.qbe.pas b/compiler/src/main/pascal/blaise.codegen.qbe.pas index 2fad1db..a8ef401 100644 --- a/compiler/src/main/pascal/blaise.codegen.qbe.pas +++ b/compiler/src/main/pascal/blaise.codegen.qbe.pas @@ -3792,67 +3792,10 @@ end; Variable reads, field reads, type casts, and lookups do NOT own their result and always need the assignment-site AddRef. } function ExprOwnsRef(AExpr: TASTExpr): Boolean; -var - FA: TFieldAccessExpr; - MC: TMethodCallExpr; - IE: TIdentExpr; begin - Result := False; - if AExpr = nil then Exit; - if AExpr.ResolvedType = nil then Exit; - { Ownership transfer applies to every ARC-managed return value, not just - classes: a function/method returning a String or dynamic array leaves - its Result at refcount +1 (the callee AddRef'd on `Result := x` and did - not release Result at scope exit). The caller's assignment site must - therefore NOT AddRef again — it consumes that transferred reference. - Without covering tyString/tyDynArray here the assignment branches below - emit a spurious _StringAddRef/_DynArrayAddRef on the call result, which - is never balanced and leaks one buffer per call. } - if not (AExpr.ResolvedType.Kind in [tyClass, tyDynArray]) - and not AExpr.ResolvedType.IsString() then Exit; - if AExpr is TIdentExpr then - begin - IE := TIdentExpr(AExpr); - if IE.IsImplicitSelfMethod then - Exit(True); - end; - { Constructor calls via TFieldAccessExpr (TFoo.Create) — do NOT own } - if AExpr is TFieldAccessExpr then - begin - FA := TFieldAccessExpr(AExpr); - if FA.IsConstructorCall then Exit; - if FA.IsMethodCall then begin Result := True; Exit end; - { Method-backed property read (read GetX): the getter returns +1. - Field-backed reads (read FX) emit a plain load and do NOT own. } - if (FA.PropRead <> nil) and (FA.PropRead.ReadMethod <> '') then - begin - Exit(True); - end; - end; - { TMethodCallExpr: constructor calls do NOT own; all other method calls DO } - if AExpr is TMethodCallExpr then - begin - MC := TMethodCallExpr(AExpr); - if not MC.IsConstructorCall then Result := True; - Exit; - end; - if AExpr is TFuncCallExpr then - begin - if (TFuncCallExpr(AExpr).ResolvedDecl <> nil) or - TFuncCallExpr(AExpr).IsIndirectCall then - Result := True; - Exit; - end; - { Indexed property subscript: L[I] desugars to Subscript(FieldAccess(Items)) - where Items has a ReadMethod. EmitStringSubscriptExpr delegates to - EmitExpr(StrExpr) which calls the getter — the result inherits the +1. } - if AExpr is TStringSubscriptExpr then - begin - if (TStringSubscriptExpr(AExpr).StrExpr is TFieldAccessExpr) and - (TFieldAccessExpr(TStringSubscriptExpr(AExpr).StrExpr).PropRead <> nil) and - (TFieldAccessExpr(TStringSubscriptExpr(AExpr).StrExpr).PropRead.ReadMethod <> '') then - Result := True; - end; + { Delegates to the shared backend-neutral predicate in blaise.codegen + (formerly a byte-identical twin of the native backend's NativeExprOwnsRef). } + Result := ArcExprOwnsRef(AExpr); end; { True when the expression produces a fresh sret record temporary that