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 lift the temp to live for the whole block instead of just the expression.

The block lifetime is enough - ref safety analysis already checks refs to rvalues cannot escape blocks.

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?

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?

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.

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.

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.

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.

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.

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.

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.

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.

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?

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.

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

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

jjonescz Nov 22, 2024

AlekseyTs Nov 22, 2024

jjonescz Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024

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

jjonescz Nov 22, 2024

AlekseyTs Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024

AlekseyTs Nov 22, 2024

AlekseyTs Nov 22, 2024

AlekseyTs Nov 22, 2024

AlekseyTs Nov 22, 2024

AlekseyTs Nov 22, 2024

AlekseyTs Nov 22, 2024

AlekseyTs Nov 22, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Nov 22, 2024

jjonescz commented Nov 21, 2024 •

edited

Loading

jjonescz Nov 22, 2024 •

edited

Loading

jjonescz Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited

Loading

AlekseyTs Nov 22, 2024 •

edited

Loading