From 03b59e8ff08c12929a6a76963d6b71c1b4203cdb Mon Sep 17 00:00:00 2001 From: Graeme Geldenhuys Date: Thu, 4 Jun 2026 23:21:25 +0100 Subject: [PATCH] Revert "perf(arc): elide retain/release for const string and class params" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 5a5b5d4. The optimisation elided the callee-side ARC retain/release for const string/class/interface value params on the premise that the caller keeps the argument alive for the whole call. That premise fails for a TEMPORARY bound to a const param (e.g. `Use(A + ' ' + B)`): the concatenation result's only reference is the argument slot, so without the callee-side retain its refcount hits zero at the call boundary and it is freed before the callee reads it — a use-after-free. This bit the RTL hardest: `_StringCopy` / `StrHead` take `const string` params and are called with built-at-runtime temporaries, so the emitted RTL was miscompiled. Under self-hosting the defect is self-reproducing and only manifests at the SECOND generation (the compiler that emits the broken RTL is itself fine), which is why a one-step fixpoint did not expose it and the compiler's own sources did not reliably trigger it. The symptom was a deterministic crash compiling any program that uses a metaclass reference (`C := TFoo`) or HasClassAttribute — which is why TestRunner (via blaise.testing.runner.text) could not be built, blocking the whole suite. The original change's valgrind e2e test passed only because it bound a string LITERAL (immortal) to the const param, not a temporary. Removed the now-invalid IR/e2e tests that asserted the elided behaviour (string const params, and the interface-const variant from 088d12f which relied on this commit's IsConstParam guard). Added TestRun_ConstStringTemp_StaysAlive_Valgrind, which passes a concatenation result as a const string param and reads it in the callee — the exact case the optimisation broke. docs/future-improvements.adoc records how to re-attempt the elision safely (condition on the argument, not the parameter; retain temporaries either caller- or callee-side). Verified: stage-2 build clean, TestRunner builds, full suite 0 failures, FIXPOINT_OK. --- .../pascal/blaise.codegen.native.x86_64.pas | 6 +- compiler/src/main/pascal/uCodeGenQBE.pas | 22 ++--- compiler/src/test/pascal/cp.test.arc.pas | 67 +------------- compiler/src/test/pascal/cp.test.e2e.arc.pas | 89 ++++++++++--------- docs/future-improvements.adoc | 49 ++++++++++ 5 files changed, 104 insertions(+), 129 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 effec36..90ab6a5 100644 --- a/compiler/src/main/pascal/blaise.codegen.native.x86_64.pas +++ b/compiler/src/main/pascal/blaise.codegen.native.x86_64.pas @@ -4047,11 +4047,7 @@ begin TODO(arc): the native backend does not yet retain string/class/interface *value* params on entry and release them on exit (the QBE backend does — see the entry/exit ARC loops in uCodeGenQBE.pas; interfaces ARC through the - object slot of their fat pointer). When that retain/release is added here, - it MUST skip params where P.IsConstParam is True (as well as IsVarParam / - IsOpenArray): a const param's object is kept alive by the caller for the - whole call, so no callee-side retain/release is needed. See the matching - `or Par.IsConstParam` guards in uCodeGenQBE.pas. } + object slot of their fat pointer). } if ADecl.Body <> nil then begin for I := 0 to ADecl.Body.Decls.Count - 1 do diff --git a/compiler/src/main/pascal/uCodeGenQBE.pas b/compiler/src/main/pascal/uCodeGenQBE.pas index d85edfe..306b803 100644 --- a/compiler/src/main/pascal/uCodeGenQBE.pas +++ b/compiler/src/main/pascal/uCodeGenQBE.pas @@ -5312,13 +5312,11 @@ begin EmitParamAllocs(AMethod, nil); { ARC: addref string and class value params on entry — balances the - release pass at method exit. const params are skipped: the caller - guarantees the object stays alive for the whole call, so the callee - needs no retain/release. } + release pass at method exit. } for I := 0 to AMethod.Params.Count - 1 do begin Par := TMethodParam(AMethod.Params.Items[I]); - if Par.IsVarParam or Par.IsOpenArray or Par.IsConstParam then Continue; + if Par.IsVarParam or Par.IsOpenArray then Continue; if Par.ResolvedType.Kind = tyString then begin ValTemp := AllocTemp; @@ -5377,12 +5375,11 @@ begin FExitLabel := SavedExitLbl; end; - { ARC: release string and class value params on exit. const params are - skipped to match the entry pass (no retain was taken). } + { ARC: release string and class value params on exit. } for I := 0 to AMethod.Params.Count - 1 do begin Par := TMethodParam(AMethod.Params.Items[I]); - if Par.IsVarParam or Par.IsOpenArray or Par.IsConstParam then Continue; + if Par.IsVarParam or Par.IsOpenArray then Continue; if Par.ResolvedType.Kind = tyString then begin ValTemp := AllocTemp; @@ -6159,13 +6156,11 @@ begin end; { ARC: addref string and class value params on entry (callee owns a - retained copy that is balanced by the release pass at function exit). - const params are skipped: the caller keeps the object alive for the - whole call, so no retain/release is needed. } + retained copy that is balanced by the release pass at function exit). } for I := 0 to ADecl.Params.Count - 1 do begin Par := TMethodParam(ADecl.Params.Items[I]); - if Par.IsVarParam or Par.IsOpenArray or Par.IsConstParam then Continue; + if Par.IsVarParam or Par.IsOpenArray then Continue; if Par.ResolvedType.Kind = tyString then begin ValTemp := AllocTemp; @@ -6230,12 +6225,11 @@ begin end; { ARC: release string and class value params on exit (balances the - addref inserted at function entry). const params are skipped to match - the entry pass (no retain was taken). } + addref inserted at function entry). } for I := 0 to ADecl.Params.Count - 1 do begin Par := TMethodParam(ADecl.Params.Items[I]); - if Par.IsVarParam or Par.IsOpenArray or Par.IsConstParam then Continue; + if Par.IsVarParam or Par.IsOpenArray then Continue; if Par.ResolvedType.Kind = tyString then begin ValTemp := AllocTemp; diff --git a/compiler/src/test/pascal/cp.test.arc.pas b/compiler/src/test/pascal/cp.test.arc.pas index 0e9603f..2f115c7 100644 --- a/compiler/src/test/pascal/cp.test.arc.pas +++ b/compiler/src/test/pascal/cp.test.arc.pas @@ -48,17 +48,12 @@ type procedure TestARC_StringVarParam_NoAddRef; procedure TestARC_StringVarParam_NoRelease; - { String const parameter: no addref, no release (caller holds it alive) } - procedure TestARC_StringConstParam_NoAddRef; - procedure TestARC_StringConstParam_NoRelease; - { Interface value parameter: addref on entry, release on exit (via the obj slot — interfaces ARC through _ClassAddRef/_ClassRelease). } procedure TestARC_IntfValueParam_AddRefOnEntry; procedure TestARC_IntfValueParam_ReleaseOnExit; - { Interface const/var parameter: no addref, no release in the callee. } - procedure TestARC_IntfConstParam_NoAddRef; + { Interface var parameter: no addref, no release in the callee. } procedure TestARC_IntfVarParam_NoAddRef; { String concatenation: calls RTL concat function } @@ -303,14 +298,6 @@ const begin end. '''; - SrcConstParam = - ''' - program P; - procedure Greet(const S: string); - begin end; - begin end. - '''; - SrcIntfValueParam = ''' program P; @@ -335,30 +322,6 @@ const end. '''; - SrcIntfConstParam = - ''' - program P; - type - IThing = interface - procedure Emit; - end; - TThing = class(TObject, IThing) - procedure Emit; - end; - procedure TThing.Emit; - begin end; - procedure DoSomething(const MyIntf: IThing); - begin - MyIntf.Emit - end; - var T: TThing; F: IThing; - begin - T := TThing.Create; - F := T; - DoSomething(F) - end. - '''; - SrcIntfVarParam = ''' program P; @@ -424,22 +387,6 @@ begin AssertFalse('no release for string var param', IRContains(IR, 'call $_StringRelease')); end; -procedure TARCTests.TestARC_StringConstParam_NoAddRef; -var - IR: string; -begin - IR := GenIR(SrcConstParam); - AssertFalse('no addref for string const param', IRContains(IR, 'call $_StringAddRef')); -end; - -procedure TARCTests.TestARC_StringConstParam_NoRelease; -var - IR: string; -begin - IR := GenIR(SrcConstParam); - AssertFalse('no release for string const param', IRContains(IR, 'call $_StringRelease')); -end; - function ExtractDoSomethingBody(const AIR: string): string; var FnPos, NextPos: Integer; @@ -474,18 +421,6 @@ begin Pos('call $_ClassRelease', Body) > 0); end; -procedure TARCTests.TestARC_IntfConstParam_NoAddRef; -var - Body: string; -begin - Body := ExtractDoSomethingBody(GenIR(SrcIntfConstParam)); - AssertTrue('DoSomething emitted', Body <> ''); - AssertFalse('no addref for interface const param', - Pos('call $_ClassAddRef', Body) > 0); - AssertFalse('no release for interface const param', - Pos('call $_ClassRelease', Body) > 0); -end; - procedure TARCTests.TestARC_IntfVarParam_NoAddRef; var Body: string; diff --git a/compiler/src/test/pascal/cp.test.e2e.arc.pas b/compiler/src/test/pascal/cp.test.e2e.arc.pas index 35ac809..401ae9f 100644 --- a/compiler/src/test/pascal/cp.test.e2e.arc.pas +++ b/compiler/src/test/pascal/cp.test.e2e.arc.pas @@ -27,8 +27,14 @@ type procedure TestRun_WeakRef_BreaksCycle_Valgrind; procedure TestRun_ClassDestroy_FreesBuffer_Valgrind; procedure TestRun_TListARC_Valgrind; - procedure TestRun_ConstParam_NoRetainRelease_Valgrind; procedure TestRun_IntfValueParam_Retained_Valgrind; + { Regression: a const string param bound to a freshly-built TEMPORARY must + stay alive for the whole call. An over-eager "elide retain/release for + const params" optimisation (reverted) dropped the callee-side retain, so + the temporary was freed mid-call -> use-after-free in the RTL string + routines. A string LITERAL hides this (it is immortal); a concatenation + result does not. } + procedure TestRun_ConstStringTemp_StaysAlive_Valgrind; { Three instantiations of the same generic class: verifies the Pointer→class ARC coercion bug is fixed (the 3rd instantiation no longer uses freed memory). } procedure TestRun_ThreeGenericInstances_AllWork; @@ -316,49 +322,6 @@ const end. '''; -const - { const class/string params: the codegen elides the callee-side - _ClassAddRef/_ClassRelease and _StringAddRef/_StringRelease pair because - the caller keeps the argument alive for the whole call. This program - passes both a class instance and a string as const and uses them inside - the callee; valgrind must report no leak and no use-after-free, proving - the elision is balanced (no over-release, no missing retain). } - SrcConstParamNoRetain = ''' - program P; - type - TThing = class - FValue: Integer; - end; - procedure Show(const T: TThing; const S: string); - begin - WriteLn(S); - WriteLn(T.FValue) - end; - var - A: TThing; - begin - A := TThing.Create; - A.FValue := 99; - Show(A, 'hello') - end. - '''; - -procedure TE2EArcTests.TestRun_ConstParam_NoRetainRelease_Valgrind; -var Output: string; RCode: Integer; Log: string; OK: Boolean; -begin - if not ToolchainAvailable then begin Ignore('toolchain unavailable'); Exit; end; - AssertTrue('compile+run', CompileAndRun(SrcConstParamNoRetain, Output, RCode)); - AssertEquals('exit 0', 0, RCode); - AssertEquals('stdout', 'hello' + LE + '99' + LE, Output); - if not ValgrindAvailable then begin Ignore('valgrind not installed'); Exit; end; - OK := RunUnderValgrind(SrcConstParamNoRetain, Log); - if not OK then - begin - if Log = '' then Log := '(valgrind produced no output)'; - Fail('const-param ARC elision unbalanced — valgrind reports:' + LE + Log); - end; -end; - const { By-value interface param: the callee must retain it on entry, because the caller's reference can be dropped during the call. Here DoSomething nils @@ -414,6 +377,44 @@ begin end; end; +const + { Pass a freshly-concatenated temporary as a const string param, then read + it inside the callee. Without the callee-side retain the temporary's + refcount hits zero at the call site and the string is freed before Use + reads it — a use-after-free valgrind catches. A literal would not expose + this (it is immortal), so the argument must be a built-at-runtime value. } + SrcConstStringTemp = ''' + program P; + procedure Use(const S: string); + begin + WriteLn(S); + WriteLn(Length(S)) + end; + var + A, B: string; + begin + A := 'hello'; + B := 'world'; + Use(A + ' ' + B) + end. + '''; + +procedure TE2EArcTests.TestRun_ConstStringTemp_StaysAlive_Valgrind; +var Output: string; RCode: Integer; Log: string; OK: Boolean; +begin + if not ToolchainAvailable then begin Ignore('toolchain unavailable'); Exit; end; + AssertTrue('compile+run', CompileAndRun(SrcConstStringTemp, Output, RCode)); + AssertEquals('exit 0', 0, RCode); + AssertEquals('stdout', 'hello world' + LE + '11' + LE, Output); + if not ValgrindAvailable then begin Ignore('valgrind not installed'); Exit; end; + OK := RunUnderValgrind(SrcConstStringTemp, Log); + if not OK then + begin + if Log = '' then Log := '(valgrind produced no output)'; + Fail('const string temp freed mid-call — valgrind reports:' + LE + Log); + end; +end; + procedure TE2EArcTests.TestRun_ThreeGenericInstances_AllWork; var Output: string; RCode: Integer; begin diff --git a/docs/future-improvements.adoc b/docs/future-improvements.adoc index 24fa7bc..1059ab0 100644 --- a/docs/future-improvements.adoc +++ b/docs/future-improvements.adoc @@ -1082,3 +1082,52 @@ to distinguish this from simple enums so existing code is unaffected. distinct `enum class` type form. Breaking from simple enum semantics is acceptable provided it is a new keyword. + +== ARC: elide retain/release for const value parameters + +A `const` parameter promises the caller keeps the argument alive for the +duration of the call, so the callee-side `_StringAddRef`/`_StringRelease` +(and `_ClassAddRef`/`_ClassRelease`) pair on entry/exit is, in principle, +redundant. Eliding it removes two RTL calls per const string/class/interface +parameter — a measurable saving on hot paths that thread strings through many +`const` parameters (the parser and semantic analyser do this heavily). + +This optimisation was implemented (commit `5a5b5d4`) and reverted after it +introduced a use-after-free. The reason it is unsound as stated: + +* The "caller keeps it alive" premise holds for a *named* argument the caller + owns for the whole call, but *not* for a *temporary* bound to the const + parameter — e.g. `Use(A + ' ' + B)`, where the concatenation result is a + freshly-allocated string whose only reference is the argument slot itself. + With the callee-side retain elided, that temporary's refcount reaches zero + at the call boundary and the string is freed *before* the callee reads it. + +* This bites the RTL especially hard: routines such as `_StringCopy` and + `StrHead` take `const string` parameters and are frequently called with + built-at-runtime temporaries. A miscompiled RTL is self-reproducing under + self-hosting — the defect only manifests at the *second* generation (the + compiler that emits the broken RTL is itself fine), so a single fixpoint + step (stage-2 == stage-3) does not necessarily expose it, and the compiler's + own sources may not trigger the exact aliasing case. The original change + passed its valgrind test only because that test passed a string *literal* + (immortal), not a temporary. + +To re-attempt the optimisation safely, the elision must be conditioned on the +*argument*, not merely the parameter: + +* Elide the callee-side retain/release only when the caller can prove the + argument outlives the call without the callee's reference — i.e. a named + local/global/parameter the caller already retains — and *keep* the + retain/release when the argument is a temporary (function/property result, + concatenation, cast, or any `ExprOwnsRef` expression). + +* Equivalently, move the responsibility to the *call site*: for a const + parameter bound to a temporary, the caller retains the temporary across the + call and releases it afterwards (caller-side balancing), leaving the callee + free of ARC traffic. This keeps the optimisation's benefit for the common + named-argument case while remaining correct for temporaries. + +* Mandatory regression coverage: a valgrind e2e test that passes a + *concatenation result* (not a literal) as a `const string` parameter and + reads it in the callee — see `TestRun_ConstStringTemp_StaysAlive_Valgrind` + in `cp.test.e2e.arc.pas`. Add the class- and interface-typed analogues.