Support SkipLocalsinit

[kerams]

Implements fsharp/fslang-suggestions#990.

When applied on a module, class or method/function, all nested methods/functions and lambdas must not set the .locals init flag (paraphrased based on docs).

This naive implementation assumes that locals should be initialized by default unless stated otherwise, though that may change in the future. It seems to work well at first glance with the exception of using the attribute on a class. The eenvinner in GenTypeDef correctly recognizes that locals should not be initialized, but the env instance is supposedly not passed down to GenMethodForBinding for the class' instance and static methods, unlike when the attribute is used on an entire module. Any clues?

Please let me know if what I did makes sense. I'll also need some pointers on authoring tests for this kind of thing.

[TIHan]

Looks straight forward so far. But yea you are right, the eenvinner with the initLocals=true isn't getting passed down. Will have to look at a bit more closely at this.

[kerams]

Do I just create a test file similar to https://github.com/dotnet/fsharp/blob/main/tests/FSharp.Compiler.ComponentTests/EmittedIL/TailCalls.fs or do I have to muck around with stuff like this https://github.com/dotnet/fsharp/tree/main/tests/fsharpqa/Source/CodeGen/EmittedIL?

[vzarytovskii]

Do I just create a test file similar to https://github.com/dotnet/fsharp/blob/main/tests/FSharp.Compiler.ComponentTests/EmittedIL/TailCalls.fs or do I have to muck around with stuff like this https://github.com/dotnet/fsharp/tree/main/tests/fsharpqa/Source/CodeGen/EmittedIL?

Creating a test in ComponentTests suite should be fine.

[kerams]

@vzarytovskii, the problem here is the fact that the attribute is .NET 5+ only, so the test snippet needs to be compiled against that TFM. What now?

[kerams]

It looks like the test only failed during the net472 run, so one option is running the test behind something like #if NET_5, even though there is no reason why it shouldn't run on net472 with an explicit target framework.

[kerams]

--targetprofile:netcore does not guarantee consistent results across platforms either. I'm afraid being able to specify the TFM is the only option left.

Or maybe I'm misunderstanding things and the net472 version of fsc cannot compile code for .NET Core?

[kerams]

I'm not quite sure if this shouldn't be tryFindSysAttrib. Attributes above use both find and try find and I can't tell what the deciding factor is. It does not throw even on net472, so I guess it's fine?

[TIHan]

This is fine. You can check to see if you can deref it. If you can't, then it couldn't find it.

[kerams]

@TIHan, got it. Methods don't get the class's eenvinner because they are processed after, not inside GenTypeDef, so every method has to individually check its as well as the parent class's attributes.

fsharp/src/fsharp/IlxGen.fs

Lines 6894 to 6901 in 04ef067

	and GenModuleDef cenv (cgbuf: CodeGenBuffer) qname lazyInitInfo eenv x =
	match x with
	| TMDefRec(_isRec, tycons, mbinds, m) ->
	tycons |> List.iter (fun tc ->
	if tc.IsExceptionDecl
	then GenExnDef cenv cgbuf.mgbuf eenv m tc
	else GenTypeDef cenv cgbuf.mgbuf lazyInitInfo eenv m tc)
	mbinds |> List.iter (GenModuleBinding cenv cgbuf qname lazyInitInfo eenv m)

Tests pass when run on against .NET 5, so this is ready for review. I don't know what to do about 'Build Windows desktop_release'.

[kerams]

I'd appreciate help with that test.

[TIHan]

Apologies for the delay. I will find time today to look at this. I've been doing similar stuff in IlxGen lately.

[TIHan]

This looks great! You don't have to worry about checking for a deref on the attribute; HasFSharpAttribute will return false if the SkipLocalsInitAttribute could not be found in the framework.

In regards to the test, you can put a #if NETCOREAPP around the test so it will only run on netcore and not framework.

[kerams]

Thanks, hopefully it's green and ready to go now.

[cartermp]

I think this looks good, but should it also be backed by LangVersion? It's not really a new feature per say, just respecting an existing attribute. So I say it's not necessary. But I'm happy to hear arguments otherwise.

[vzarytovskii]

Thanks!

[vzarytovskii]

