Constrained execution behavior change in .net7 preview 6

[jaredpar]

Description

The behavior around constrained execution calls observably changed between .net7 preview 5 and preview 6. This was caught by a regression test in the roslyn repository: dotnet/roslyn#63221.

This is an old regression test tracked by the following work item. https://vstfdevdiv/DevDiv2/DevDiv/_workitems/edit/1021941

Reproduction Steps

Create a Visual Basic console application with the following code:

Imports System

Interface IMoveable
    Property Position As Integer
End Interface

Class Item
    Implements IMoveable

    Public Property Name As String

    Public Property Position As Integer Implements IMoveable.Position
        Get
            Console.WriteLine("Position get for item '{0}'", Me.Name)
            Return 0
        End Get
        Set
            Console.WriteLine("Position set for item '{0}'", Me.Name)
        End Set
    End Property
End Class

Class Program
    Shared Sub Main()
        Dim item = New Item With {.Name = "Goo"}
        Shift(item)
    End Sub

    Shared Sub Shift(Of T As {Class, IMoveable})(item As T)
        item.Position += GetOffset(item)
    End Sub

    Shared Function GetOffset(Of T)(ByRef item As T) As Integer
        item = DirectCast(DirectCast(New Item With {.Name = "Bar"}, IMoveable), T)
        Return 0
    End Function
End Class

And the following project file:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <RootNamespace>vbconsole</RootNamespace>
    <TargetFrameworks>net6.0;net7.0</TargetFrameworks>
  </PropertyGroup>

</Project>

Expected behavior

C:\Users\jaredpar\code\temp\vbconsole
> dotnet run --configuration Release --framework net6.0
Position get for item 'Goo'
Position set for item 'Goo'
C:\Users\jaredpar\code\temp\vbconsole 
> dotnet run --configuration Release --framework net7.0
Position get for item 'Goo'
Position set for item 'Goo'

Actual behavior

C:\Users\jaredpar\code\temp\vbconsole
> dotnet run --configuration Release --framework net6.0
Position get for item 'Goo'
Position set for item 'Goo'
C:\Users\jaredpar\code\temp\vbconsole 
> dotnet run --configuration Release --framework net7.0
Position get for item 'Goo'
Position set for item 'Bar'

Regression?

Yes this is a regression from net7.0 preview 5 and all previous .net runtime releases (core and framework).

Known Workarounds

No response

Configuration

No response

Other information

No response

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

The behavior around constrained execution calls observably changed between .net7 preview 5 and preview 6. This was caught by a regression test in the roslyn repository: dotnet/roslyn#63221.

This is an old regression test tracked by the following work item. This link is unavailable at the moment, trying to track down the current link: http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/1021941

Reproduction Steps

Create a Visual Basic console application with the following code:

Imports System

Interface IMoveable
    Property Position As Integer
End Interface

Class Item
    Implements IMoveable

    Public Property Name As String

    Public Property Position As Integer Implements IMoveable.Position
        Get
            Console.WriteLine("Position get for item '{0}'", Me.Name)
            Return 0
        End Get
        Set
            Console.WriteLine("Position set for item '{0}'", Me.Name)
        End Set
    End Property
End Class

Class Program
    Shared Sub Main()
        Dim item = New Item With {.Name = "Goo"}
        Shift(item)
    End Sub

    Shared Sub Shift(Of T As {Class, IMoveable})(item As T)
        item.Position += GetOffset(item)
    End Sub

    Shared Function GetOffset(Of T)(ByRef item As T) As Integer
        item = DirectCast(DirectCast(New Item With {.Name = "Bar"}, IMoveable), T)
        Return 0
    End Function
End Class

And the following project file:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <RootNamespace>vbconsole</RootNamespace>
    <TargetFrameworks>net6.0;net7.0</TargetFrameworks>
  </PropertyGroup>

</Project>

Expected behavior

C:\Users\jaredpar\code\temp\vbconsole
> dotnet run --configuration Release --framework net6.0
Position get for item 'Goo'
Position set for item 'Goo'
C:\Users\jaredpar\code\temp\vbconsole 
> dotnet run --configuration Release --framework net7.0
Position get for item 'Goo'
Position set for item 'Bar'

Actual behavior

C:\Users\jaredpar\code\temp\vbconsole
> dotnet run --configuration Release --framework net6.0
Position get for item 'Goo'
Position set for item 'Goo'
C:\Users\jaredpar\code\temp\vbconsole 
> dotnet run --configuration Release --framework net7.0
Position get for item 'Goo'
Position set for item 'Goo'

Regression?

Yes this is a regression from net7.0 preview 5 and all previous .net runtime releases (core and framework).

Known Workarounds

No response

Configuration

No response

Other information

No response

Author:	jaredpar
Assignees:	-
Labels:	area-CodeGen-coreclr, untriaged
Milestone:	-

[jakobbotsch]

The previous behavior seems wrong to me based on the IL generated, which is the following:

