Repository navigation
Trace async bodies with a debug hook - #369
Conversation
slozier
left a comment
slozier
left a comment
There was a problem hiding this comment.
This one is outside of my comfort zone so I'll defer to you. What I did check seemed fine. Presumably I'd need the IronPython side of it to try and break things?
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved tracing and lowering defects can swallow exceptions, misexecute handlers, reject valid bodies, or dereference a missing generator factory.
5 open issues
What changed in this PR
Adds opt-in tracing for async bodies, extending the support left unresolved by #363 without changing the async drivers.
Changes:
- Adds debug hooks and sequence-point instrumentation to async expressions.
- Wraps state machines to dispatch tracing events.
- Guards statement relocation for frames without generator factories.
| File | Description |
|---|---|
| src/core/Microsoft.Dynamic/Debugging/TracingStateMachine.cs | Dispatches async trace events and manages frame lifetime. |
| src/core/Microsoft.Dynamic/Debugging/TracePipeline/TracePipeline.cs | Guards unsupported statement relocation. |
| src/core/Microsoft.Dynamic/Debugging/DebugInfoRewriter.cs | Updates comments describing async tracing. |
| src/core/Microsoft.Dynamic/Ast/GeneratorRewriter.cs | Updates comments describing async boundaries. |
| src/core/Microsoft.Dynamic/Ast/AsyncRewriter.cs | Integrates tracing and shares exception-slot checks. |
| src/core/Microsoft.Dynamic/Ast/AsyncExpression.cs | Adds and preserves debug hooks. |
| src/core/Microsoft.Dynamic/Ast/AsyncEnumerableRewriter.cs | Integrates async-enumerable tracing. |
| src/core/Microsoft.Dynamic/Ast/AsyncEnumerableExpression.cs | Adds and preserves debug hooks. |
| src/core/Microsoft.Dynamic/Ast/AsyncDebugSupport.cs | Instruments sequence points and exception handlers. |
Review effort: Balanced
|
|
||
| return Expression.Block( | ||
| typeof(void), | ||
| Utils.YieldReturn(_yieldLabel, Expression.Constant(new DebugPoint(cookie))), |
There was a problem hiding this comment.
The observation is correct but not applicable for this PR. Async tracing deliberately behaves like the DLR's sync tracing here, and this limitation is DLR-wide:
- Sync tracing has it too.
DebugContext.TransformLambdaturns every sequence point into a yield in its debug generator, andGeneratorRewriterdoesn't allow yields in catch filters or fault blocks. So a lambda with a sequence point inside a filter or fault block already fails to transform for sync tracing (yield in filter is not allowed/yield in fault block is not supported). Filters without sequence points trace fine. - The DLR interpreter doesn't support catch filters at all.
LightCompilerthrows NRE for any filter, traced or not. So a language that compiles its exception tests into .NET filters is already limited to compiled code. - IronPython isn't affected. It emits neither filters nor fault blocks:
exceptclauses are tested inside a plaincatch (Exception)body. IronPython has no filters to step through. A Python except clause is compiled as ordinary code inside a plaincatch (Exception)block: an if/else chain that callsCheckExceptionwith each clause's expression in turn, and rethrows if none matches. So everything Python considers part of theexcepttest runs in the catch body, where sequence points are allowed (in async bodies, debug-point yields there are supported, through the deferred-handler lowering). Perhaps other languages do something similar to sidestep the DLR limitation.
If sequence points in filters and fault blocks need supporting (e.g. for a language with C#-like catch … when), it should be done for sync and async tracing together, for example by emitting no debug point there, or a non-yielding one.
| } | ||
|
|
||
| protected override Expression VisitTry(TryExpression node) { | ||
| var visited = (TryExpression)base.VisitTry(node); |
There was a problem hiding this comment.
Resolved in 7ece84a: The variables are now reset on each entry into the try. A resumption into the handler is routed past the reset, so the exception still survives the handler's own yields.
This change to the DLR by itself does not affect IronPython. IronPython has its own copy of GeneratorRewriter so the change goes there too.
| // A nested async node is sliced into its own state machine against its own label; leave it opaque. | ||
| if (node is AsyncExpression |
There was a problem hiding this comment.
Indeed, though IronPython never builds inline DLR generators. Accepted for correctes (in 87ea5cb), though no test coverage.
| ) { | ||
| return node; | ||
| } | ||
| return base.VisitExtension(node); |
There was a problem hiding this comment.
Confirmed: when the trace function raises on the call event as an async generator resumes after a yield, CPython raises it at the yield and stops. IronPython swallows it and continues.
The cause: the wrapper parks the exception in the exception slot, but a plain yield has no "rethrow if pending" check after it. The next debug point overwrites the slot. In practice this hits pdb, whose q makes the next call event raise BdbQuit.
Adding the check after yields on the body's own label is safe. The driver always clears the slot before resuming past a yield, so the check can only see the wrapper's exception.
I am accepting this fix in a separate commit (8068b3e), because for some reason after I accepted the suggestion from comment 5 (in TracePipeline.cs), suddenly all proposed autofix suggestions by Copilot turned as "outdated".
IPY will carry a new test that covers this issue.
|
@BCSharp feel free to ignore AI review if not relevant, just trying it out. |
This issue has no relevance for IronPython, but it prevents an NRE in other language hosts that call `TrySetNextStatement` directly. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Well, it was useful. Of the five problem areas "only" two can be triggered by IronPython, but the DLR should work with any dynamic language, so all five need fixing. I will add some tests to IronPython to catch those two visible cases; they will land with the IPY PR. One of them fails on the "old" async implementation in IPY as well; I will think about it when I get there. This PR is an N-th iteration for this problem I tried and it changed substantially in the process. I see there are still some sharp corners, but the Copilot did a good job of raising the issues. This is the old challenge: tracing using yields can put them in some difficult places: in handlers, in |
Yes, this one is annoying and has caught me multiple times. Definitely something I want to look into again... |
Indeed, it is also on my TODO list (already for a long time 😃 ). I consider it important and it is finally getting close to the top. |
This is a continuation of #363, which left tracing unresolved.
In the approach in this PR, an
AsyncExpression/AsyncEnumerableExpressioncan carry a debug hook (WithDebugInfo). When it does, its reduction instruments the body (aDebugPointyield at each sequence point, an exception report at the start of each catch block) and wraps the body's state machine in aTracingStateMachine, which dispatches call/line/return/exception events through theDebugContext. The drivers inAsyncHelpersare unchanged.I have the corresponding change to IronPython that makes use of this mechanism.
A side change:
TracePipeline.TrySetNextStatementreturns false for a frame without a generator factory instead of failing with a NRE.