Revert "perf(arc): elide retain/release for const string and class params"

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.

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

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;

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;

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

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.