That should be good testing wise, I'll see what we can do with the x-framework IL validation.

[kerams]

It would be great if you could pass TFM to compile because the .NET Framework version of the compiler needs to be able to compile code like this, so it makes sense to test it too, but I understand a bit of infrastructure plumbing work may be required. Plus it will also help with cases where binaries for different TFMs end up with slightly different IL, making these tests tricky, as I've mentioned elsewhere.

[kerams]

On the off chance that you're waiting to hear my opinion, I don't think LangVersion is needed either. The attribute is fairly new and, more importantly, currently has no impact in F#, so I find it extremely unlikely there's code out there that uses it. The risks (someone using SkipLocalsInit, but actually relying on initialized locals, recompiling their code and suddenly getting different runtime behavior) are thus negligible.

That's assuming the design (primarily the way the attribute affects types, methods, nested functions and lambdas) is final.

[cartermp]

Agreed. Let's merge it!

[dsyme]

There is a problem here

We use locals zero init to generate default values of arbitrary value types here:

            let ilTy = GenType cenv.amap m eenv.tyenv ty
            LocalScope "ilzero" cgbuf (fun scopeMarks ->
                let locIdx, realloc, _ =
                    // Ensure that we have an g.CompilerGlobalState
                    assert(g.CompilerGlobalState |> Option.isSome)
                    AllocLocal cenv cgbuf eenv true (g.CompilerGlobalState.Value.IlxGenNiceNameGenerator.FreshCompilerGeneratedName ("default", m), ilTy, false) scopeMarks
                // "initobj" (Generated by EmitInitLocal) doesn't work on byref types
                // But ilzero(&ty) only gets generated in the built-in get-address function so
                // we can just rely on zeroinit of all IL locals.
                if realloc then
                    match ilTy with
                    | ILType.Byref _ -> ()
                    | _ -> EmitInitLocal cgbuf ilTy locIdx
                EmitGetLocal cgbuf ilTy locIdx
            )

You should change the if realloc to be if realloc or <skipping locals init>.

However that leaves a separate problem about how to generate a zero byref value, because from the comment initobj evidently can't be used with byref type. I think if this code is generated for the ILType.Byref case we should still set the zero init flag. I'm not entirely sure what code would generate a zero byref value (you can't do Unchecked.defaultof<byref<int>>) but it may be possible somehow

[kerams]

@dsyme

This code

[<System.Runtime.CompilerServices.SkipLocalsInit>]
let z () =
    let mutable a = Unchecked.defaultof<System.DateTime>
    a <- Unchecked.defaultof<System.DateTime>

generates

.locals (
	[0] valuetype [System.Runtime]System.DateTime,
	[1] valuetype [System.Runtime]System.DateTime
)

/* 0x0000025C 07           */ IL_0000: ldloc.1
/* 0x0000025D 0A           */ IL_0001: stloc.0
/* 0x0000025E 1201         */ IL_0002: ldloca.s  V_1
/* 0x00000260 FE1503000001 */ IL_0004: initobj   [System.Runtime]System.DateTime
/* 0x00000266 07           */ IL_000A: ldloc.1
/* 0x00000267 0A           */ IL_000B: stloc.0
/* 0x00000268 2A           */ IL_000C: ret

With if realloc && eenv.initLocals then I get

.locals (
	[0] valuetype [System.Runtime]System.DateTime,
	[1] valuetype [System.Runtime]System.DateTime
)

/* 0x0000025C 07           */ IL_0000: ldloc.1
/* 0x0000025D 0A           */ IL_0001: stloc.0
/* 0x0000025E 07           */ IL_0002: ldloc.1
/* 0x0000025F 0A           */ IL_0003: stloc.0
/* 0x00000260 2A           */ IL_0004: ret

Is that what you want to see? I'm not sure I follow your second point. If the compiler didn't generate init code for byrefs before, why would SkipLocalsInitAttribute make a difference?

[kerams]

Ah, perhaps you want to always zero init when locals are no longer initialized, so Unchecked.defaultof would not return the default but an undefined/invalid state in the case of structs?

[dsyme]

I think you want if realloc || not eenv.initLocals

[kerams]

Ok, will send a PR with a test for this.