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.

Sign up for GitHub

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

Clear hoisted locals when disposing iterator and async-iterator state machines #75908

Merged
merged 20 commits into from
Nov 21, 2024
Merged

Clear hoisted locals when disposing iterator and async-iterator state machines #75908

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
accbe6c
Add more tests for baseline
jcouv Nov 9, 2024
04902e1
Change for iterators and async-iterators
jcouv Nov 12, 2024
61ca295
Remove VerifyIL
jcouv Nov 14, 2024
e19d7ab
Refine tests
jcouv Nov 14, 2024
386a056
typo
jcouv Nov 14, 2024
cdcadd0
Add test baseline
jcouv Nov 15, 2024
e170504
Clear all hoisted locals in async too
jcouv Nov 15, 2024
0d40220
Collect hoisted fields when adding to proxies instead
jcouv Nov 15, 2024
b52733e
Revert name changes
jcouv Nov 15, 2024
3ac0137
Move to local function
jcouv Nov 15, 2024
37d3b3d
Tweaks
jcouv Nov 15, 2024
e948e85
Typo
jcouv Nov 15, 2024
b1a5ebb
Report use-site diagnostic from IsManagedType
jcouv Nov 16, 2024
fdd444c
Revert "Clear all hoisted locals in async too"
jcouv Nov 16, 2024
3eb020f
Clean up a test
jcouv Nov 18, 2024
58ab13e
Collect fields to be cleared when they are created
jcouv Nov 18, 2024
0df2e4b
Rename parameter
jcouv Nov 18, 2024
0f63ecd
Use ImmutableArray
jcouv Nov 18, 2024
5529fd6
usings
jcouv Nov 18, 2024
57cafd7
Address feedback
jcouv Nov 19, 2024
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
20 changes: 18 additions & 2 deletions ...ilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncIteratorMethodToStateMachineRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,14 @@ internal AsyncIteratorMethodToStateMachineRewriter(
FieldSymbol? instanceIdField,
IReadOnlySet<Symbol> hoistedVariables,
IReadOnlyDictionary<Symbol, CapturedSymbolReplacement> nonReusableLocalProxies,
ImmutableArray<FieldSymbol> nonReusableFieldsForCleanup,
SynthesizedLocalOrdinalsDispenser synthesizedLocalOrdinals,
ArrayBuilder<StateMachineStateDebugInfo> stateMachineStateDebugInfoBuilder,
VariableSlotAllocator? slotAllocatorOpt,
int nextFreeHoistedLocalSlot,
BindingDiagnosticBag diagnostics)
: base(method, methodOrdinal, asyncMethodBuilderMemberCollection, F,
state, builder, instanceIdField, hoistedVariables, nonReusableLocalProxies, synthesizedLocalOrdinals,
state, builder, instanceIdField, hoistedVariables, nonReusableLocalProxies, nonReusableFieldsForCleanup, synthesizedLocalOrdinals,
stateMachineStateDebugInfoBuilder, slotAllocatorOpt, nextFreeHoistedLocalSlot, diagnostics)
{
Debug.Assert(asyncIteratorInfo != null);
Expand All @@ -88,11 +89,21 @@ internal AsyncIteratorMethodToStateMachineRewriter(

return (asyncDispatch != null) ? F.Block(asyncDispatch, iteratorDispatch) : iteratorDispatch;
}

protected override BoundStatement GenerateHoistedLocalsCleanupForExit(ImmutableArray<StateMachineFieldSymbol> rootHoistedLocals)
{
// We need to clean nested hoisted local variables too (not just top-level ones)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We need to clean nested hoisted local variables too (not just top-level ones)

This isn't obvious. It looks like there are two call sites of this method and it is not clear why we want to have this behavior for all of them. Instead of completely ignoring the passed argument, consider adjusting the call sites to construct the right set of locals to clear. We can discuss offline in more details. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior could be a source of bugs in the future, since the method doesn't do what a reasonable dev would expect it to do.

// as they are not cleaned when exiting a block if we exit using a `yield break`
// or if the caller interrupts the enumeration after we reached a `yield return`.
// So we clean both top-level and nested hoisted local variables
return GenerateAllHoistedLocalsCleanup();
}
#nullable disable
protected override BoundStatement GenerateSetResultCall()
{
// ... _exprReturnLabel: ...
// ... this.state = FinishedState; ...
// ... hoisted locals cleanup ...

// if (this.combinedTokens != null) { this.combinedTokens.Dispose(); this.combinedTokens = null; } // for enumerables only
// _current = default;
Expand Down Expand Up @@ -319,7 +330,12 @@ public override BoundNode VisitYieldBreakStatement(BoundYieldBreakStatement node

protected override BoundStatement MakeAwaitPreamble()
{
if (_asyncIteratorInfo.CurrentField.Type.IsManagedTypeNoUseSiteDiagnostics)
var useSiteInfo = new CompoundUseSiteInfo<AssemblySymbol>(F.Diagnostics, F.Compilation.Assembly);
var field = _asyncIteratorInfo.CurrentField;
bool isManaged = field.Type.IsManagedType(ref useSiteInfo);
F.Diagnostics.Add(field.GetFirstLocationOrNone(), useSiteInfo);

if (isManaged)
{
// _current = default;
return GenerateClearCurrent();
Expand Down
83 changes: 43 additions & 40 deletions src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncMethodToStateMachineRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
Expand Down Expand Up @@ -72,12 +73,13 @@ internal AsyncMethodToStateMachineRewriter(
FieldSymbol? instanceIdField,
IReadOnlySet<Symbol> hoistedVariables,
IReadOnlyDictionary<Symbol, CapturedSymbolReplacement> nonReusableLocalProxies,
ImmutableArray<FieldSymbol> nonReusableFieldsForCleanup,
SynthesizedLocalOrdinalsDispenser synthesizedLocalOrdinals,
ArrayBuilder<StateMachineStateDebugInfo> stateMachineStateDebugInfoBuilder,
VariableSlotAllocator? slotAllocatorOpt,
int nextFreeHoistedLocalSlot,
BindingDiagnosticBag diagnostics)
: base(F, method, state, instanceIdField, hoistedVariables, nonReusableLocalProxies, synthesizedLocalOrdinals, stateMachineStateDebugInfoBuilder, slotAllocatorOpt, nextFreeHoistedLocalSlot, diagnostics)
: base(F, method, state, instanceIdField, hoistedVariables, nonReusableLocalProxies, nonReusableFieldsForCleanup, synthesizedLocalOrdinals, stateMachineStateDebugInfoBuilder, slotAllocatorOpt, nextFreeHoistedLocalSlot, diagnostics)
{
_method = method;
_asyncMethodBuilderMemberCollection = asyncMethodBuilderMemberCollection;
Expand Down Expand Up @@ -155,7 +157,7 @@ internal void GenerateMoveNext(BoundStatement body, MethodSymbol moveNextMethod)
// [body]
rewrittenBody
),
F.CatchBlocks(GenerateExceptionHandling(exceptionLocal, rootScopeHoistedLocals)))
F.CatchBlocks(generateExceptionHandling(exceptionLocal, rootScopeHoistedLocals)))
);

// ReturnLabel (for the rewritten return expressions in the user's method body)
Expand All @@ -176,7 +178,7 @@ internal void GenerateMoveNext(BoundStatement body, MethodSymbol moveNextMethod)
// The remaining code is hidden to hide the fact that it can run concurrently with the task's continuation
}

bodyBuilder.Add(GenerateHoistedLocalsCleanup(rootScopeHoistedLocals));
bodyBuilder.Add(GenerateHoistedLocalsCleanupForExit(rootScopeHoistedLocals));

bodyBuilder.Add(GenerateSetResultCall());

Expand Down Expand Up @@ -206,6 +208,42 @@ internal void GenerateMoveNext(BoundStatement body, MethodSymbol moveNextMethod)
newBody = F.Instrument(newBody, instrumentation);

F.CloseMethod(newBody);
return;

BoundCatchBlock generateExceptionHandling(LocalSymbol exceptionLocal, ImmutableArray<StateMachineFieldSymbol> rootHoistedLocals)
{
// catch (Exception ex)
// {
// _state = finishedState;
//
// for each hoisted local:
// <>x__y = default
//
// builder.SetException(ex); OR if (this.combinedTokens != null) this.combinedTokens.Dispose(); _promiseOfValueOrEnd.SetException(ex); /* for async-iterator method */
// return;
// }

// _state = finishedState;
BoundStatement assignFinishedState =
F.ExpressionStatement(F.AssignmentExpression(F.Field(F.This(), stateField), F.Literal(StateMachineState.FinishedState)));

// builder.SetException(ex); OR if (this.combinedTokens != null) this.combinedTokens.Dispose(); _promiseOfValueOrEnd.SetException(ex);
BoundStatement callSetException = GenerateSetExceptionCall(exceptionLocal);

return new BoundCatchBlock(
F.Syntax,
ImmutableArray.Create(exceptionLocal),
F.Local(exceptionLocal),
exceptionLocal.Type,
exceptionFilterPrologueOpt: null,
exceptionFilterOpt: null,
body: F.Block(
assignFinishedState, // _state = finishedState;
GenerateHoistedLocalsCleanupForExit(rootHoistedLocals),
callSetException, // builder.SetException(ex); OR _promiseOfValueOrEnd.SetException(ex);
GenerateReturn(false)), // return;
isSynthesizedAsyncCatchAll: true);
}
}

