Avoid reusing temps whose refs might be captured #76009

jjonescz · 2024-11-21T15:55:05Z

The idea is to have a "heuristic" to detect whether a method might capture the references passed to it. If such method call is detected, and a temporary reference is being emitted, we avoid freeing the temp so it lives during the whole method instead of just the expression.

The heuristic is implemented by CodeGenerator.MightEscapeTemporaryRefs. It runs on the lowered nodes (because it's the emit layer which decides to emit a temporary). It might have false positives (some calls like M(rvalue, out _) might be marked by the heuristic as dangerous but they are not), but it shouldn't have false negatives.

Without a heuristic, we would need to avoid reusing many more temps, which would be a regression (at least in IL size). But perhaps that's negligible and it would be better to avoid this complexity? I'm not sure.

AlekseyTs · 2024-11-22T14:45:02Z

src/Compilers/Core/Portable/CodeGen/LocalSlotManager.cs

+        // At the same time, these expression locals might be lifted to the containing block
+        // (to avoid reusing them if they might be captured by a ref struct).
+        // Then we use this map to keep track of the redeclared locals.
+        private Dictionary<LocalDefinition, LocalDefinition>? _redeclaredLocals;


private Dictionary<LocalDefinition, LocalDefinition>? _redeclaredLocals;

It is not obvious what do map to what here. Could you, please, elaborate and add a comment? #Resolved

It maps from the old LocalDefinition to the new LocalDefinition (which is redeclaring/shadowing the old one). Does that make sense?

I will try to explain it in the comment. Thanks.

AlekseyTs · 2024-11-22T15:24:59Z

src/Compilers/CSharp/Portable/Lowering/SyntheticBoundNodeFactory.cs

@@ -887,7 +887,7 @@ public BoundCall Call(BoundExpression? receiver, MethodSymbol method, ImmutableA
            return new BoundCall(
                Syntax, receiver, initialBindingReceiverIsSubjectToCloning: ThreeState.Unknown, method, args,
                argumentNamesOpt: default(ImmutableArray<String?>), argumentRefKindsOpt: refKinds, isDelegateCall: false, expanded: false, invokedAsExtensionMethod: false,
-                argsToParamsOpt: ImmutableArray<int>.Empty, defaultArguments: default(BitVector), resultKind: LookupResultKind.Viable, type: method.ReturnType)
+                argsToParamsOpt: default, defaultArguments: default(BitVector), resultKind: LookupResultKind.Viable, type: method.ReturnType)


argsToParamsOpt: default,

Why is this change necessary? #Resolved

Because the utility Binder.GetCorrespondingParameter that's used in this PR as part of CodeGenerator.MightEscapeTemporaryRefs would fail on an assert - it expects argsToParamsOpt to be either default or matching the number of arguments:

roslyn/src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs

Line 1447 in 14244f6

Debug.Assert(argumentOrdinal < argsToParamsOpt.Length);

Which seems like a reasonable invariant which this callsite was violating.

AlekseyTs · 2024-11-22T15:29:42Z

src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs

@@ -673,6 +673,8 @@ private void EmitBlock(BoundBlock block)
            {
                EmitUninstrumentedBlock(block);
            }
+
+            ReleaseBlockTemps();


ReleaseBlockTemps();

This feels fragile because we cannot assume that this block necessary maps to one syntactically present in source, which was used for "ref safety analysis already checks refs to rvalues cannot escape blocks". We still might not be done with that block. #Resolved

we cannot assume that this block necessary maps to one syntactically present in source

Why cannot we assume that? To me it seems we don't synthesize BoundBlocks.

Why cannot we assume that? To me it seems we don't synthesize BoundBlocks.

Because we shouldn't be assuming that. There is nothing wrong in introducing a bound block.

There is nothing wrong in introducing a bound block.

This comment suggests otherwise:

roslyn/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

Line 1030 in 14244f6

BoundBlock specify SCOPE (visibility) of a variable.

This comment suggests otherwise: ...

I am failing to guess what part of the comment you find relevant. Could you quote that part, etc.?

For example, SyntheticBoundNodeFactory has a bunch of helpers to synthesize blocks and they are used for one reason or the other. There is also a way to synthesize a block manually. So, even if you haven't found an example, it doesn't mean there isn't one already, or that one won't be introduced in the future.

Sorry, I was referring to this line: "BoundBlock specify SCOPE (visibility) of a variable" but that doesn't imply it's wrong to synthesize blocks, you're right.

Do you have suggestions how to best solve this?

I'm thinking if we can assume blocks are never lost during lowering, we could perhaps make sure synthesized blocks are marked as such (e.g., via WasCompilerGenerated; or maybe we could mark user-defined blocks instead) and then we could call ReleaseBlockTemps only for non-compiler-generated blocks.

Do you have suggestions how to best solve this?

I am leaning towards a simpler and a more conservative approach - simply dropping "might escape" temps from _expressionTemps as EmitAssignmentValue does, or not adding them to that list, whichever is more natural from the implementation perspective.

AlekseyTs · 2024-11-22T15:40:58Z

src/Compilers/Core/Portable/CodeGen/LocalSlotManager.cs

@@ -64,6 +64,23 @@ public override bool Equals(object? obj)
        // maps local identities to locals.
        private Dictionary<ILocalSymbolInternal, LocalDefinition>? _localMap;

+        // The lowered tree might define the same local symbol
+        // in multiple sequences that are part of one expression, for example:


in multiple sequences that are part of one expression, for example:

This doesn't sound right. I think we should fix this, a local symbol should belong to exactly one scope in the bound tree. I assume this is a pre-existing condition, i.e. it is not introduced by this change. If so, I would prefer the fix to go into a separate PR. #Resolved

Sounds good, I will look into that.

Just to confirm: I think this "invalid" tree shape is introduced somewhere in lowering simply by reusing the same node twice, e.g., something like _factory.Call(arguments: [node, node]), and that node happens to contain a BoundSequence which then causes the local to be declared twice in the tree. So are you saying we should never do that (reuse the same node) and if we do it somewhere, it's a bug (hopefully a rare one)?

In general, we can reuse bound nodes as long as that doesn't violate some invariants. For example, we reuse some primitive nodes: for constants, for local references (BoundLocal), etc. I consider "a local symbol used in a tree should belong to exactly one scope in the tree" as one of the invariants that is violated in the specific scenario.

AlekseyTs · 2024-11-22T15:47:06Z

src/Compilers/CSharp/Portable/CodeGen/CodeGenerator_RefSafety.cs

+
+        for (var arg = 0; arg < arguments.Length; arg++)
+        {
+            var parameter = Binder.GetCorrespondingParameter(


Binder.GetCorrespondingParameter(

I think post lowering phase all the parameters must be in the right order, no one is going to reorder them (lowering does that). Therefore, we probably do not need to use this helper. #Resolved

AlekseyTs · 2024-11-22T17:27:12Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

+                arguments,
+                method.Parameters,
+                call.ArgumentRefKindsOpt,
+                mightEscapeTemporaryRefs: MightEscapeTemporaryRefs(call, used: useKind != UseKind.Unused, receiverAddressKind: null));


mightEscapeTemporaryRefs: MightEscapeTemporaryRefs(call, used: useKind != UseKind.Unused, receiverAddressKind: null)

Instead of threading an extra parameter through many layers, consider simply truncating _expressionTemps after the EmitArguments call as EmitAssignmentValue does. #Resolved

AlekseyTs · 2024-11-22T17:28:23Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

@@ -1754,7 +1767,11 @@ private void EmitInstanceCallExpression(BoundCall call, UseKind useKind)
                        }
                    }

-                    emitArgumentsAndCallEpilogue(call, callKind, receiverUseKind);
+                    emitArgumentsAndCallEpilogue(call, callKind, receiverUseKind,


emitArgumentsAndCallEpilogue

Similar suggestion about truncating the list vs. passing a parameter. #Resolved

AlekseyTs · 2024-11-22T17:32:00Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

+                    expression.Arguments,
+                    constructor.Parameters,
+                    expression.ArgumentRefKindsOpt,
+                    mightEscapeTemporaryRefs: MightEscapeTemporaryRefs(expression, used));


mightEscapeTemporaryRefs: MightEscapeTemporaryRefs(expression, used)

Similar suggestion about truncating the list vs. passing a parameter. #Resolved

AlekseyTs · 2024-11-22T17:46:59Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs

@@ -1640,13 +1639,11 @@ private void RewriteArgumentsForComCall(

                actualArguments[argIndex] = new BoundSequence(
                    argument.Syntax,
-                    locals: ImmutableArray<LocalSymbol>.Empty,
-                    sideEffects: ImmutableArray.Create<BoundExpression>(boundAssignmentToTemp),
+                    locals: [boundTemp.LocalSymbol],


locals: [boundTemp.LocalSymbol],

Strictly speaking this is wrong. It looks like we are going to pass a reference to a local that goes out of scope. I know, we probably have done things like that in other places and EmitSequenceAddress compensates for situations like that by not releasing the locals. But that is likely to lead to a worse IL. I suggest reverting the change. #Resolved

It looks like we are going to pass a reference to a local that goes out of scope.

True, but even without the change, the temp might live too short - see test RefTemp_Escapes_ComCall. Moving the temp into the sequence inside the call ensures the emit layer does not free the temp after it detects the call might escape temps. If the temp is declared outside the call, the emit layer does not see that it is a part of a "might escape" call and that its freeing should be avoided.

It turns out this change is what causes the problem with one local being declared twice in one lowered subtree. But I imagine this can be fixed in the callers of this method.

But that is likely to lead to a worse IL

The mentioned test verifies IL and at least in that scenario it does not look worse.

It turns out this change is what causes the locals being declared twice in one lowered tree. But I imagine this can be fixed in the callers of this method.

That did not seem as a good approach after all - the caller (VisitCompoundAssignmentOperator) expects all temps to be lifted up so the resulting node can be reused. Changing that would likely require some rewriter that would replace redeclared locals in the reused subtree. I'm going to investigate different approaches.

AlekseyTs · 2024-11-22T17:56:12Z

src/Compilers/CSharp/Portable/CodeGen/CodeGenerator_RefSafety.cs

+        ImmutableArray<ParameterSymbol> parameters,
+        ImmutableArray<BoundExpression> arguments,
+        ImmutableArray<int> argsToParamsOpt,
+        bool expanded)


The last two parameters should not be relevant post lowering. #Resolved

AlekseyTs · 2024-11-22T18:04:32Z

src/Compilers/CSharp/Portable/CodeGen/CodeGenerator_RefSafety.cs

+                {
+                    readonlyRefs++;
+                }
+            }


}

