Roslyn: Avoid setting localsinit flag for unsafe methods

[RossNordby]

(Branched from dotnet/coreclr#1279)

Currently, MethodBody.LocalsAreZeroed just returns true, so (most) methods end up with the localsinit flag set.

This can cause significant costs when using unsafe code and stackalloc. For example, the JIT is required by CLI spec to obey the localsinit flag, resulting in lengthy zeroing loops on stackalloc'd memory:

000007FE8D8A05C4  mov         eax,10000h
000007FE8D8A05C9  push        0
000007FE8D8A05CB  push        0
000007FE8D8A05CD  sub         rax,10h
000007FE8D8A05D1  jne         000007FE8D8A05C9

Since unsafe code and stackalloc are often used in performance sensitive situations, it would be nice if the JIT wasn't forced to do this.

Returning false for LocalsAreZeroed for unsafe-including methods would free the JIT to use a faster path.

Would this break anything? It appears the main issue would be around verifiability, but unsafe code is unverifiable anyway.

[HaloFour]

It's a shame that it's an all-or-none proposition. Having the JIT not zero-out stackalloc'd memory seems like an easy win and I doubt much code depends on that memory being zeroed-out (although still a possibility), but it might be more likely that someone depends on one of the other variables being zeroed-out. Given this I think I'd rather see a new keyword (or attribute) to inform Roslyn that the method should be emitted without that flag set.

[RossNordby]

I may have missed it, but I couldn't find anything perfectly explicit regarding the contents of unassigned local variables. The closest I found was in 18.5.4:

The & operator does not require its argument to be definitely assigned, but following an & operation, the variable to which the operator is applied is considered definitely assigned in the execution path in which the operation occurs. It is the responsibility of the programmer to ensure that correct initialization of the variable actually does take place in this situation.

That at least suggests that it is undefined. 18.7.3 and 18.8 explicitly state that the initial content of local fixed size buffers and stackalloc'd memory is undefined. So, it seems that using unsafe as the behavior switch could be okay strictly from the perspective of the specification.

That said, changing the behavior does indeed run the risk of breaking existing code, even if that code was relying on undefined behavior. I'd be happy with applying an attribute to get the desired behavior if the risk is deemed too great.

[gafter]

We would have to analyze this very carefully to ensure we don't introduce unintentional incompatibilities.

[gafter]

It is possible to observe this, for example by passing an uninitialized local as an out parameter to a method in VB, where VB can actually read the value.

[GSPP]

Seems like the C# compiler could introduce zeroing assignments at the start of the method in any case it needs the localsinit behavior.

(For that reason I don't understand why localsinit even exists at the CLR level? Is there an important use case?)

[omariom]

@jaredpar
What was decided on this issue?

[glenn-slayden]

The out solution has promise. Indeed, my (mixed) C# (+IL) class library includes the ability to call the following helpers:

.method public static void Touch<T>(!!T& d) aggressiveinlining { ret }
.method public static void Touch<T1, T2>(!!T1& d1, !!T2& d2) aggressiveinlining { ret }
.method public static void Touch<T1, T2, T3>(!!T1& d1, !!T2& d2, !!T3& d3) aggressiveinlining { ret }

These are greatly useful to C# coding, i.e., especially for satisfying struct initialization requirement in the face of LayoutKind.Explicit, as well as the localsinit issue raised here. And of course this IL usually ends up being entirely optimized away, contributing zero x86/x64 instructions.

^{p.s. In case you were wondering, d stands for 'dummy'.}