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

Compound Assignment when ref temps are required crashes the compiler #36443

Closed
333fred opened this issue Jun 14, 2019 · 3 comments · Fixed by #38196
Closed

Compound Assignment when ref temps are required crashes the compiler #36443

333fred opened this issue Jun 14, 2019 · 3 comments · Fixed by #38196

Comments

@333fred
Copy link
Member

333fred commented Jun 14, 2019

using System;
using System.Threading.Tasks;
struct S
{
    int? i;

    static async Task Main()
    {    

        S s = default;
        Console.WriteLine(s.i += await GetInt());
    }

    static Task<int?> GetInt() => Task.FromResult((int?)1);
}

This code crashes the compiler with the following stack:

Message: 
    System.InvalidOperationException : Unexpected value 'Local' of type 'Microsoft.CodeAnalysis.CSharp.BoundKind'
    
  Stack Trace: 
    at ThrowingTraceListener.Fail(String message, String detailMessage) in ThrowingTraceListener.cs line: 24
    at TraceListener.Fail(String message)
    at TraceInternal.Fail(String message)
    at Debug.Assert(Boolean condition, String message)
    at ExceptionUtilities.UnexpectedValue(Object o) in ExceptionUtilities.cs line: 18
    at MethodToStateMachineRewriter.HoistExpression(BoundExpression expr, AwaitExpressionSyntax awaitSyntaxOpt, Int32 syntaxOffset, RefKind refKind, ArrayBuilder`1 sideEffects, ArrayBuilder`1 hoistedFields, Boolean& needsSacrificialEvaluation) in MethodToStateMachineRewriter.cs line: 606
    at MethodToStateMachineRewriter.HoistExpression(BoundExpression expr, AwaitExpressionSyntax awaitSyntaxOpt, Int32 syntaxOffset, RefKind refKind, ArrayBuilder`1 sideEffects, ArrayBuilder`1 hoistedFields, Boolean& needsSacrificialEvaluation) in MethodToStateMachineRewriter.cs line: 549
    at MethodToStateMachineRewriter.HoistRefInitialization(SynthesizedLocal local, BoundAssignmentOperator node) in MethodToStateMachineRewriter.cs line: 484
    at MethodToStateMachineRewriter.VisitAssignmentOperator(BoundAssignmentOperator node) in MethodToStateMachineRewriter.cs line: 779
    at BoundAssignmentOperator.Accept(BoundTreeVisitor visitor) in BoundNodes.xml.Generated.cs line: 1507
    at BoundTreeVisitor.Visit(BoundNode node) in BoundTreeVisitors.cs line: 145
    at BoundTreeRewriterWithStackGuard.VisitExpressionWithoutStackGuard(BoundExpression node) in BoundTreeRewriter.cs line: 97
    at BoundTreeVisitor.VisitExpressionWithStackGuard(BoundExpression node) in BoundTreeVisitors.cs line: 223
    at BoundTreeVisitor.VisitExpressionWithStackGuard(Int32& recursionDepth, BoundExpression node) in BoundTreeVisitors.cs line: 204
    at BoundTreeRewriterWithStackGuard.Visit(BoundNode node) in BoundTreeRewriter.cs line: 84
    at MethodToStateMachineRewriter.Visit(BoundNode node) in MethodToStateMachineRewriter.cs line: 658
    at AsyncMethodToStateMachineRewriter.VisitExpressionStatement(BoundExpressionStatement node) in AsyncMethodToStateMachineRewriter.cs line: 272
    at BoundExpressionStatement.Accept(BoundTreeVisitor visitor) in BoundNodes.xml.Generated.cs line: 3023
    at BoundTreeVisitor.Visit(BoundNode node) in BoundTreeVisitors.cs line: 145
    at BoundTreeRewriterWithStackGuard.Visit(BoundNode node) in BoundTreeRewriter.cs line: 87
    at MethodToStateMachineRewriter.Visit(BoundNode node) in MethodToStateMachineRewriter.cs line: 658
    at BoundTreeRewriter.DoVisitList[T](ImmutableArray`1 list) in BoundTreeRewriter.cs line: 37
    at BoundTreeRewriter.VisitList[T](ImmutableArray`1 list) in BoundTreeRewriter.cs line: 26
    at MethodToClassRewriter.VisitBlock(BoundBlock node) in MethodToClassRewriter.cs line: 139
    at MethodToStateMachineRewriter.<>n__0(BoundBlock node)
    at <>c__DisplayClass42_0.<VisitBlock>b__0() in MethodToStateMachineRewriter.cs line: 665
    at MethodToStateMachineRewriter.PossibleIteratorScope(ImmutableArray`1 locals, Func`1 wrapped) in MethodToStateMachineRewriter.cs line: 301
    at MethodToStateMachineRewriter.VisitBlock(BoundBlock node) in MethodToStateMachineRewriter.cs line: 665
    at BoundBlock.Accept(BoundTreeVisitor visitor) in BoundNodes.xml.Generated.cs line: 2673
    at BoundTreeVisitor.Visit(BoundNode node) in BoundTreeVisitors.cs line: 145
    at BoundTreeRewriterWithStackGuard.Visit(BoundNode node) in BoundTreeRewriter.cs line: 87
    at MethodToStateMachineRewriter.Visit(BoundNode node) in MethodToStateMachineRewriter.cs line: 658
    at BoundTreeRewriter.VisitSequencePoint(BoundSequencePoint node) in BoundNodes.xml.Generated.cs line: 9546
    at BoundSequencePoint.Accept(BoundTreeVisitor visitor) in BoundNodes.xml.Generated.cs line: 2614
    at BoundTreeVisitor.Visit(BoundNode node) in BoundTreeVisitors.cs line: 145
    at BoundTreeRewriterWithStackGuard.Visit(BoundNode node) in BoundTreeRewriter.cs line: 87
    at MethodToStateMachineRewriter.Visit(BoundNode node) in MethodToStateMachineRewriter.cs line: 658
    at BoundTreeRewriter.DoVisitList[T](ImmutableArray`1 list) in BoundTreeRewriter.cs line: 37
    at BoundTreeRewriter.VisitList[T](ImmutableArray`1 list) in BoundTreeRewriter.cs line: 26
    at MethodToClassRewriter.VisitBlock(BoundBlock node) in MethodToClassRewriter.cs line: 139
    at MethodToStateMachineRewriter.<>n__0(BoundBlock node)
    at <>c__DisplayClass42_0.<VisitBlock>b__0() in MethodToStateMachineRewriter.cs line: 665
    at MethodToStateMachineRewriter.PossibleIteratorScope(ImmutableArray`1 locals, Func`1 wrapped) in MethodToStateMachineRewriter.cs line: 301
    at MethodToStateMachineRewriter.VisitBlock(BoundBlock node) in MethodToStateMachineRewriter.cs line: 665
    at BoundBlock.Accept(BoundTreeVisitor visitor) in BoundNodes.xml.Generated.cs line: 2673
    at BoundTreeVisitor.Visit(BoundNode node) in BoundTreeVisitors.cs line: 145
    at BoundTreeRewriterWithStackGuard.Visit(BoundNode node) in BoundTreeRewriter.cs line: 87
    at MethodToStateMachineRewriter.Visit(BoundNode node) in MethodToStateMachineRewriter.cs line: 658
    at AsyncMethodToStateMachineRewriter.VisitBody(BoundStatement body) in AsyncMethodToStateMachineRewriter.cs line: 255
    at AsyncMethodToStateMachineRewriter.GenerateMoveNext(BoundStatement body, MethodSymbol moveNextMethod) in AsyncMethodToStateMachineRewriter.cs line: 121
    at AsyncRewriter.GenerateMoveNext(SynthesizedImplementationMethod moveNextMethod) in AsyncRewriter.cs line: 269
    at AsyncRewriter.GenerateMethodImplementations() in AsyncRewriter.cs line: 140
    at StateMachineRewriter.Rewrite() in StateMachineRewriter.cs line: 114
    at AsyncRewriter.Rewrite(BoundStatement bodyWithAwaitLifted, MethodSymbol method, Int32 methodOrdinal, VariableSlotAllocator slotAllocatorOpt, TypeCompilationState compilationState, DiagnosticBag diagnostics, AsyncStateMachine& stateMachineType) in AsyncRewriter.cs line: 81
    at MethodCompiler.LowerBodyOrInitializer(MethodSymbol method, Int32 methodOrdinal, BoundStatement body, SynthesizedSubmissionFields previousSubmissionFields, TypeCompilationState compilationState, Boolean instrumentForDynamicAnalysis, DebugDocumentProvider debugDocumentProvider, ImmutableArray`1& dynamicAnalysisSpans, DiagnosticBag diagnostics, VariableSlotAllocator& lazyVariableSlotAllocator, ArrayBuilder`1 lambdaDebugInfoBuilder, ArrayBuilder`1 closureDebugInfoBuilder, StateMachineTypeSymbol& stateMachineTypeOpt) in MethodCompiler.cs line: 1346
    at MethodCompiler.CompileMethod(MethodSymbol methodSymbol, Int32 methodOrdinal, ProcessedFieldInitializers& processedInitializers, SynthesizedSubmissionFields previousSubmissionFields, TypeCompilationState compilationState) in MethodCompiler.cs line: 1099
    at MethodCompiler.CompileNamedType(NamedTypeSymbol containingType) in MethodCompiler.cs line: 507
    at <>c__DisplayClass22_0.<CompileNamedTypeAsTask>b__0() in MethodCompiler.cs line: 397
    at <>c__DisplayClass5_0.<WithCurrentUICulture>b__0() in UICultureUtilities.cs line: 136
    at Task.InnerInvoke()
    at Task.Execute()
    at --- End of stack trace from previous location where exception was thrown ---

The issue here is that TransformCompoundAssignmentLHS can return a ref local for the following case:

// Case 5: otherwise, it must be structVariable.field += value or array[index] += value. Either way
// we have a variable on the left. Transform it into:
// ref temp = ref variable
// temp = temp + value

The fix is likely to simply issue an understandable error here, instead of throwing an UnreachableCode exception. FYI @jcouv @agocke.

@jcouv
Copy link
Member

jcouv commented Jun 14, 2019

Compiling this sort of scenario seems to work fine from the command-line, so this may just be a problem with the assertion. This is consistent with the observation that nobody reported this issue.
That's strange because the exception above is from ExceptionUtilities.UnexpectedValue (not an assertion).

With both the native compiler and Roslyn, the write to the field involves an additional dereference after the await completes (ie. we couldn't save a ref to the field).

// produced by native compiler, 4.7.3190.0
private void MoveNext()
{
	try
	{
		bool flag = true;
		object item;
		int num;
		TaskAwaiter<int> awaiter;
		if (<>1__state != 0)
		{
			item = <>4__this;
			num = ((C)item).field;
			awaiter = <>4__this.M2().GetAwaiter();
			if (!awaiter.IsCompleted)
			{
				Tuple<C, int> tuple = (Tuple<C, int>)(<>t__stack = new Tuple<C, int>((C)item, num));
				<>1__state = 0;
				<>u__$awaiter1 = awaiter;
				<>t__builder.AwaitUnsafeOnCompleted(ref awaiter, ref this);
				flag = false;
				return;
			}
		}
		else
		{
			Tuple<C, int> tuple = (Tuple<C, int>)<>t__stack;
			item = tuple.Item1;
			num = tuple.Item2;
			<>t__stack = null;
			awaiter = <>u__$awaiter1;
			<>u__$awaiter1 = default(TaskAwaiter<int>);
			<>1__state = -1;
		}
		int result = awaiter.GetResult();
		awaiter = default(TaskAwaiter<int>);
		((C)item).field = num + result; // assign to the field
		Console.Write(<>4__this.field);
	}
	catch (Exception exception)
	{
		<>1__state = -2;
		<>t__builder.SetException(exception);
		return;
	}
	<>1__state = -2;
	<>t__builder.SetResult();
}
// produced by 3.2 compiler
private void MoveNext()
{
	int num = <>1__state;
	try
	{
		TaskAwaiter<int> awaiter;
		if (num != 0)
		{
			<>s__1 = <>4__this.field;
			awaiter = <>4__this.M2().GetAwaiter();
			if (!awaiter.IsCompleted)
			{
				num = (<>1__state = 0);
				<>u__1 = awaiter;
				<M>d__1 stateMachine = this;
				<>t__builder.AwaitUnsafeOnCompleted(ref awaiter, ref stateMachine);
				return;
			}
		}
		else
		{
			awaiter = <>u__1;
			<>u__1 = default(TaskAwaiter<int>);
			num = (<>1__state = -1);
		}
		<>s__2 = awaiter.GetResult();
		<>4__this.field = <>s__1 + <>s__2; // assign to the field
		Console.Write(<>4__this.field);
	}
	catch (Exception exception)
	{
		<>1__state = -2;
		<>t__builder.SetException(exception);
		return;
	}
	<>1__state = -2;
	<>t__builder.SetResult();
}
// original source
class C
{
    int field = 1;
    async System.Threading.Tasks.Task M()
    {
         this.field += await M2();
         System.Console.Write(this.field);
    }

    async System.Threading.Tasks.Task<int> M2()
    {
         await System.Threading.Tasks.Task.Yield();
         return 42;
    }
}

@jcouv
Copy link
Member

jcouv commented Jun 17, 2019

The code sample in OP doesn't crash the latest compiler (tested in command-line, unittest and sharplab). Let's confirm and narrow down the repro scenario. Correction: it does crash in unittest with CompileAndVerify.

Update: repros with /target:library /debug- /optimize+ test.cs (to compile with "release" option)

IteratorAndAsyncCaptureWalker.Analyze is hoisting all locals and parameters in debug mode, which explains the difference between passing/crashing.

            if (compilation.Options.OptimizationLevel != OptimizationLevel.Release)
            {
                Debug.Assert(variablesToHoist.Count == 0);

                // In debug build we hoist all locals and parameters:
                foreach (var v in allVariables)
                {
                    var symbol = v.Symbol;
                    if ((object)symbol != null && HoistInDebugBuild(symbol))
                    {
                        variablesToHoist.Add(symbol);
                    }
                }
            }

@gafter
Copy link
Member

gafter commented Aug 28, 2019

The analysis fails to determine that the local s needs to be hoisted to the state machine. I am fixing the test in this PR to demonstrate that.

@gafter gafter added 4 - In Review A fix for the issue is submitted for review. and removed Investigation Required labels Aug 28, 2019
gafter pushed a commit that referenced this issue Aug 30, 2019
@gafter gafter removed the 4 - In Review A fix for the issue is submitted for review. label Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants