Diagnose or hoist NativePtr.stackalloc where localloc is invalid IL

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

@@ -1,5 +1,7 @@
 ### Fixed
 
+* Fix `NativePtr.stackalloc` used inside the `with` handler, filter or `finally` block of a `try` expression producing an assembly that throws `InvalidProgramException` at method load. The `localloc` IL instruction it emits is rejected by the runtime inside an exception-handling region, so this is now reported at compile time as error FS3916. ([Issue #20295](https://github.com/dotnet/fsharp/issues/20295))
+* Fix `NativePtr.stackalloc` used as a chained base-constructor argument producing an assembly that throws `InvalidProgramException` at method load; the base-constructor arguments are now hoisted into locals before the uninitialized `this` is pushed. ([Issue #20295](https://github.com/dotnet/fsharp/issues/20295))
 * Fix `NativePtr.stackalloc` nested in a larger expression (e.g. a call argument or the right of an assignment) producing an assembly that throws `InvalidProgramException` at load. ([Issue #8083](https://github.com/dotnet/fsharp/issues/8083), [PR #20302](https://github.com/dotnet/fsharp/pull/20302))
 * Fix internal error "Unexpected generalized type variables when compiling an active pattern" when an active pattern is used in a `let` binding whose right-hand side is a generic value, e.g. `let (T) = id`. Such a binding is now checked like the equivalent `match` and is not generalized. ([Issue #16856](https://github.com/dotnet/fsharp/issues/16856), [PR #20383](https://github.com/dotnet/fsharp/pull/20383))
 * Fix Release-only (`--optimize+`) `System.InvalidProgramException` from `Seq.collect` / `yield!` over a value-type (struct) collection implementing `seq<'T>` (e.g. `ImmutableArray<_>`) when materialised with `List.ofSeq` / `Seq.toList` / `Seq.toArray` or a list/array comprehension. The collector lowering now boxes a struct sub-collection to `seq<'T>` before calling `AddMany`/`AddManyAndClose` (matching the coercion the type checker already inserts for `yield!`), and uses `unit` as the try/finally result type instead of the body type (removing a spurious `ldnull` store). ([Issue #20203](https://github.com/dotnet/fsharp/issues/20203))

src/Compiler/CodeGen/IlxGen.fs

@@ -1261,6 +1261,12 @@ and IlxGenEnv =
         /// Are we under the scope of a try, catch or finally? If so we can't tailcall. SEH = structured exception handling
         withinSEH: bool
 
+        /// Are we within the 'with'/filter/'finally'/fault handler region of a 'try' (but not merely its try body)?
+        /// The JIT rejects the 'localloc' IL instruction (emitted by NativePtr.stackalloc) inside such a region, so
+        /// emitting it here is reported as error FS3916. This is checked at codegen, after inlining and closure
+        /// conversion, so an escaping closure whose 'localloc' lives in its own method stays legal.
+        withinExnHandler: bool
+
         /// Suppresses filter block emission inside finally/fault handlers (workaround for dotnet/runtime#112406).
         insideFinallyOrFaultHandler: bool
 
@@ -3055,6 +3061,7 @@ let CodeGenThen (cenv: cenv) mgbuf (entryPointInfo, methodName, eenv, alreadyUse
         cgbuf
         { eenv with
             withinSEH = false
+            withinExnHandler = false
             insideFinallyOrFaultHandler = false
             liveLocals = IntMap.empty ()
             innerVals = innerVals
@@ -3166,6 +3173,21 @@ let ComputeDebugPointForBinding g bind =
 // Generate expressions
 //-------------------------------------------------------------------------
 
+/// True if evaluating this expression may emit the 'localloc' IL instruction (e.g. NativePtr.stackalloc,
+/// which the optimizer inlines to inline IL containing 'localloc' before IlxGen runs).
+let exprMayLocalloc expr =
+    (false, expr)
+    ||> FoldExpr
+            { ExprFolder0 with
+                exprIntercept =
+                    (fun _exprF noInterceptF z expr ->
+                        z
+                        || (match expr with
+                            | Expr.Op(TOp.ILAsm(instrs, _), _, _, _) -> instrs |> List.contains I_localloc
+                            | _ -> false)
+                        || noInterceptF false expr)
+            }
+
 let rec GenExpr cenv cgbuf eenv (expr: Expr) sequel =
     cenv.stackGuard.Guard(fun () ->
 
@@ -4717,21 +4739,47 @@ and GenApp (cenv: cenv) cgbuf eenv (f, fty, tyargs, curriedArgs, m) sequel =
                 else
                     mspec.DeclaringType
 
-            if isSuperInit || isSelfInit then
-                CG.EmitInstr cgbuf (pop 0) (Push [ thisTy ]) mkLdarg0
-
             let pendingUninitializedThis = (isSuperInit || isSelfInit) && not valu
 
-            if pendingUninitializedThis then
+            let genArgs () =
+                if not cenv.g.generateWitnesses || witnessInfos.IsEmpty then
+                    () // no witness args
+                else
+                    let _ctyargs, mtyargs = List.splitAt ctps.Length tyargs
+                    GenWitnessArgs cenv cgbuf eenv m mtps mtyargs
+
+                GenUntupledArgsDiscardingLoneUnit cenv cgbuf eenv m vref.NumObjArgs curriedArgInfos nowArgs
+
+            // An uninitialized 'this' cannot be spilled, so a 'localloc' emitted by a base/self-ctor
+            // argument while 'this' is pending on the stack yields invalid IL (InvalidProgramException at
+            // load). When that can happen, evaluate the args into locals first (at a clean stack), then
+            // push 'this' and reload them; left-to-right evaluation order is preserved and ordinary ctors
+            // are unaffected.
+            let hoistArgsBeforeThis =
+                pendingUninitializedThis && List.exists exprMayLocalloc nowArgs
+
+            if hoistArgsBeforeThis then
+                let stackBefore = cgbuf.GetCurrentStack()
+                genArgs ()
+
+                let argTys =
+                    let stackAfter = cgbuf.GetCurrentStack()
+                    stackAfter |> List.truncate (stackAfter.Length - stackBefore.Length)
+
+                let argLocals = [ for ty in argTys -> cgbuf.SpillToLocal(ty, false) ]
+                CG.EmitInstr cgbuf (pop 0) (Push [ thisTy ]) mkLdarg0
                 cgbuf.StartUninitializedThisOnStack()
 
-            if not cenv.g.generateWitnesses || witnessInfos.IsEmpty then
-                () // no witness args
+                for local in List.rev argLocals do
+                    cgbuf.ReloadFromLocal local
             else
-                let _ctyargs, mtyargs = List.splitAt ctps.Length tyargs
-                GenWitnessArgs cenv cgbuf eenv m mtps mtyargs
+                if isSuperInit || isSelfInit then
+                    CG.EmitInstr cgbuf (pop 0) (Push [ thisTy ]) mkLdarg0
+
+                if pendingUninitializedThis then
+                    cgbuf.StartUninitializedThisOnStack()
 
-            GenUntupledArgsDiscardingLoneUnit cenv cgbuf eenv m vref.NumObjArgs curriedArgInfos nowArgs
+                genArgs ()
 
             // Generate laterArgs (for effects) and save
             LocalScope "callstack" cgbuf (fun scopeMarks ->
@@ -5178,6 +5226,7 @@ and GenTryWith cenv cgbuf eenv (e1, valForFilter: Val, filterExpr, valForHandler
 
                 let eenvinner =
                     { eenvinner with
+                        withinExnHandler = true
                         exitSequel = sequelOnBranches
                     }
                 // We emit the debug point for the 'with' keyword span on the start of the filter
@@ -5248,6 +5297,7 @@ and GenTryWith cenv cgbuf eenv (e1, valForFilter: Val, filterExpr, valForHandler
 
                 let eenvinner =
                     { eenvinner with
+                        withinExnHandler = true
                         exitSequel = exitSequel
                     }
 
@@ -5293,6 +5343,7 @@ and GenTryFinally cenv cgbuf eenv (bodyExpr, handlerExpr, m, resTy, spTry, spFin
 
         let eenvHandler =
             { eenvinner with
+                withinExnHandler = true
                 insideFinallyOrFaultHandler = true
             }
 
@@ -5600,6 +5651,13 @@ and GenAsmCode cenv cgbuf eenv (il, tyargs, args, returnTys, m) sequel =
         && ilReturnTys |> List.forall (fun ty -> ty <> ILType.Void)
         ->
 
+        // The JIT rejects 'localloc' inside an exception-handling region, producing an
+        // InvalidProgramException at method load. By this point inlining and closure conversion have run,
+        // so eenv.withinExnHandler reflects the true handler region: an escaping closure carrying the
+        // 'localloc' into its own method has had the flag reset and stays legal.
+        if eenv.withinExnHandler then
+            errorR (Error(FSComp.SR.chkNativePtrStackallocInHandler (), m))
+
         CG.EmitLocallocCode cgbuf (fun () ->
             GenExprs cenv cgbuf eenv args
             CG.EmitInstrs cgbuf (pop args.Length) (Push ilReturnTys) ilAfterInst)
@@ -5809,17 +5867,41 @@ and GenILCall
         else
             ilMethSpec.DeclaringType
 
-    // Load the 'this' pointer to pass to the superclass constructor. This argument is not
-    // in the expression tree since it can't be treated like an ordinary value
-    if isSuperInit then
-        CG.EmitInstr cgbuf (pop 0) (Push [ thisTy ]) mkLdarg0
-
+    // An uninitialized 'this' cannot be spilled, so a 'localloc' emitted by a base-ctor argument while
+    // 'this' is pending on the stack produces invalid IL (InvalidProgramException at load). When that
+    // can happen, evaluate the args into locals first (at a clean stack), then push 'this' and reload
+    // them. Left-to-right evaluation order is preserved; ordinary base ctors are unaffected.
     let pendingUninitializedThis = isSuperInit && not valu
 
-    if pendingUninitializedThis then
+    let hoistArgsBeforeThis =
+        pendingUninitializedThis && List.exists exprMayLocalloc argExprs
+
+    if hoistArgsBeforeThis then
+        let g = cenv.g
+
+        let argLocals =
+            [
+                for argExpr in argExprs ->
+                    let ilTy = argExpr |> tyOfExpr g |> GenType cenv m eenv.tyenv
+                    GenExpr cenv cgbuf eenv argExpr Continue
+                    cgbuf.SpillToLocal(ilTy, false)
+            ]
+
+        CG.EmitInstr cgbuf (pop 0) (Push [ thisTy ]) mkLdarg0
         cgbuf.StartUninitializedThisOnStack()
 
-    GenExprs cenv cgbuf eenv argExprs
+        for local in argLocals do
+            cgbuf.ReloadFromLocal local
+    else
+        // Load the 'this' pointer to pass to the superclass constructor. This argument is not
+        // in the expression tree since it can't be treated like an ordinary value
+        if isSuperInit then
+            CG.EmitInstr cgbuf (pop 0) (Push [ thisTy ]) mkLdarg0
+
+        if pendingUninitializedThis then
+            cgbuf.StartUninitializedThisOnStack()
+
+        GenExprs cenv cgbuf eenv argExprs
 
     let il =
         if newobj then
@@ -13066,6 +13148,7 @@ let GetEmptyIlxGenEnv (g: TcGlobals) ccu =
         innerVals = []
         sigToImplRemapInfo = [] (* "module remap info" *)
         withinSEH = false
+        withinExnHandler = false
         insideFinallyOrFaultHandler = false
         isInLoop = false
         initLocals = true

src/Compiler/FSComp.txt

@@ -1821,3 +1821,4 @@ featureRecordSpreads,"record type and expression spreads"
 3913,tcExtendedLayoutCannotBeUsedOnUnions,"The 'ExtendedLayoutAttribute' cannot be applied to discriminated unions"
 3914,tcExtendedLayoutStructMustHaveInstanceField,"A struct with the 'ExtendedLayoutAttribute' must have at least one instance field"
 3915,tcTupleTypeExtensionTooManyElements,"Tuple type extensions are supported only for tuples of up to 7 elements, but this tuple type has %d elements. Extensions of larger tuples are not supported."
+3916,chkNativePtrStackallocInHandler,"'NativePtr.stackalloc' cannot be used inside the 'with' handler, filter or 'finally' block of a 'try' expression. The 'localloc' IL instruction this emits is rejected inside an exception-handling region, producing a 'System.InvalidProgramException' when the method is loaded."