protected virtual BoundStatement GenerateTopLevelTry(BoundBlock tryBlock, ImmutableArray<BoundCatchBlock> catchBlocks)
Expand All @@ -223,48 +261,13 @@ protected virtual BoundStatement GenerateSetResultCall()
: ImmutableArray<BoundExpression>.Empty));
}

protected BoundCatchBlock GenerateExceptionHandling(LocalSymbol exceptionLocal, ImmutableArray<StateMachineFieldSymbol> hoistedLocals)
{
// catch (Exception ex)
// {
// _state = finishedState;
//
// for each hoisted local:
// <>x__y = default
//
// builder.SetException(ex); OR if (this.combinedTokens != null) this.combinedTokens.Dispose(); _promiseOfValueOrEnd.SetException(ex); /* for async-iterator method */
// return;
// }

// _state = finishedState;
BoundStatement assignFinishedState =
F.ExpressionStatement(F.AssignmentExpression(F.Field(F.This(), stateField), F.Literal(StateMachineState.FinishedState)));

// builder.SetException(ex); OR if (this.combinedTokens != null) this.combinedTokens.Dispose(); _promiseOfValueOrEnd.SetException(ex);
BoundStatement callSetException = GenerateSetExceptionCall(exceptionLocal);

return new BoundCatchBlock(
F.Syntax,
ImmutableArray.Create(exceptionLocal),
F.Local(exceptionLocal),
exceptionLocal.Type,
exceptionFilterPrologueOpt: null,
exceptionFilterOpt: null,
body: F.Block(
assignFinishedState, // _state = finishedState;
GenerateHoistedLocalsCleanup(hoistedLocals),
callSetException, // builder.SetException(ex); OR _promiseOfValueOrEnd.SetException(ex);
GenerateReturn(false)), // return;
isSynthesizedAsyncCatchAll: true);
}