IL_0000: ldarga.s  item
IL_0002: ldarga.s  item
IL_0004: constrained. !!T
IL_000A: callvirt  instance int32 vb_playground.IMoveable::get_Position()
IL_000F: ldarga.s  item
IL_0011: call      int32 vb_playground.Program::GetOffset<!!T>(!!0&)
IL_0016: add.ovf
IL_0017: constrained. !!T
IL_001D: callvirt  instance void vb_playground.IMoveable::set_Position(int32)
IL_0022: ret

Before preview 6 the call to set_Position is seeing the wrong instance of item. It is an object reference that is modified by the call to GetOffset, and the constrained call to set_Position dereferences the address loaded at IL_0000 which should see the modified instance. The previous behavior is due to a JIT bug where the JIT improperly reorders the indirection of the this byref for constrained calls with the rest of the arguments (in this case, loading item too early). The new behavior is a side effect of #66887; this PR caused the JIT to see slightly different runtime lookups which meant we now end up with the correct behavior. I believe the JIT bug still exists, so I will open an issue for that (edit: #73615).

FWIW, the same example translated to C# generates the following IL:

IL_0000: ldarga.s item
IL_0002: stloc.0
IL_0003: ldloc.0
IL_0004: ldloc.0
IL_0005: constrained. !!T
IL_000b: callvirt instance int32 IMoveable::get_Position()
IL_0010: ldarga.s item
IL_0012: call int32 Program::GetOffset<!!T>(!!0&)
IL_0017: add
IL_0018: constrained. !!T
IL_001e: callvirt instance void IMoveable::set_Position(int32)
IL_0023: ret

which looks more correct to me. (edit: actually, this still has the bug)

I guess two wrongs really do make a right ...

[jakobbotsch]

Though I'm not really sure what Roslyn can do here, I suppose making a copy would be wrong in the struct case. On the other hand I'm not sure what the JIT can do here either, there can be arbitrary trees between loading the address of the this variable and the constrained call which will cause the JIT to spill the arguments for any number of reasons.

[davidwrighton]

Updated the link the bug description such that it works. I'm taking a look at this now, trying to decipher what the intentions are here.

[davidwrighton]

Ok, so looking at this, I agree with @jakobbotsch, this appears to be incorrect codegen by Roslyn. Correct code generation for this would be something like...

ldarga.s  item
ldloca tempVar
initobj !!T
ldloc tempVar
box !!T
ldnull
ceq
brfalse hasAddressForPairedGetSetCalls
ldobj !!T
stloc tempForPairedGet
ldloca tempForPairedGet
hasAddressForPairedGetSetCalls: dup
IL_0004: constrained. !!T
IL_000A: callvirt  instance int32 vb_playground.IMoveable::get_Position()
IL_000F: ldarga.s  item
IL_0011: call      int32 vb_playground.Program::GetOffset<!!T>(!!0&)
IL_0016: add.ovf
IL_0017: constrained. !!T
IL_001D: callvirt  instance void vb_playground.IMoveable::set_Position(int32)
IL_0022: ret

Note: this design relies on the detail that Nullable implements no interfaces, and there are no constrained calls on Nullable which would need this sort of paired call.

[jakobbotsch]

C# example:

using System;
using System.Runtime.CompilerServices;

public interface IMoveable
{
    int Position { get; set; }
}

public class Item : IMoveable
{
    public string Name { get; set; }

    public int Position
    {
        get
        {
            Console.WriteLine("Position get for item '{0}'", Name);
            return 0;
        }
        set
        {
            Console.WriteLine("Position get for item '{0}'", Name);
        }
    }
}

public class Program
{
    public static void Main()
    {
        Item item = new Item { Name = "Goo" };
        Shift(item);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static void Shift<T>(T item) where T : class, IMoveable
    {
        item.Position += GetOffset(ref item);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static int GetOffset<T>(ref T item)
    {
        item = (T)(IMoveable)(new Item { Name = "Bar" });
        return 0;
    }
}

[jakobbotsch]

F# may have a similar bug, though it does not have compound operators, so I'm not quite sure what the output is supposed to be.
@TIHan helped me translate to the F# source

open System
type IMoveable =
    abstract Position : int with get, set
   
type Item() =
    member val Name = "Bar" with get, set

    interface IMoveable with
        member this.Position
            with get() = Console.WriteLine $"Position get for item '{this.Name}'"; 0
            and set (value : int) = Console.WriteLine $"Position set for item '{this.Name}'"

module Program =
    let GetOffset<'T> (item : 'T byref) : int =
        item <- ((Item(Name = "Bar") :> IMoveable) :> obj) :?> 'T
        0

    let Shift<'T when 'T :> IMoveable and 'T : not struct> (item : 'T) =
        let mutable item = item
        item.Position <- item.Position + (GetOffset &item)

    [<EntryPoint>]
    let main (argv : string[]) =
        let item = Item(Name = "Goo")
        Shift(item)
        0