What are the scenarios when we can fail to get a parameter and why they are not interesting? #Resolved

AlekseyTs · 2024-11-22T18:47:37Z

src/Compilers/CSharp/Portable/CodeGen/CodeGenerator_RefSafety.cs

+            expanded: false);
+    }
+
+    private static bool MightEscapeTemporaryRefs(


MightEscapeTemporaryRefs

I will let Ref-Safety experts to make a call on completeness/correctness of the code in this method. However, it would be good to add comments about explaining the logic. #Resolved

AlekseyTs · 2024-11-22T18:49:13Z

Done with review pass (commit 1), tests are not looked at.

Avoid reusing temps whose refs might be captured

80aaae6

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 21, 2024

jjonescz marked this pull request as ready for review November 21, 2024 18:32

jjonescz requested a review from a team as a code owner November 21, 2024 18:32

AlekseyTs reviewed Nov 22, 2024

View reviewed changes

jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 26, 2024

jjonescz marked this pull request as draft November 27, 2024 11:03

jjonescz added 5 commits November 27, 2024 14:33

Revert some changes

4672a11

Simplify the heuristic

4dc8f82

Avoid reusing any local whose address has been taken

60dec1e

Update tests

1d6b786

Inline a function

6ccc2ad

jjonescz marked this pull request as ready for review November 28, 2024 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid reusing temps whose refs might be captured #76009

Avoid reusing temps whose refs might be captured #76009

jjonescz commented Nov 21, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

jjonescz Nov 22, 2024

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

jjonescz Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

jjonescz Nov 22, 2024

AlekseyTs Nov 22, 2024

jjonescz Nov 22, 2024

AlekseyTs Nov 22, 2024

jjonescz Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

jjonescz Nov 22, 2024

AlekseyTs Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

jjonescz Nov 26, 2024 •

edited

Loading

jjonescz Nov 26, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs commented Nov 22, 2024

Avoid reusing temps whose refs might be captured #76009

Are you sure you want to change the base?

Avoid reusing temps whose refs might be captured #76009

Conversation

jjonescz commented Nov 21, 2024 • edited Loading

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

jjonescz Nov 22, 2024

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

jjonescz Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

jjonescz Nov 22, 2024

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024

Choose a reason for hiding this comment

jjonescz Nov 22, 2024

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024

Choose a reason for hiding this comment

jjonescz Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

jjonescz Nov 22, 2024

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

jjonescz Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

jjonescz Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 22, 2024

jjonescz commented Nov 21, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

jjonescz Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

jjonescz Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

jjonescz Nov 26, 2024 •

edited

Loading

jjonescz Nov 26, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading

AlekseyTs Nov 22, 2024 •

edited by jjonescz

Loading