protected BoundStatement GenerateHoistedLocalsCleanup(ImmutableArray<StateMachineFieldSymbol> hoistedLocals)
protected virtual BoundStatement GenerateHoistedLocalsCleanupForExit(ImmutableArray<StateMachineFieldSymbol> rootHoistedLocals)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 19, 2024 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

GenerateHoistedLocalsCleanupForExit

Consider removing "HoistedLocals" from the name to completely avoid any confusion that the clean up is limited to the locals in the rootHoistedLocals parameter.

        protected virtual BoundStatement GenerateCleanupForExit(ImmutableArray<StateMachineFieldSymbol> rootHoistedLocals)
``` #Resolved

{
var builder = ArrayBuilder<BoundStatement>.GetInstance();

// Cleanup all hoisted local variables
// so that they can be collected by GC if needed
foreach (var hoistedLocal in hoistedLocals)
foreach (var hoistedLocal in rootHoistedLocals)
{
var useSiteInfo = new CompoundUseSiteInfo<AssemblySymbol>(F.Diagnostics, F.Compilation.Assembly);
var isManagedType = hoistedLocal.Type.IsManagedType(ref useSiteInfo);
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncRewriter.AsyncIteratorRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,7 @@ protected override void GenerateMoveNext(SynthesizedImplementationMethod moveNex
instanceIdField: instanceIdField,
hoistedVariables: hoistedVariables,
nonReusableLocalProxies: nonReusableLocalProxies,
nonReusableFieldsForCleanup: nonReusableFieldsForCleanup,
synthesizedLocalOrdinals: synthesizedLocalOrdinals,
stateMachineStateDebugInfoBuilder,
slotAllocatorOpt: slotAllocatorOpt,
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ protected virtual void GenerateMoveNext(SynthesizedImplementationMethod moveNext
instanceIdField: instanceIdField,
hoistedVariables: hoistedVariables,
nonReusableLocalProxies: nonReusableLocalProxies,
nonReusableFieldsForCleanup: nonReusableFieldsForCleanup,
synthesizedLocalOrdinals: synthesizedLocalOrdinals,
stateMachineStateDebugInfoBuilder,
slotAllocatorOpt: slotAllocatorOpt,
Expand Down
10 changes: 8 additions & 2 deletions ...mpilers/CSharp/Portable/Lowering/IteratorRewriter/IteratorMethodToStateMachineRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,13 @@ internal IteratorMethodToStateMachineRewriter(
FieldSymbol? instanceIdField,
IReadOnlySet<Symbol> hoistedVariables,
IReadOnlyDictionary<Symbol, CapturedSymbolReplacement> nonReusableLocalProxies,
ImmutableArray<FieldSymbol> nonReusableFieldsForCleanup,
SynthesizedLocalOrdinalsDispenser synthesizedLocalOrdinals,
ArrayBuilder<StateMachineStateDebugInfo> stateMachineStateDebugInfoBuilder,
VariableSlotAllocator slotAllocatorOpt,
int nextFreeHoistedLocalSlot,
BindingDiagnosticBag diagnostics)
: base(F, originalMethod, state, instanceIdField, hoistedVariables, nonReusableLocalProxies, synthesizedLocalOrdinals, stateMachineStateDebugInfoBuilder, slotAllocatorOpt, nextFreeHoistedLocalSlot, diagnostics)
: base(F, originalMethod, state, instanceIdField, hoistedVariables, nonReusableLocalProxies, nonReusableFieldsForCleanup, synthesizedLocalOrdinals, stateMachineStateDebugInfoBuilder, slotAllocatorOpt, nextFreeHoistedLocalSlot, diagnostics)
{
_current = current;

Expand Down Expand Up @@ -160,7 +161,11 @@ internal void GenerateMoveNextAndDispose(BoundStatement body, SynthesizedImpleme
if (rootFrame.knownStates == null)
{
// nothing to finalize
F.CloseMethod(F.Return());
var disposeBody = F.Block(
GenerateAllHoistedLocalsCleanup(),
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

GenerateAllHoistedLocalsCleanup(),

Are there tests observing effect of this on IL? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see the diff between commits 1 and 2 for VerifyIL differences.
This change affects all iterator scenarios without try/finally (such as AddVariableCleanup_StringLocal), whereas the change below affects scenarios with try/finally (such as AddVariableCleanup_NestedStringLocal_InTryFinally)

F.Return());

F.CloseMethod(disposeBody);
}
else
{
Expand All @@ -171,6 +176,7 @@ internal void GenerateMoveNextAndDispose(BoundStatement body, SynthesizedImpleme
ImmutableArray.Create<LocalSymbol>(stateLocal),
F.Assignment(F.Local(stateLocal), F.Field(F.This(), stateField)),
EmitFinallyFrame(rootFrame, state),
GenerateAllHoistedLocalsCleanup(),
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

GenerateAllHoistedLocalsCleanup(),

Are there tests observing effect of this on IL? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. See AddVariableCleanup_NestedStringLocal_InTryFinally

F.Return());

F.CloseMethod(disposeBody);
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Lowering/IteratorRewriter/IteratorRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ private void GenerateMoveNextAndDispose(
instanceIdField,
hoistedVariables,
nonReusableLocalProxies,
nonReusableFieldsForCleanup,
synthesizedLocalOrdinals,
stateMachineStateDebugInfoBuilder,
slotAllocatorOpt,
Expand Down
Loading