Skip to content

Commit

Permalink
Avoid deep recursion while emitting deeply nested logical conditions
Browse files Browse the repository at this point in the history 
Fixes dotnet#66900.
  • Loading branch information
@AlekseyTs
AlekseyTs committed Feb 22, 2023
1 parent 006abc2 commit 3c35f65
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 31 deletions.
88 changes: 65 additions & 23 deletions src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Diagnostics;
using System.Linq;
using System.Reflection.Metadata;
using System.Runtime.CompilerServices;
using Microsoft.CodeAnalysis.CodeGen;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
Expand Down Expand Up @@ -415,43 +416,84 @@ private void EmitCondBranchCore(BoundExpression condition, ref object dest, bool
switch (condition.Kind)
{
case BoundKind.BinaryOperator:

var binOp = (BoundBinaryOperator)condition;
bool testBothArgs = sense;
Debug.Assert(binOp.ConstantValueOpt is null);

switch (binOp.OperatorKind.OperatorWithLogical())
#nullable enable
if (binOp.OperatorKind.OperatorWithLogical() is BinaryOperatorKind.LogicalOr or BinaryOperatorKind.LogicalAnd)
{
case BinaryOperatorKind.LogicalOr:
testBothArgs = !testBothArgs;
// Fall through
goto case BinaryOperatorKind.LogicalAnd;

case BinaryOperatorKind.LogicalAnd:
if (testBothArgs)
{
// gotoif(a != sense) fallThrough
// gotoif(b == sense) dest
// fallThrough:

object fallThrough = null;
var stack = ArrayBuilder<(BoundExpression? condition, StrongBox<object?> destBox, bool sense)>.GetInstance();
var destBox = new StrongBox<object?>(dest);
stack.Push((binOp, destBox, sense));

EmitCondBranch(binOp.Left, ref fallThrough, !sense);
EmitCondBranch(binOp.Right, ref dest, sense);
do
{
(BoundExpression? condition, StrongBox<object?> destBox, bool sense) top = stack.Pop();

if (top.condition is null)
{
// This is a special entry to indicate that it is time to append the block
object? fallThrough = top.destBox.Value;
if (fallThrough != null)
{
_builder.MarkLabel(fallThrough);
}
}
else
else if (top.condition.ConstantValueOpt is null &&
top.condition is BoundBinaryOperator binary &&
binary.OperatorKind.OperatorWithLogical() is BinaryOperatorKind.LogicalOr or BinaryOperatorKind.LogicalAnd)
{
// gotoif(a == sense) labDest
// gotoif(b == sense) labDest
if (binary.OperatorKind.OperatorWithLogical() is BinaryOperatorKind.LogicalOr ? !top.sense : top.sense)
{
// gotoif(a != sense) fallThrough
// gotoif(b == sense) dest
// fallThrough:

var fallThrough = new StrongBox<object?>();

// Note, operations are pushed to the stack in opposite order
stack.Push((null, fallThrough, true)); // This is a special entry to indicate that it is time to append the fallThrough block
stack.Push((binary.Right, top.destBox, top.sense));
stack.Push((binary.Left, fallThrough, !top.sense));
}
else
{
// gotoif(a == sense) labDest
// gotoif(b == sense) labDest

EmitCondBranch(binOp.Left, ref dest, sense);
condition = binOp.Right;
// Note, operations are pushed to the stack in opposite order
stack.Push((binary.Right, top.destBox, top.sense));
stack.Push((binary.Left, top.destBox, top.sense));
}
}
else if (stack.Count == 0 && ReferenceEquals(destBox.Value, top.destBox.Value))
{
// Instead of recursion we can restart from the top with new condition
condition = top.condition;
sense = top.sense;
dest = destBox.Value;
stack.Free();
goto oneMoreTime;
}
return;
else
{
EmitCondBranch(top.condition, ref top.destBox.Value, top.sense);
}
}
while (stack.Count != 0);

dest = destBox.Value;
stack.Free();
return;
}
#nullable disable

switch (binOp.OperatorKind.OperatorWithLogical())
{
case BinaryOperatorKind.LogicalOr:
case BinaryOperatorKind.LogicalAnd:
throw ExceptionUtilities.Unreachable();

case BinaryOperatorKind.Equal:
case BinaryOperatorKind.NotEqual:
Expand Down
38 changes: 30 additions & 8 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenOperators.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5286,9 +5286,9 @@ public static bool Calculate(bool[] a, bool[] f)
}

diagnostics.Verify(
// (10,16): error CS8078: An expression is too long or complex to compile
// return a[0] && f[0] || a[1] && f[1] || a[2] && f[2] || ...
Diagnostic(ErrorCode.ERR_InsufficientStack, "a").WithLocation(10, 16)
// (10,16): error CS8078: An expression is too long or complex to compile
// return a[0] & f[0] | a[1] & f[1] | a[2] & f[2] | ...
Diagnostic(ErrorCode.ERR_InsufficientStack, "a").WithLocation(10, 16)
);
}

Expand All @@ -5301,10 +5301,10 @@ private static string BuildSequenceOfBinaryExpressions_03(int count = 8192)
builder.Append("a[");
builder.Append(i);
builder.Append("]");
builder.Append(" && ");
builder.Append(" & ");
builder.Append("f[");
builder.Append(i);
builder.Append("] || ");
builder.Append("] | ");
}

builder.Append("a[");
Expand Down Expand Up @@ -5415,7 +5415,7 @@ static void Main()
public static bool Calculate(S1[] a, S1[] f)
{
" + $" return {BuildSequenceOfBinaryExpressions_03()};" + @"
" + $" return {BuildSequenceOfBinaryExpressions_06()};" + @"
}
}
Expand Down Expand Up @@ -5456,6 +5456,28 @@ public static implicit operator bool (S1 x)
);
}

