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

Compiler crash #19033

Closed
igor-muratov opened this issue Apr 27, 2017 · 4 comments
Closed

Compiler crash #19033

igor-muratov opened this issue Apr 27, 2017 · 4 comments

Comments

@igor-muratov
Copy link

igor-muratov commented Apr 27, 2017

Any version below apr 17:

using System;

namespace ConsoleApp3
{
    class BC { }
    class DC1 : BC { }
    class DC2 : BC { }
    class Program
    {
        static void Main(string[] args)
        {
        }

        private string Q(BC bclass, int n = 0)
        {
            switch (bclass)
            {
                case DC1 mc:

                    string A(BC map)
                    {
                        return "";
                    }

                    string B(BC map) 
                    {
                        switch (map)
                        {
                            case DC1 ne:
                                Action<int> a = _ => B(new DC2());
                                return "";
                            case DC2 b: 
                                return C(b);
                            default:
                                return A(map);
                        }
                    }

                    string C(BC map)
                    {
                        switch (map)
                        {
                            case DC1 me:
                                return n.ToString();
                            default:
                                return "";
                        }
                    }
                    break;

                default:
                    break;
            }

            return "";
        }
    }
}

"csc.exe" exited with code -2146232797

@binki
Copy link

binki commented May 17, 2017

The repro can be made more minimal:

using System;

class Program
{
    void Q(int n = 0)
    {
        {
            object mc;

            string B(object map)
            {
                Action<int> a = _ => B(new object());
                return n.ToString();
            }
        }
    }
}

I think this is the same as #19182 as the pared down repro exhibits the same features: lambda accessing inner scope of a block (B is in inner scope of the {}), local method referencing variables outside of the enclosing block (n) and variables inside the block (B via lambda?).

@jcouv
Copy link
Member

jcouv commented Jun 7, 2017

This seems the same stacktrace as #15322
Ping @agocke to confirm and verify.

System.Collections.Generic.KeyNotFoundException: The given key was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Microsoft.CodeAnalysis.CodeGen.LocalSlotManager.GetLocal(ILocalSymbol symbol) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\Core\Portable\CodeGen\LocalSlotManager.cs:line 148
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.GetLocal(LocalSymbol symbol) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitStatement.cs:line 1422
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.GetLocal(BoundLocal localExpression) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitStatement.cs:line 1417
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitLocalLoad(BoundLocal local, Boolean used) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitExpression.cs:line 1129
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitExpressionCore(BoundExpression expression, Boolean used) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitExpression.cs:line 116
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitExpression(BoundExpression expression, Boolean used) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitExpression.cs:line 56
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitDelegateCreation(BoundExpression node, BoundExpression receiver, Boolean isExtensionMethod, MethodSymbol method, TypeSymbol delegateType, Boolean used) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitConversion.cs:line 263
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitDelegateCreationExpression(BoundDelegateCreationExpression expression, Boolean used) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitExpression.cs:line 640
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitExpressionCore(BoundExpression expression, Boolean used) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitExpression.cs:line 100
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitExpression(BoundExpression expression, Boolean used) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitExpression.cs:line 56
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitAssignmentValue(BoundAssignmentOperator assignmentOperator) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitExpression.cs:line 2227
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitAssignmentExpression(BoundAssignmentOperator assignmentOperator, UseKind useKind) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitExpression.cs:line 1871
   at Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator.EmitExpressionCore(BoundExpression expression, Boolean used) in D:\#TeamCity\buildAgent\work\596e688e8f7f38c8\!roslyn\sources\dotnet\src\Compilers\CSharp\Portable\CodeGen\EmitExpression.cs:line 88

@jcouv jcouv added this to the 15.6 milestone Jun 7, 2017
@agocke
Copy link
Member

agocke commented Jun 27, 2017

This looks very similar, but I think it's a different bug. When I looked into it, the problem was that here we were erasing the fact that the lambda captured B and incorrectly placed the frame higher up in the method. This causes us to have a missing frame pointer, but because our proxies system doesn't know what scope the proxies are actually valid in we don't notice until CodeGen, where we ask for a local that doesn't exist.

The reason we were erasing B in analysis is due to an overaggressive optimization for closures calling local functions assuming we don't need an explicit frame pointer, we only need this (which is technically a frame pointer, but is usually ambient).

@agocke
Copy link
Member

agocke commented Jun 27, 2017

I think https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=403646 contains another repro of this bug:

using System;
class C
{
    static void F(Action a)
    {
        object x = null;
        {
            object y = null;
            void G(object z)
            {
                F(() => G(x));
            }
        }
    }
}

agocke added a commit to agocke/roslyn that referenced this issue Aug 8, 2017
Currently, the lambda rewriter has an early optimization pass in
analysis that tries to find all local functions that only capture 'this'
and remove references to local functions that do the same. There are two
problems with this approach:

    1) Generally, removing information from the tree is a bad idea
    because it hurts further analysis passes that may have needed that
    information.

    2) The optimization strategy itself is very tricky and has a number
    of complex corner cases. This has lead to bugs, for example dotnet#19033.

This PR deletes the current method and adds a new optimization routine
at the end of the analysis, operating on assigned scopes and
environments rather than removing captured variable analysis. The new
optimization is as follows: if we end up with an environment containing
only 'this', the environment can be removed, all containing methods can
be moved to the top-level type, and all environments which capture the
'this' environment can instead directly capture the 'this' parameter.
This produces almost the same results as the previous optimization, but
is easier to validate as an algebraic equivalence.

Fixes dotnet#19033, dotnet#20577
@agocke agocke moved this from To Do to In Progress in Compiler: Closure Conversion Rewrite Aug 9, 2017
agocke added a commit that referenced this issue Aug 11, 2017
Currently, the lambda rewriter has an early optimization pass in
analysis that tries to find all local functions that only capture 'this'
and remove references to local functions that do the same. There are two
problems with this approach:

    1) Generally, removing information from the tree is a bad idea
    because it hurts further analysis passes that may have needed that
    information.

    2) The optimization strategy itself is very tricky and has a number
    of complex corner cases. This has lead to bugs, for example #19033.

This PR deletes the current method and adds a new optimization routine
at the end of the analysis, operating on assigned scopes and
environments rather than removing captured variable analysis. The new
optimization is as follows: if we end up with an environment containing
only 'this', the environment can be removed, all containing methods can
be moved to the top-level type, and all environments which capture the
'this' environment can instead directly capture the 'this' parameter.
This produces almost the same results as the previous optimization, but
is easier to validate as an algebraic equivalence.

The baseline changes come from the new optimization being less aggressive 
about moving functions which only capture 'this' to the top level. This
appears to be a wash -- some codegen gets slightly better, some gets
slightly worse.

Fixes #19033
Fixes #20577
@agocke agocke moved this from In Progress to Done in Compiler: Closure Conversion Rewrite Aug 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

5 participants