Skip to content

Commit 1de7a62

Browse files
CopilotT-Gro
andauthored
Fix F# compiler to prevent tail call emission when pinned locals are present (#18893)
* Initial plan * Add tracking for pinned locals in TailCallChecks (WIP) Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com> * Add pinned locals tracking to IlxGen (still WIP - tail calls not blocked yet) Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com> * Implement fix to prevent tail calls when pinned locals are present Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com> * Fix PR review comments: remove mutable, gitignore .trx files, add recursive test, add release notes, format code Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com> * Move release notes from 9.0.300 to 10.0.100 as requested Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
1 parent c4001e5 commit 1de7a62

File tree

5 files changed

+76
-2
lines changed

5 files changed

+76
-2
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,7 @@ positive.exe
142142

143143
# Test result files
144144
tests/**/TestResults/*.trx
145+
TestResults/*.trx
145146

146147
# Standard output/error files in root directory
147148
StandardOutput.txt

docs/release-notes/.FSharp.Compiler.Service/10.0.100.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
### Fixed
1111

12+
* Fix F# compiler to prevent tail call emission when pinned locals are present ([PR #XXXX](https://github.com/dotnet/fsharp/pull/XXXX))
1213
* Fix SignatureHash to include constant values in hash computation ([Issue #18758](https://github.com/dotnet/fsharp/issues/18758))
1314
* Fix parsing errors using anonymous records and units of measures ([PR #18543](https://github.com/dotnet/fsharp/pull/18543))
1415
* Fix parsing errors using anonymous records and code quotations ([PR #18603](https://github.com/dotnet/fsharp/pull/18603))

src/Compiler/Checking/TailCallChecks.fs

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,9 @@ type cenv =
7878

7979
/// Values in module that have been marked [<TailCall>]
8080
mustTailCall: Zset<Val>
81+
82+
/// Indicates whether the current method has pinned locals that would prevent tail calls
83+
hasPinnedLocals: bool
8184
}
8285

8386
override x.ToString() = "<cenv>"
@@ -202,6 +205,7 @@ let CheckForNonTailRecCall (cenv: cenv) expr (tailCall: TailCall) =
202205
&& not (IsValRefIsDllImport cenv.g vref)
203206
&& not isCCall
204207
&& not hasByrefArg
208+
&& not cenv.hasPinnedLocals
205209

206210
noTailCallBlockers // blockers that will prevent the IL level from emitting a tail instruction
207211
else
@@ -730,11 +734,26 @@ and CheckBinding cenv alwaysCheckNoReraise ctxt (TBind(v, bindRhs, _) as bind) :
730734
| Some info -> info
731735
| _ -> ValReprInfo.emptyValData
732736

737+
// Check if this binding introduces a pinned local
738+
let cenv =
739+
if v.IsFixed then
740+
{ cenv with hasPinnedLocals = true }
741+
else
742+
cenv
743+
733744
CheckLambdas isTop (Some v) cenv v.ShouldInline valReprInfo tailCall alwaysCheckNoReraise bindRhs v.Range v.Type ctxt
734745

735746
and CheckBindings cenv binds =
736747
for bind in binds do
737-
CheckBinding cenv false PermitByRefExpr.Yes bind
748+
let (TBind(v, _, _)) = bind
749+
// Update the environment if this binding is fixed
750+
let currentCenv =
751+
if v.IsFixed then
752+
{ cenv with hasPinnedLocals = true }
753+
else
754+
cenv
755+
756+
CheckBinding currentCenv false PermitByRefExpr.Yes bind
738757

739758
let CheckModuleBinding cenv (isRec: bool) (TBind _ as bind) =
740759

@@ -871,6 +890,7 @@ let CheckImplFile (g: TcGlobals, amap, reportErrors, implFileContents) =
871890
stackGuard = StackGuard(PostInferenceChecksStackGuardDepth, "CheckImplFile")
872891
amap = amap
873892
mustTailCall = Zset.empty valOrder
893+
hasPinnedLocals = false
874894
}
875895

876896
CheckDefnInModule cenv implFileContents

src/Compiler/CodeGen/IlxGen.fs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2689,6 +2689,10 @@ type CodeGenBuffer(m: range, mgbuf: AssemblyBuilder, methodName, alreadyUsedArgs
26892689
j, true
26902690
| None -> cgbuf.AllocLocal(ranges, ty, isFixed, canBeReallocd), false
26912691

2692+
/// Check if any locals have been allocated as pinned/fixed
2693+
member _.HasPinnedLocals() =
2694+
locals |> Seq.exists (fun (_, _, isFixed, _) -> isFixed)
2695+
26922696
member _.Close() =
26932697

26942698
let instrs = codebuf.ToArray()
@@ -4371,6 +4375,7 @@ and GenApp (cenv: cenv) cgbuf eenv (f, fty, tyargs, curriedArgs, m) sequel =
43714375
isDllImport,
43724376
isSelfInit,
43734377
makesNoCriticalTailcalls,
4378+
cgbuf,
43744379
sequel
43754380
)
43764381
else
@@ -4482,11 +4487,15 @@ and CanTailcall
44824487
isDllImport,
44834488
isSelfInit,
44844489
makesNoCriticalTailcalls,
4490+
cgbuf: CodeGenBuffer,
44854491
sequel
44864492
) =
44874493

44884494
// Can't tailcall with a struct object arg since it involves a byref
44894495
// Can't tailcall with a .NET 2.0 generic constrained call since it involves a byref
4496+
// Can't tailcall when there are pinned locals since the stack frame must remain alive
4497+
let hasPinnedLocals = cgbuf.HasPinnedLocals()
4498+
44904499
if
44914500
not hasStructObjArg
44924501
&& Option.isNone ccallInfo
@@ -4495,6 +4504,7 @@ and CanTailcall
44954504
&& not isDllImport
44964505
&& not isSelfInit
44974506
&& not makesNoCriticalTailcalls
4507+
&& not hasPinnedLocals
44984508
&&
44994509

45004510
// We can tailcall even if we need to generate "unit", as long as we're about to throw the value away anyway as par of the return.
@@ -4693,7 +4703,7 @@ and GenIndirectCall cenv cgbuf eenv (funcTy, tyargs, curriedArgs, m) sequel =
46934703
check ilxClosureApps
46944704

46954705
let isTailCall =
4696-
CanTailcall(false, None, eenv.withinSEH, hasByrefArg, false, false, false, false, sequel)
4706+
CanTailcall(false, None, eenv.withinSEH, hasByrefArg, false, false, false, false, cgbuf, sequel)
46974707

46984708
CountCallFuncInstructions()
46994709

@@ -5431,6 +5441,7 @@ and GenILCall
54315441
isDllImport,
54325442
false,
54335443
makesNoCriticalTailcalls,
5444+
cgbuf,
54345445
sequel
54355446
)
54365447

tests/FSharp.Compiler.ComponentTests/EmittedIL/TailCalls.fs

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,3 +235,44 @@ let run() = let mutable x = 0 in foo(&x,&x,5)
235235
"""
236236
]
237237

238+
[<Fact>]
239+
let ``TailCall 06 - No tail call with pinned locals``() =
240+
FSharp """
241+
module TailCall06
242+
let foo(x:int, y) = printfn "%d" x
243+
let run() =
244+
use ptr = fixed [| 1; 2; 3 |]
245+
foo(42, 5)
246+
"""
247+
|> compileWithTailCalls
248+
|> shouldSucceed
249+
|> verifyIL [
250+
"""
251+
IL_0040: ldc.i4.s 42
252+
IL_0042: ldc.i4.5
253+
IL_0043: call void TailCall06::foo<int32>(int32,
254+
!!0)
255+
"""
256+
]
257+
258+
[<Fact>]
259+
let ``TailCall 07 - No tail call with pinned locals in recursive function``() =
260+
FSharp """
261+
module TailCall07
262+
let rec countdown n =
263+
use ptr = fixed [| n |]
264+
if n <= 0 then
265+
0
266+
else
267+
countdown (n - 1)
268+
"""
269+
|> compileWithTailCalls
270+
|> shouldSucceed
271+
|> verifyIL [
272+
"""
273+
.locals init (native int V_0,
274+
int32[] V_1,
275+
int32& pinned V_2)
276+
"""
277+
]
278+

0 commit comments

Comments
 (0)