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

Expression breakpoints for switch expression. #42711

Merged
merged 7 commits into from
Apr 1, 2020
Merged
Show file tree
Hide file tree
Changes from 2 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
32 changes: 31 additions & 1 deletion src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,37 @@
<Field Name="StatementOpt" Type="BoundStatement?"/>
<Field Name="Span" Type="TextSpan"/>
</Node>


<!--
This is used to save the debugger's idea of what the enclosing sequence
point is at this location in the code so that it can be restored later
by a BoundRestorePreviousSequencePoint node. When this statement appears,
the previous non-hidden sequence point is saved and associated with the
given Identifier.
-->
<Node Name="BoundSavePreviousSequencePoint" Base="BoundStatement">
<Field Name="Identifier" Type="object"/>
</Node>

<!--
This is used to restore the debugger's idea of what the enclosing statement
is to some previous location without introducing a place where a breakpoint
would cause the debugger to stop. The identifier must have
previously been given in a BoundSavePreviousSequencePoint statement. This is used
to implement breakpoints within expressions (e.g. a switch expression).
-->
<Node Name="BoundRestorePreviousSequencePoint" Base="BoundStatement">
<Field Name="Identifier" Type="object"/>
</Node>

<!--
This is used to set the debugger's idea of what the enclosing statement
is without causing the debugger to stop here when single stepping.
-->
<Node Name="BoundStepThroughSequencePoint" Base="BoundStatement">
<Field Name="Span" Type="TextSpan"/>
</Node>

<!--
BoundBlock contains
a) Statements - actions performed within the scope of the block
Expand Down
60 changes: 59 additions & 1 deletion src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ internal sealed partial class CodeGenerator

// There are scenarios where rvalues need to be passed to ref/in parameters
// in such cases the values must be spilled into temps and retained for the entirety of
// the most encompassing expression.
// the most encompassing expression.
private ArrayBuilder<LocalDefinition> _expressionTemps;

// not 0 when in a protected region with a handler.
Expand All @@ -57,6 +57,11 @@ internal sealed partial class CodeGenerator
/// </summary>
private IndirectReturnState _indirectReturnState;

/// <summary>
/// Used to implement <see cref="BoundSavePreviousSequencePoint"/> and <see cref="BoundRestorePreviousSequencePoint"/>.
/// </summary>
private PooledDictionary<object, TextSpan> _savedSequencePoints;

private enum IndirectReturnState : byte
{
NotNeeded = 0, // did not see indirect returns
Expand Down Expand Up @@ -294,6 +299,7 @@ private void GenerateImpl()

Debug.Assert(!(_expressionTemps?.Count > 0), "leaking expression temps?");
_expressionTemps?.Free();
_savedSequencePoints?.Free();
}

private void HandleReturn()
Expand Down Expand Up @@ -401,6 +407,58 @@ private void EmitSequencePointStatement(BoundSequencePointWithSpan node)
}
}

private void EmitSavePreviousSequencePoint(BoundSavePreviousSequencePoint statement)
{
if (!_emitPdbSequencePoints)
return;

ArrayBuilder<RawSequencePoint> sequencePoints = _builder.SeqPointsOpt;
if (sequencePoints is null)
return;

for (int i = sequencePoints.Count - 1; i >= 0; i--)
{
var span = sequencePoints[i].Span;
if (span == RawSequencePoint.HiddenSequencePointSpan)
continue;

// Found the previous non-hidden sequence point. Save it.
_savedSequencePoints ??= PooledDictionary<object, TextSpan>.GetInstance();
_savedSequencePoints.Add(statement.Identifier, span);
return;
}
}
gafter marked this conversation as resolved.
Show resolved Hide resolved

private void EmitRestorePreviousSequencePoint(BoundRestorePreviousSequencePoint node)
{
Debug.Assert(node.Syntax is { });
if (_savedSequencePoints is null || !_savedSequencePoints.TryGetValue(node.Identifier, out var span))
return;

EmitStepThroughSequencePoint(node.Syntax.SyntaxTree, span);
}

private void EmitStepThroughSequencePoint(BoundStepThroughSequencePoint node)
{
EmitStepThroughSequencePoint(node.Syntax.SyntaxTree, node.Span);
}

private void EmitStepThroughSequencePoint(SyntaxTree syntaxTree, TextSpan span)
{
if (!_emitPdbSequencePoints)
return;

var label = new object();
// The IL builder is eager to discard unreachable code, so
jcouv marked this conversation as resolved.
Show resolved Hide resolved
// we fool it by branching on a condition that is always true at runtime.
_builder.EmitConstantValue(ConstantValue.Create(true));
_builder.EmitBranch(ILOpCode.Brtrue, label);
EmitSequencePoint(syntaxTree, span);
_builder.EmitOpCode(ILOpCode.Nop);
Copy link

Choose a reason for hiding this comment

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

@gafter Just curious if sequence points can be empty w/o instructions, why having EmitSequencePoint() followed by EmitHiddenSequencePoint() is not sufficient?

I'm asking because coverage tools show uncovered statements, for example: https://developercommunity.visualstudio.com/content/problem/1140065/coverage-on-switch-expression-dropped-in-167.html , and now they should always ignore the unreachable SP in ldc.i4.1 brtrue.s 1 nop pattern. Is there a guarantee the user's code will never produce such IL?

Copy link
Member Author

Choose a reason for hiding this comment

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

_builder.MarkLabel(label);
EmitHiddenSequencePoint();
}

private void SetInitialDebugDocument()
{
if (_emitPdbSequencePoints && _methodBodySyntaxOpt != null)
Expand Down
12 changes: 12 additions & 0 deletions src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,18 @@ private void EmitStatement(BoundStatement statement)
this.EmitSequencePointStatement((BoundSequencePointWithSpan)statement);
break;

case BoundKind.SavePreviousSequencePoint:
this.EmitSavePreviousSequencePoint((BoundSavePreviousSequencePoint)statement);
break;

case BoundKind.RestorePreviousSequencePoint:
this.EmitRestorePreviousSequencePoint((BoundRestorePreviousSequencePoint)statement);
break;

case BoundKind.StepThroughSequencePoint:
this.EmitStepThroughSequencePoint((BoundStepThroughSequencePoint)statement);
break;

case BoundKind.ExpressionStatement:
EmitExpression(((BoundExpressionStatement)statement).Expression, false);
break;
Expand Down
15 changes: 15 additions & 0 deletions src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3064,6 +3064,21 @@ protected virtual void VisitAssignmentOfNullCoalescingAssignment(
}
}

public override BoundNode VisitSavePreviousSequencePoint(BoundSavePreviousSequencePoint node)
{
return null;
}

public override BoundNode VisitRestorePreviousSequencePoint(BoundRestorePreviousSequencePoint node)
{
return null;
}

public override BoundNode VisitStepThroughSequencePoint(BoundStepThroughSequencePoint node)
{
return null;
}

/// <summary>
/// This visitor represents just the non-assignment part of the null coalescing assignment
/// operator (when the left operand is non-null).
Expand Down
144 changes: 144 additions & 0 deletions src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ protected BaseSwitchLocalRewriter(

// We start each switch block of a switch statement with a hidden sequence point so that
// we do not appear to be in the previous switch block when we begin.
if (IsSwitchStatement)
if (GenerateSequencePoints)
armBuilder.Add(_factory.HiddenSequencePoint());

_switchArms.Add(arm, armBuilder);
Expand Down
Loading