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

Avoid stack overflow due to deep recursion on long chain of calls. #67913

Merged
merged 6 commits into from
Apr 29, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 53 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -175,17 +175,67 @@ private BoundExpression BindInvocationExpression(
BindArgumentsAndNames(node.ArgumentList, diagnostics, analyzedArguments, allowArglist: false);
result = BindArgListOperator(node, diagnostics, analyzedArguments);
}
else if (receiverIsInvocation(node, out InvocationExpressionSyntax nested))
{
var invocations = ArrayBuilder<InvocationExpressionSyntax>.GetInstance();

invocations.Push(node);
node = nested;
while (receiverIsInvocation(node, out nested))
{
invocations.Push(node);
node = nested;
}

BoundExpression boundExpression = BindMethodGroup(node.Expression, invoked: true, indexed: false, diagnostics: diagnostics);

while (true)
{
result = bindArgumentsAndInvocation(node, boundExpression, analyzedArguments, diagnostics);
nested = node;

if (!invocations.TryPop(out node))
{
break;
}

Debug.Assert(node.Expression.Kind() is SyntaxKind.SimpleMemberAccessExpression);
var memberAccess = (MemberAccessExpressionSyntax)node.Expression;
analyzedArguments.Clear();
VerifyUnchecked(nested, diagnostics, result);
Copy link
Member

@jcouv jcouv Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't follow where this came from. May be worth a comment #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't follow where this came from. May be worth a comment

Added a comment

AlekseyTs marked this conversation as resolved.
Show resolved Hide resolved
boundExpression = BindMemberAccessWithBoundLeft(memberAccess, result, memberAccess.Name, memberAccess.OperatorToken, invoked: true, indexed: false, diagnostics);
}

invocations.Free();
}
else
{
BoundExpression boundExpression = BindMethodGroup(node.Expression, invoked: true, indexed: false, diagnostics: diagnostics);
result = bindArgumentsAndInvocation(node, boundExpression, analyzedArguments, diagnostics);
}

analyzedArguments.Free();
return result;

BoundExpression bindArgumentsAndInvocation(InvocationExpressionSyntax node, BoundExpression boundExpression, AnalyzedArguments analyzedArguments, BindingDiagnosticBag diagnostics)
{
boundExpression = CheckValue(boundExpression, BindValueKind.RValueOrMethodGroup, diagnostics);
string name = boundExpression.Kind == BoundKind.MethodGroup ? GetName(node.Expression) : null;
BindArgumentsAndNames(node.ArgumentList, diagnostics, analyzedArguments, allowArglist: true);
result = BindInvocationExpression(node, node.Expression, name, boundExpression, analyzedArguments, diagnostics);
return BindInvocationExpression(node, node.Expression, name, boundExpression, analyzedArguments, diagnostics);
}

analyzedArguments.Free();
return result;
static bool receiverIsInvocation(InvocationExpressionSyntax node, out InvocationExpressionSyntax nested)
{
if (node.Expression is MemberAccessExpressionSyntax { Expression: InvocationExpressionSyntax receiver, RawKind: (int)SyntaxKind.SimpleMemberAccessExpression } && !receiver.MayBeNameofOperator())
{
nested = receiver;
return true;
}

nested = null;
return false;
}
}

private BoundExpression BindArgListOperator(InvocationExpressionSyntax node, BindingDiagnosticBag diagnostics, AnalyzedArguments analyzedArguments)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,6 @@ protected void FindExpressionVariables(
_variablesBuilder = save;
}

public override void Visit(SyntaxNode node)
{
if (node != null)
{
// no stackguard
((CSharpSyntaxNode)node).Accept(this);
}
}

public override void VisitSwitchExpression(SwitchExpressionSyntax node)
{
Visit(node.GoverningExpression);
Expand Down Expand Up @@ -348,6 +339,50 @@ public override void VisitBinaryExpression(BinaryExpressionSyntax node)
operands.Free();
}

public override void VisitInvocationExpression(InvocationExpressionSyntax node)
{
if (receiverIsInvocation(node, out InvocationExpressionSyntax nested))
{
var invocations = ArrayBuilder<InvocationExpressionSyntax>.GetInstance();

invocations.Push(node);

node = nested;
if (receiverIsInvocation(node, out nested))
Copy link
Member

@jaredpar jaredpar Apr 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be while not if? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be while not if?

Yes, the typo has been fixed already

Copy link
Member

@jcouv jcouv Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we'll push at most two nodes into the invocations queue. Should this be a while loop instead? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a while loop instead?

Yes, this is a typo.

AlekseyTs marked this conversation as resolved.
Show resolved Hide resolved
{
invocations.Push(node);
node = nested;
}

Visit(node.Expression);

do
{
Visit(node.ArgumentList);
}
while (invocations.TryPop(out node));

invocations.Free();
}
else
{
Visit(node.Expression);
Visit(node.ArgumentList);
}

static bool receiverIsInvocation(InvocationExpressionSyntax node, out InvocationExpressionSyntax nested)
{
if (node.Expression is MemberAccessExpressionSyntax { Expression: InvocationExpressionSyntax receiver })
{
nested = receiver;
return true;
}

nested = null;
return false;
}
}

public override void VisitDeclarationExpression(DeclarationExpressionSyntax node)
{
var argumentSyntax = node.Parent as ArgumentSyntax;
Expand Down
43 changes: 42 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using ReferenceEqualityComparer = Roslyn.Utilities.ReferenceEqualityComparer;

namespace Microsoft.CodeAnalysis.CSharp
Expand Down Expand Up @@ -240,9 +241,49 @@ public override void VisitInvocationExpression(InvocationExpressionSyntax node)
return;
}

base.VisitInvocationExpression(node);
if (receiverIsInvocation(node, out InvocationExpressionSyntax? nested))
Copy link
Member

@jaredpar jaredpar Apr 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is roughly the same code on several derivations of CSharpSyntaxWalker? Did you consider putting a helper directly there that we could access? Probably cost us a Delegate allocation but maybe that's not too much. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? Did you consider putting a helper directly there that we could access?

There are only two of them and they are slightly different. I do not think it is worth adding an abstraction across them

{
var invocations = ArrayBuilder<InvocationExpressionSyntax>.GetInstance();

invocations.Push(node);

node = nested;
while (receiverIsInvocation(node, out nested))
{
invocations.Push(node);
node = nested;
}

Visit(node.Expression);

do
{
Visit(node.ArgumentList);
}
while (invocations.TryPop(out node!));

invocations.Free();
}
else
{
Visit(node.Expression);
Visit(node.ArgumentList);
}

return;

static bool receiverIsInvocation(InvocationExpressionSyntax node, [NotNullWhen(true)] out InvocationExpressionSyntax? nested)
{
if (node.Expression is MemberAccessExpressionSyntax { Expression: InvocationExpressionSyntax receiver } && !receiver.MayBeNameofOperator())
{
nested = receiver;
return true;
}

nested = null;
return false;
}

static Symbol getAttributeTarget(Binder current)
{
Debug.Assert((current.Flags & BinderFlags.InContextualAttributeBinder) != 0);
Expand Down
12 changes: 8 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs
Original file line number Diff line number Diff line change
Expand Up @@ -647,8 +647,6 @@ private void SetPatternLocalScopes(BoundObjectPattern pattern)

private void VisitArgumentsAndGetArgumentPlaceholders(BoundExpression? receiverOpt, ImmutableArray<BoundExpression> arguments)
{
Visit(receiverOpt);

for (int i = 0; i < arguments.Length; i++)
{
var arg = arguments[i];
Expand All @@ -663,7 +661,7 @@ private void VisitArgumentsAndGetArgumentPlaceholders(BoundExpression? receiverO
}
}

public override BoundNode? VisitCall(BoundCall node)
protected override void VisitArguments(BoundCall node)
{
VisitArgumentsAndGetArgumentPlaceholders(node.ReceiverOpt, node.Arguments);

Expand All @@ -682,7 +680,12 @@ private void VisitArgumentsAndGetArgumentPlaceholders(BoundExpression? receiverO
_diagnostics);
}

return null;
#if DEBUG
if (_visited is { } && _visited.Count <= MaxTrackVisited)
{
_visited.Add(node);
}
#endif
}

private void GetInterpolatedStringPlaceholders(
Expand Down Expand Up @@ -786,6 +789,7 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase

public override BoundNode? VisitIndexerAccess(BoundIndexerAccess node)
{
Visit(node.ReceiverOpt);
VisitArgumentsAndGetArgumentPlaceholders(node.ReceiverOpt, node.Arguments);

if (!node.HasErrors)
Expand Down
44 changes: 44 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -135,5 +135,49 @@ protected BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator
rightOperands.Free();
return null;
}

public sealed override BoundNode? VisitCall(BoundCall node)
{
if (node.ReceiverOpt is BoundCall receiver1)
{
var calls = ArrayBuilder<BoundCall>.GetInstance();

calls.Push(node);

node = receiver1;
while (node.ReceiverOpt is BoundCall receiver2)
{
calls.Push(node);
node = receiver2;
}

VisitReceiver(node);
Copy link
Member

@jaredpar jaredpar Apr 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this inside the do loop below? Seems like we miss intermediate receiver calls as written. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this inside the do loop below? Seems like we miss intermediate receiver calls as written.

The intermediate receiver is the previous call and we already visited all interesting parts of it. It is our intention to not call VisitReceiver for them. I'll add a comment.


do
{
VisitArguments(node);
}
while (calls.TryPop(out node!));

calls.Free();
}
else
{
VisitReceiver(node);
VisitArguments(node);
}

return null;
}

protected virtual void VisitReceiver(BoundCall node)
AlekseyTs marked this conversation as resolved.
Show resolved Hide resolved
{
this.Visit(node.ReceiverOpt);
}

protected virtual void VisitArguments(BoundCall node)
{
this.VisitList(node.Arguments);
}
}
}
39 changes: 22 additions & 17 deletions src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,8 @@ private LocalDefinition EmitAddress(BoundExpression expression, AddressKind addr

case BoundKind.Call:
var call = (BoundCall)expression;
var methodRefKind = call.Method.RefKind;

if (methodRefKind == RefKind.Ref ||
(IsAnyReadOnly(addressKind) && methodRefKind == RefKind.RefReadOnly))
if (UseCallResultAsAddress(call, addressKind))
{
EmitCallExpression(call, UseKind.UsedAsAddress);
break;
Expand Down Expand Up @@ -178,6 +176,13 @@ private LocalDefinition EmitAddress(BoundExpression expression, AddressKind addr
return null;
}

private static bool UseCallResultAsAddress(BoundCall call, AddressKind addressKind)
{
var methodRefKind = call.Method.RefKind;
return methodRefKind == RefKind.Ref ||
(IsAnyReadOnly(addressKind) && methodRefKind == RefKind.RefReadOnly);
}

private LocalDefinition EmitPassByCopyAddress(BoundPassByCopy passByCopyExpr, AddressKind addressKind)
{
// Normally we can just defer PassByCopy to the `default`,
Expand Down Expand Up @@ -505,7 +510,7 @@ private LocalDefinition EmitReceiverRef(BoundExpression receiver, AddressKind ad
return null;
}

if (receiverType.TypeKind == TypeKind.TypeParameter)
if (BoxNonVerifierReferenceReceiver(receiverType, addressKind))
{
//[Note: Constraints on a generic parameter only restrict the types that
//the generic parameter may be instantiated with. Verification (see Partition III)
Expand All @@ -514,26 +519,26 @@ private LocalDefinition EmitReceiverRef(BoundExpression receiver, AddressKind ad
//via the generic parameter unless it is first boxed (see Partition III) or
//the callvirt instruction is prefixed with the constrained. prefix instruction
//(see Partition III). end note]
if (addressKind == AddressKind.Constrained)
EmitExpression(receiver, used: true);
// conditional receivers are already boxed if needed when pushed
if (receiver.Kind != BoundKind.ConditionalReceiver)
{
return EmitAddress(receiver, addressKind);
}
else
{
EmitExpression(receiver, used: true);
// conditional receivers are already boxed if needed when pushed
if (receiver.Kind != BoundKind.ConditionalReceiver)
{
EmitBox(receiver.Type, receiver.Syntax);
}
return null;
EmitBox(receiver.Type, receiver.Syntax);
}

return null;
}

Debug.Assert(receiverType.IsVerifierValue());
Debug.Assert(receiverType.TypeKind == TypeKind.TypeParameter || receiverType.IsValueType);
return EmitAddress(receiver, addressKind);
}

private static bool BoxNonVerifierReferenceReceiver(TypeSymbol receiverType, AddressKind addressKind)
{
Debug.Assert(!receiverType.IsVerifierReference());
return receiverType.TypeKind == TypeKind.TypeParameter && addressKind != AddressKind.Constrained;
}

/// <summary>
/// May introduce a temp which it will return. (otherwise returns null)
/// </summary>
Expand Down
Loading