private static string BuildSequenceOfBinaryExpressions_06(int count = 8192)
{
var builder = new System.Text.StringBuilder();
int i;
for (i = 0; i < count; i++)
{
builder.Append("a[");
builder.Append(i);
builder.Append("]");
builder.Append(" && ");
builder.Append("f[");
builder.Append(i);
builder.Append("] || ");
}

builder.Append("a[");
builder.Append(i);
builder.Append("]");

return builder.ToString();
}

[ConditionalFact(typeof(ClrOnly), typeof(NoIOperationValidation), Reason = "https://github.com/dotnet/roslyn/issues/29428")]
[WorkItem(63689, "https://github.com/dotnet/roslyn/issues/63689")]
public void EmitSequenceOfBinaryExpressions_07()
Expand All @@ -5478,7 +5500,7 @@ static void Main()
public static bool Calculate(S1[] a, S1[] f)
{
" + $" return {BuildSequenceOfBinaryExpressions_03(count)};" + @"
" + $" return {BuildSequenceOfBinaryExpressions_06(count)};" + @"
}
}
Expand Down Expand Up @@ -5544,7 +5566,7 @@ static void Main()
public static bool Calculate(bool[] a, bool[] f)
{
" + $" return {BuildSequenceOfBinaryExpressions_03(2048)};" + @"
" + $" return {BuildSequenceOfBinaryExpressions_06(2048)};" + @"
}
}
";
Expand Down
44 changes: 44 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30419,5 +30419,49 @@ class Attr : System.Attribute {}
Assert.DoesNotContain("System.Int32 y", model.LookupSymbols(attrApplication.ArgumentList!.OpenParenToken.SpanStart + 1).Select(s => s.ToTestDisplayString()));
Assert.DoesNotContain("System.Int32 y", model.LookupSymbols(mDefinition.SpanStart).Select(s => s.ToTestDisplayString()));
}

[Fact]
[WorkItem(66900, "https://github.com/dotnet/roslyn/issues/66900")]
public void Issue66900()
{
// public record ClassWithManyConstructorParameters(int P0, int P1, int P2, ...)
// {
// public static ClassWithManyConstructorParameters Create()
// {
// return new ClassWithManyConstructorParameters(P0: 0, P1: 1, P2: 2, ...);
// }
// }

var src = @"
public record ClassWithManyConstructorParameters(int P0";

const int count = 2000;

for (int i = 1; i < count; i++)
{
src += ", int P" + i;
}

src += @")
{
public static ClassWithManyConstructorParameters Create()
{
return new ClassWithManyConstructorParameters(P0: 0";

for (int i = 1; i < count; i++)
{
src += ", P" + i + ": " + i;
}

src += @");
}
}
";
var comp = CreateCompilation(new[] { src, IsExternalInitTypeDefinition }, targetFramework: TargetFramework.NetCoreApp);
CompileAndVerify(comp, verify: Verification.Skipped).VerifyDiagnostics();

comp = CreateCompilation(new[] { src, IsExternalInitTypeDefinition }, targetFramework: TargetFramework.DesktopLatestExtended);
CompileAndVerify(comp, verify: Verification.Skipped).VerifyDiagnostics();
}
}
}

0 comments on commit 3c35f65

Please sign in to comment.