Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Assertion failed '!m_rootNode->OperIsLocal() && !m_rootNode->OperIsLocalAddr()' during 'Morph - Structs/AddrExp' #81018

Closed
jakobbotsch opened this issue Jan 23, 2023 · 1 comment · Fixed by #81031
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@jakobbotsch
Copy link
Member

// Generated by Fuzzlyn v1.5 on 2023-01-22 16:00:16
// Run on Arm64 Windows
// Seed: 17286164302317655577
// Reduced from 117.6 KiB to 0.4 KiB in 00:02:13
// Hits JIT assert in Release:
// Assertion failed '!m_rootNode->OperIsLocal() && !m_rootNode->OperIsLocalAddr()' in 'Program:Main(Fuzzlyn.ExecutionServer.IRuntime)' during 'Morph - Structs/AddrExp' (IL size 83; hash 0xade6b36b; FullOpts)
// 
//     File: D:\a\_work\1\s\src\coreclr\jit\lclmorph.cpp Line: 34
// 
public interface I0
{
}

public struct S0 : I0
{
    public sbyte F0;
    public S0 M17(I0 arg0, ulong arg1)
    {
        return this;
    }
}

public class Program
{
    public static ulong s_2;
    public static void Main()
    {
        var vr6 = new S0();
        var vr7 = new S0();
        new S0().M17(new S0().M17(vr7, 0).M17(vr6, s_2), s_2);
    }
}

We end up with:

LocalAddressVisitor visiting statement:
STMT00012 ( ??? ... ??? )
               [000077] n--X-------IND       byte  
               [000076] -----------                         └──▌  LCL_VAR_ADDR byref  V03 loc3         
                                                            └──▌    byte   V03.S0:F0 (offs=0x00) -> V12 tmp8         
Replacing the field in promoted struct with local var V12

This gives a standalone top-level LCL_VAR node and the locals linking assumes that these cannot exist.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 23, 2023
@jakobbotsch jakobbotsch self-assigned this Jan 23, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 23, 2023
@ghost
Copy link

ghost commented Jan 23, 2023

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

Issue Details
// Generated by Fuzzlyn v1.5 on 2023-01-22 16:00:16
// Run on Arm64 Windows
// Seed: 17286164302317655577
// Reduced from 117.6 KiB to 0.4 KiB in 00:02:13
// Hits JIT assert in Release:
// Assertion failed '!m_rootNode->OperIsLocal() && !m_rootNode->OperIsLocalAddr()' in 'Program:Main(Fuzzlyn.ExecutionServer.IRuntime)' during 'Morph - Structs/AddrExp' (IL size 83; hash 0xade6b36b; FullOpts)
// 
//     File: D:\a\_work\1\s\src\coreclr\jit\lclmorph.cpp Line: 34
// 
public interface I0
{
}

public struct S0 : I0
{
    public sbyte F0;
    public S0 M17(I0 arg0, ulong arg1)
    {
        return this;
    }
}

public class Program
{
    public static ulong s_2;
    public static void Main()
    {
        var vr6 = new S0();
        var vr7 = new S0();
        new S0().M17(new S0().M17(vr7, 0).M17(vr6, s_2), s_2);
    }
}

We end up with:

LocalAddressVisitor visiting statement:
STMT00012 ( ??? ... ??? )
               [000077] n--X-------IND       byte  
               [000076] -----------                         └──▌  LCL_VAR_ADDR byref  V03 loc3         
                                                            └──▌    byte   V03.S0:F0 (offs=0x00) -> V12 tmp8         
Replacing the field in promoted struct with local var V12

This gives a standalone top-level LCL_VAR node and the locals linking assumes that these cannot exist.

Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch jakobbotsch added this to the 8.0.0 milestone Jan 23, 2023
@jakobbotsch jakobbotsch removed the untriaged New issue has not been triaged by the area owner label Jan 23, 2023
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Jan 23, 2023
Before this change statements have only one field for the "tree list"
even though bidirectional iteration is supported after nodes have been
linked. This worked fine before the locals tree list existed since the
"root node" was always the last node.

This does not work for the locals tree list since the "root node" is not
part of the list, meaning that we need somewhere else to store the
'last' node. Before this PR the assumption was that the root node was
_never_ part of the locals linked list, so we could use the
gtNext/gtPrev fields of the root node. However, the added test case
shows that in fact it is possible we end up with a top level local.

This PR fixes the problem by adding a Statement::m_treeListEnd field
that can keep the last node of the locals linked list.  While this takes
some memory, it seems like the most maintainable way to resolve the
problem. I experimented with making the linked list circular or by
allocating a new stand-in node when necessary but eventually I ended up
here.

Fix dotnet#81018
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 23, 2023
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Jan 23, 2023
Statements have only one field for the "tree list" even though
bidirectional iteration is supported after nodes have been linked. This
worked fine before the locals tree list existed since the "root node"
was always the last node.

This does not work for the locals tree list since the "root node" is not
part of the list, meaning that we need somewhere else to store the
'last' node. Before this PR the assumption was that the root node was
_never_ part of the locals linked list, so we could use the
gtNext/gtPrev fields of the root node. However, the added test case
shows that in fact it is possible we end up with a top level local.

This PR fixes the problem by allocating a stand-in node in this scenario
and keeping it in Statement::m_treeList.

Fix dotnet#81018
jakobbotsch added a commit that referenced this issue Jan 24, 2023
Before this change statements have only one field for the "tree list"
even though bidirectional iteration is supported after nodes have been
linked. This worked fine before the locals tree list existed since the
"root node" was always the last node.

This does not work for the locals tree list since the "root node" is not
part of the list, meaning that we need somewhere else to store the
'last' node. Before this PR the assumption was that the root node was
_never_ part of the locals linked list, so we could use the
gtNext/gtPrev fields of the root node. However, the added test case
shows that in fact it is possible we end up with a top level local.

This PR fixes the problem by adding a Statement::m_treeListEnd field
that can keep the last node of the locals linked list.  While this takes
some memory, it seems like the most maintainable way to resolve the
problem. I experimented with making the linked list circular or by
allocating a new stand-in node when necessary but eventually I ended up
here.

Fix #81018
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant