Skip to content

Commit

Permalink
Fix sequence point update for switch/if/for/while/do-while/do-until s…
Browse files Browse the repository at this point in the history 
…tatements (PowerShell#7305)

- Make switch-statement report correct error position when it fails to evaluate the condition.
- Make for-statement report correct error position when it fails to evaluate the initializer.
- For the condition of `if/for/while/do-while/do-until` statements, the sequence point update is either duplicate in some cases (for `if/for/while`) which causes debugger to stop at the condition twice  before moving forward, or missing (for `do-while/do-until`) which causes debugger to skip the condition. They are fixed.
  • Loading branch information
@daxian-dbw
daxian-dbw committed Jul 25, 2018
1 parent 439f97c commit 87d8fc9
Show file tree
Hide file tree
Showing 3 changed files with 461 additions and 44 deletions.
103 changes: 77 additions & 26 deletions src/System.Management.Automation/engine/parser/Compiler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -743,14 +743,16 @@ static Compiler()
s_builtinAttributeGenerator.Add(typeof(ValidateNotNullOrEmptyAttribute), NewValidateNotNullOrEmptyAttribute);
}

private Compiler(List<IScriptExtent> sequencePoints)
private Compiler(List<IScriptExtent> sequencePoints, Dictionary<IScriptExtent, int> sequencePointIndexMap)
{
_sequencePoints = sequencePoints;
_sequencePointIndexMap = sequencePointIndexMap;
}

internal Compiler()
{
_sequencePoints = new List<IScriptExtent>();
_sequencePointIndexMap = new Dictionary<IScriptExtent, int>();
}

internal bool CompilingConstantExpression { get; set; }
Expand All @@ -766,6 +768,7 @@ internal Compiler()
private int _switchTupleIndex = VariableAnalysis.Unanalyzed;
private int _foreachTupleIndex = VariableAnalysis.Unanalyzed;
private readonly List<IScriptExtent> _sequencePoints;
private readonly Dictionary<IScriptExtent, int> _sequencePointIndexMap;
private int _stmtCount;

internal bool CompilingMemberFunction { get; set; }
Expand Down Expand Up @@ -903,17 +906,31 @@ internal static Expression IsStrictMode(int version, Expression executionContext
ExpressionCache.Constant(version));
}

internal Expression UpdatePosition(Ast ast)
private int AddSequencePoint(IScriptExtent extent)
{
_sequencePoints.Add(ast.Extent);
// Make sure we don't add the same extent to the sequence point list twice.
if (!_sequencePointIndexMap.TryGetValue(extent, out int index))
{
_sequencePoints.Add(extent);
index = _sequencePoints.Count - 1;
_sequencePointIndexMap.Add(extent, index);
}

return index;
}

private Expression UpdatePosition(Ast ast)
{
IScriptExtent extent = ast.Extent;
int index = AddSequencePoint(extent);

// If we just added the first sequence point, then we don't want to check for breakpoints - we'll do that
// in EnterScriptFunction.
// Except for while/do loops, in this case we want to check breakpoints on the first sequence point since it
// will be executed multiple times.
return ((_sequencePoints.Count == 1) && !_generatingWhileOrDoLoop)
return (index == 0 && !_generatingWhileOrDoLoop)
? ExpressionCache.Empty
: new UpdatePositionExpr(ast.Extent, _sequencePoints.Count - 1, _debugSymbolDocument, !_compilingSingleExpression);
: new UpdatePositionExpr(extent, index, _debugSymbolDocument, !_compilingSingleExpression);
}

private int _tempCounter;
Expand Down Expand Up @@ -1683,7 +1700,7 @@ internal void Compile(CompiledScriptBlockData scriptBlock, bool optimize)
// on this sequence point, but it makes it safe to access the CurrentPosition
// property in FunctionContext (which can happen if there are exceptions
// defining the functions.)
_sequencePoints.Add(ast.Extent);
AddSequencePoint(ast.Extent);
}

var compileInterpretChoice = (_stmtCount > 300) ? CompileInterpretChoice.NeverCompile : CompileInterpretChoice.CompileOnDemand;
Expand Down Expand Up @@ -1853,8 +1870,8 @@ private Func<FunctionContext, object> CompileSingleExpression(ExpressionAst expr
var exprs = new List<Expression>();
var temps = new List<ParameterExpression> { _executionContextParameter, LocalVariablesParameter };
GenerateFunctionProlog(exprs, temps, null);
_sequencePoints.Add(expressionAst.Extent);
exprs.Add(new UpdatePositionExpr(expressionAst.Extent, _sequencePoints.Count - 1, _debugSymbolDocument, checkBreakpoints: true));
int index = AddSequencePoint(expressionAst.Extent);
exprs.Add(new UpdatePositionExpr(expressionAst.Extent, index, _debugSymbolDocument, checkBreakpoints: true));
var result = Compile(expressionAst).Cast(typeof(object));
exprs.Add(Expression.Label(_returnTarget, result));
var body = Expression.Block(new[] { _executionContextParameter, LocalVariablesParameter }, exprs);
Expand Down Expand Up @@ -2143,7 +2160,7 @@ private Expression<Action<FunctionContext>> CompileNamedBlock(NamedBlockAst name

private Tuple<Action<FunctionContext>, Type> CompileTrap(TrapStatementAst trap)
{
var compiler = new Compiler(_sequencePoints) { _compilingTrap = true };
var compiler = new Compiler(_sequencePoints, _sequencePointIndexMap) { _compilingTrap = true };
string funcName = _currentFunctionName + "<trap>";
if (trap.TrapType != null)
{
Expand Down Expand Up @@ -2517,8 +2534,8 @@ private void GenerateFunctionProlog(List<Expression> exprs, List<ParameterExpres

if (entryExtent != null)
{
_sequencePoints.Add(entryExtent);
exprs.Add(new UpdatePositionExpr(entryExtent, _sequencePoints.Count - 1, _debugSymbolDocument, checkBreakpoints: false));
int index = AddSequencePoint(entryExtent);
exprs.Add(new UpdatePositionExpr(entryExtent, index, _debugSymbolDocument, checkBreakpoints: false));
}

exprs.Add(
Expand Down Expand Up @@ -2568,8 +2585,8 @@ private Expression InitializeMemberProperties(Expression ourThis)
}

var extent = propertyMember.InitialValue.Extent;
_sequencePoints.Add(extent);
exprs.Add(new UpdatePositionExpr(extent, _sequencePoints.Count - 1, _debugSymbolDocument, checkBreakpoints: false));
int index = AddSequencePoint(extent);
exprs.Add(new UpdatePositionExpr(extent, index, _debugSymbolDocument, checkBreakpoints: false));
var property = _memberFunctionType.Type.GetProperty(propertyMember.Name, bindingFlags);
exprs.Add(
Expression.Assign(
Expand Down Expand Up @@ -2952,12 +2969,12 @@ public object VisitIfStatement(IfStatementAst ifStmtAst)
{
int clauseCount = ifStmtAst.Clauses.Count;

var exprs = new Tuple<BlockExpression, Expression>[clauseCount];
var exprs = new Tuple<Expression, Expression>[clauseCount];
for (int i = 0; i < clauseCount; ++i)
{
IfClause ifClause = ifStmtAst.Clauses[i];
var cond = Expression.Block(
UpdatePosition(ifClause.Item1),
var cond = UpdatePositionForInitializerOrCondition(
ifClause.Item1,
CaptureStatementResults(ifClause.Item1, CaptureAstContext.Condition).Convert(typeof(bool)));
var body = Compile(ifClause.Item2);
exprs[i] = Tuple.Create(cond, body);
Expand Down Expand Up @@ -3022,7 +3039,7 @@ private Expression CompileAssignment(
}

var exprs = new List<Expression>
{
{
// Set current position in case of errors.
UpdatePosition(assignmentStatementAst),
ReduceAssignment((ISupportsAssignment)assignmentStatementAst.Left,
Expand Down Expand Up @@ -4174,7 +4191,7 @@ private Expression GenerateDoLoop(LoopStatementAst loopStatement)
_loopTargets.Add(new LoopGotoTargets(loopLabel ?? string.Empty, breakLabel, continueLabel));
_generatingWhileOrDoLoop = true;
var loopBodyExprs = new List<Expression>
{
{
s_callCheckForInterrupts,
Compile(loopStatement.Body),
ExpressionCache.Empty
Expand All @@ -4190,6 +4207,7 @@ private Expression GenerateDoLoop(LoopStatementAst loopStatement)
{
test = Expression.Not(test);
}
test = UpdatePositionForInitializerOrCondition(loopStatement.Condition, test);
exprs.Add(Expression.IfThen(test, Expression.Goto(repeatLabel)));
exprs.Add(Expression.Label(breakLabel));

Expand Down Expand Up @@ -4240,10 +4258,26 @@ private Expression GenerateIteratorStatement(VariablePath iteratorVariablePath,
// $foreach/$switch = GetEnumerator $enumerable
var enumerable = NewTemp(typeof(object), "enumerable");
temps.Add(enumerable);

// Update position to make it safe to access 'CurrentPosition' property in FunctionContext in case
// that the evaluation of 'stmt.Condition' throws exception.
if (generatingForeach)
{
// For foreach statement, we want the debugger to stop at 'stmt.Condition' before evaluating it.
// The debugger will stop at 'stmt.Condition' only once. The following enumeration will stop at
// the foreach variable.
exprs.Add(UpdatePosition(stmt.Condition));
}
else
{
// For switch statement, we don't want the debugger to stop at 'stmt.Condition' before evaluating it.
// The following enumeration will stop at 'stmt.Condition' again, and we don't want the debugger to
// stop at 'stmt.Condition' twice before getting into one of its case clauses.
var extent = stmt.Condition.Extent;
int index = AddSequencePoint(extent);
exprs.Add(new UpdatePositionExpr(extent, index, _debugSymbolDocument, checkBreakpoints: false));
}

exprs.Add(
Expression.Assign(enumerable,
GetRangeEnumerator(stmt.Condition.GetPureExpression())
Expand Down Expand Up @@ -4354,6 +4388,19 @@ private Expression GetRangeEnumerator(ExpressionAst condExpr)
return null;
}

private Expression UpdatePositionForInitializerOrCondition(PipelineBaseAst pipelineBaseAst, Expression initializerOrCondition)
{
if (pipelineBaseAst is PipelineAst pipelineAst && !pipelineAst.Background && pipelineAst.GetPureExpression() != null)
{
// If the initializer or condition is a pure expression (CommandExpressionAst without redirection),
// then we need to add a sequence point. If it's an AssignmentStatementAst, we don't need to add
// sequence point here because one will be added when processing the AssignmentStatementAst.
initializerOrCondition = Expression.Block(UpdatePosition(pipelineBaseAst), initializerOrCondition);
}

return initializerOrCondition;
}

public object VisitDoWhileStatement(DoWhileStatementAst doWhileStatementAst)
{
return GenerateDoLoop(doWhileStatementAst);
Expand All @@ -4367,13 +4414,17 @@ public object VisitDoUntilStatement(DoUntilStatementAst doUntilStatementAst)
public object VisitForStatement(ForStatementAst forStatementAst)
{
// We should not preserve the partial output if exception is thrown when evaluating the initializer.
var init = (forStatementAst.Initializer != null)
? CaptureStatementResults(forStatementAst.Initializer, CaptureAstContext.AssignmentWithoutResultPreservation)
: null;
Expression init = null;
PipelineBaseAst initializer = forStatementAst.Initializer;
if (initializer != null)
{
init = CaptureStatementResults(initializer, CaptureAstContext.AssignmentWithoutResultPreservation);
init = UpdatePositionForInitializerOrCondition(initializer, init);
}

var generateCondition = forStatementAst.Condition != null
? () => Expression.Block(UpdatePosition(forStatementAst.Condition),
CaptureStatementResults(forStatementAst.Condition, CaptureAstContext.Condition))
PipelineBaseAst condition = forStatementAst.Condition;
var generateCondition = condition != null
? () => UpdatePositionForInitializerOrCondition(condition, CaptureStatementResults(condition, CaptureAstContext.Condition))
: (Func<Expression>)null;

var loop = GenerateWhileLoop(forStatementAst.Label, generateCondition,
Expand All @@ -4389,9 +4440,9 @@ public object VisitForStatement(ForStatementAst forStatementAst)

public object VisitWhileStatement(WhileStatementAst whileStatementAst)
{
PipelineBaseAst condition = whileStatementAst.Condition;
return GenerateWhileLoop(whileStatementAst.Label,
() => Expression.Block(UpdatePosition(whileStatementAst.Condition),
CaptureStatementResults(whileStatementAst.Condition, CaptureAstContext.Condition)),
() => UpdatePositionForInitializerOrCondition(condition, CaptureStatementResults(condition, CaptureAstContext.Condition)),
(loopBody, breakTarget, continueTarget) => loopBody.Add(Compile(whileStatementAst.Body)));
}

Expand Down
Loading

0 comments on commit 87d8fc9

Please sign in to comment.