The generated IL for Shift is

            IL_0000: ldarg.0
            IL_0001: stloc.0
            IL_0002: ldloca.s 0
            IL_0004: ldloca.s 0
            IL_0006: constrained. !!T
            IL_000c: callvirt instance int32 _/IMoveable::get_Position()
            IL_0011: ldloca.s 0
            IL_0013: call int32 _/Program::GetOffset<!!T>(!!0&)
            IL_0018: add
            IL_0019: constrained. !!T
            IL_001f: callvirt instance void _/IMoveable::set_Position(int32)
            IL_0024: ret

which also prints "Goo" followed by "Bar" (when the JIT bug does not trigger).
@dsyme What is the expected behavior of the F# source code here?

[jaredpar]

@jakobbotsch

The generated IL for Shift is

What is the type of loc 0 for the F# case? Changes the meaning of the IL if it's a ref / non-ref

[jakobbotsch]

What is the type of loc 0 for the F# case?

Full decompiled IL:

        .method public static 
            void Shift<class (_/IMoveable) T> (
                !!T item
            ) cil managed 
        {
            // Method begins at RVA 0x2108
            // Code size 37 (0x25)
            .maxstack 5
            .locals init (
                [0] !!T
            )

            IL_0000: ldarg.0
            IL_0001: stloc.0
            IL_0002: ldloca.s 0
            IL_0004: ldloca.s 0
            IL_0006: constrained. !!T
            IL_000c: callvirt instance int32 _/IMoveable::get_Position()
            IL_0011: ldloca.s 0
            IL_0013: call int32 _/Program::GetOffset<!!T>(!!0&)
            IL_0018: add
            IL_0019: constrained. !!T
            IL_001f: callvirt instance void _/IMoveable::set_Position(int32)
            IL_0024: ret
        } // end of method Program::Shift

Sharplab link

Interestingly C++/CLI always boxes item and does not do constrained calls, so it does not allow mutating structs through the interface calls (but it is correct for the reference type case).

[dsyme]

Tricky case - the codegen we use does indeed expose the potential that the target of the property setter call is mutated.

This sort of code is so vanishingly rare in F# (it's likely the above is the first time it's been written) that we would simply add a note to the spec that for generic calls the assumption is made that this doesn't matter.

[TIHan]

I'm trying to wrap my head around this. The ECMA spec doesn't specifically say when the this ptr is to be deref'ed in regards to constrained.. It mentions boxing arguments as the alternative equivalent if the thisType is a reference type. So something like this:

.locals init ([0] !!T)
...
IL: ldloc.s 0
IL: box !!T
(mutate loc 0)
IL: callvirt instance void _/IMoveable::set_Position(int32)
... 

would be semantically equivalent to this:

.locals init ([0] !!T)
...
IL: ldloca.s 0
(mutate loc 0)
IL: constrained. !!T
IL: callvirt instance void _/IMoveable::set_Position(int32)
... 

Based on the new runtime behavior, these would not be semantically equivalent anymore.

[jakobbotsch]

The ECMA spec doesn't specifically say when the this ptr is to be deref'ed in regards to constrained..

It does mention that the constrained. prefix modifies how the callvirt instruction is executed, so this part seems clear to me.

Based on the new runtime behavior, these would not be semantically equivalent anymore.

I'm not sure I understand. These are not semantically equivalent today -- the latter should call set_Position on the modified instance, the former on the unmodified instance.

The rationale in the spec seems to be more about how to represent virtual calls on value types and reference types in a uniform way, for which there are the two ways (boxing + callvirt or constrained callvirt). But it does not say that these are necessarily semantically equivalent in any of the cases (most obviously for value types, of course).

[TIHan]

But it does not say that these are necessarily semantically equivalent in any of the cases (most obviously for value types, of course).

It only mentions the alternative was to box thisType but the reason to not do it was because value types. This is what makes me think they would be equivalent for reference types.

[dsyme]

Just to note that in ~2002 when we decided to use address-of-generic-thing for these constrained calls - to get uniform representation of calls to value types and reference types that respected mutation on value types - I believe we simply had no thought that the contents of the address might be mutated in the process of preparing the arguments for the call - I can't find any record of discussing this. I'm not actually sure what we would have done - likely we would have just pushed it under the carpet and accepted that the semantics of generic code differ to the semantics of non-generic version of the code instantiated at reference types.

It's really strange however that this has been doing the "wrong right wrong" thing for so long.

I wouldn't recommend changing C# codegen to "fix" this. Better to put a note in the C# spec.

[jakobbotsch]

It's really strange however that this has been doing the "wrong right wrong" thing for so long.

Part of the problem here is that there is a JIT bug (#73615) that makes the wrong IL give the right result from the language semantics perspective of the three languages. It is quite unpredictable when the JIT bug triggers, but the test in this case has been relying on it for many years. Once we fix that in .NET 8 we will end up with the "incorrect" behavior consistently which is going to be a potential break without fixing the F#/VB/C# codegen.

[dsyme]

Yes, I understand.

We won't change the F# codegen, and it's fine if the execution changes in such a case - no one writes code like this in F#.

I can't make the call for C# or VB.

[jakobbotsch]

Will close this, the underlying issues here are tracked by #73615 and #73681, the latter of which has an open PR that will fix the behavior change.