Prevent accidental passing by reference of reduced expressions when it was not valid originally. #24957

VSadov · 2018-02-20T22:41:38Z

Customer scenario

Customer writes some code where compiler is able to reduce complex expressions into simpler ones. That makes the resulting code smaller and possibly faster.
However if while doing so a reduction replaced an expression that could not be passed by reference - like (x + 0) into one that can - like x, the code may subtly change its semantics by passing the x by reference.

Generally C# is very explicit about passing variables by reference and (x + 1) would be an error in such contexts.
There are two situations where it is not an error

receiver of a struct/generic, method calls
ordinary byval argument that is matched to an in parameter of a method/operator/conversion

In the above cases the expressions in the source are required to only be readable, but also can pass the variable by reference if it is possible.
As a result we must be careful to not make passing by reference possible in the process of reduction.

Bugs this fixes

##24806

Workarounds, if any

No workaround. User must be very careful to not use the code as described or not take dependency on "no-aliasing" behavior.

Risk

Ultimately this is a bit of a corner case. Accidental aliasing is relatively hard to achieve and hard to observe. It is unlikely to be breaking in the majority of cases.

We are fixing this to have a rational specified behavior that we can comfortably preserve in the future.

Performance impact

Low perf impact.
The new checks are relatively cheap.
A new node will be allocated in very rare cases and is typically just a node for a node swap. (something needs to be reduced in order for the new node even be needed) - the bound tree cannot get bigger because of this.

Is this a regression from a previous update?

N/A.

Root cause analysis

The problem we are fixing is not new. In some cases the bug was present for more than a decade.
We are adding more contexts now where the bug is observable and therefore we are fixing it here.

How was the bug found?

Customer reported. More scenarios from internal testing.

Test documentation updated?

N/A

VSadov · 2018-02-20T23:54:29Z

@dotnet/roslyn-compiler - please review

gafter · 2018-02-21T22:55:47Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenStructsAndEnum.cs

-  IL_002e:  callvirt   ""bool object.Equals(object)""
-  IL_0033:  call       ""void System.Console.WriteLine(bool)""
-  IL_0038:  ret
+  IL_0010:  ldloc.0


ldloc [](start = 12, length = 5)

The only change appears to be the addition of these two instructions, which have no effect. Why did this happen? #Resolved

The receiver of Equals is an RValue that is lowered into passable by reference sequence. As a result we force a copy now. It is correct but a bit redundant. The JIT will understand this store/load as a NOP, so it is not a big concern. It is very specific to this particular pattern.

It can be rectified, but I did not feel like it is important to be a part of this fix.

In reply to: 169805013 [](ancestors = 169805013)

I've added a small peephole optimization to deal with this case. Thanks for nudging me to do it :-)

In reply to: 169812182 [](ancestors = 169812182,169805013)

gafter

gafter · 2018-02-22T00:19:49Z

Latest delta looks good, thanks!

VSadov · 2018-02-22T04:40:52Z

@dotnet-bot test windows_release_unit64_prtest please

jcouv · 2018-02-22T17:44:38Z

Should an entry be added to breaking changes documentation? #Resolved

VSadov · 2018-02-22T17:50:06Z

Yes. We should have done this last time when we (almost) fixed this bug.
I will add an entry in the doc.

In reply to: 367762051 [](ancestors = 367762051)

VSadov · 2018-02-22T18:14:10Z

The doc update PR is #24999

In reply to: 367763681 [](ancestors = 367763681,367762051)

jcouv · 2018-02-22T22:23:31Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs

+  IL_0026:  ret
+}
+");
+        }


Nit: double empty lines below #Resolved

jcouv · 2018-02-22T22:23:36Z

src/Compilers/CSharp/Portable/BoundTree/Expression.cs

@@ -144,4 +144,9 @@ internal partial class BoundStatementList
        protected override ImmutableArray<BoundNode> Children => 
            (this.Kind == BoundKind.StatementList || this.Kind == BoundKind.Scope) ? StaticCast<BoundNode>.From(this.Statements) : ImmutableArray<BoundNode>.Empty;
    }
+
+    internal partial class BoundPassByCopy


Is there some test(s) for GetOperations? #Resolved

The node only appears in the lowered tree, so I am not sure it is observable. I have added a case here mostly out of consistency/completeness.

In reply to: 170112647 [](ancestors = 170112647)

jcouv · 2018-02-22T22:26:56Z

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

@@ -186,6 +186,24 @@ private BoundExpression VisitExpressionImpl(BoundExpression node)
                    visited.Type.Equals(node.Type, TypeCompareKind.IgnoreDynamicAndTupleNames) ||
                    IsUnusedDeconstruction(node));

+            if (visited != null & visited != node)


Did you mean &&? #Resolved

Short circuiting is not needed here so having no branches could make it a bit faster.
But probably does not make a lot of difference though. I will change it to &&

In reply to: 170113468 [](ancestors = 170113468)

jcouv · 2018-02-22T23:14:08Z

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

+                    return true;
+
+                case BoundKind.DeconstructValuePlaceholder:
+                    // is this always a proxy for a temp local?


Here's the link to replace the comment: #24160 #Resolved

jcouv · 2018-02-22T23:30:15Z

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AwaitExpressionSpiller.cs

+            var result = RefKind.None;
+            if (!receiver.Type.IsReferenceType && LocalRewriter.CanBePassedByReference(receiver))
+            {
+                result = receiver.Type.IsReadOnly ?


Nit: odd identation. Could fit on one line. #Resolved

jcouv

Done with review pass (iteration 4).

jcouv

LGTM

VSadov · 2018-02-22T23:55:42Z

CC:@jaredpar for the ask mode approval

VSadov · 2018-02-23T00:00:28Z

apt-get install failed on ubuntu

VSadov · 2018-02-23T00:00:47Z

@dotnet-bot test ubuntu_16_mono_debug_prtest please

jaredpar · 2018-02-23T00:11:55Z

approved pending suites

VSadov · 2018-02-23T00:17:20Z

Ugh - now a failure about "CreateCompilationWithMscorlibAndSystemCore"

I guess the megachange with test helpers has been merged :-)
Need to rebase.

…t can be passed by reference is prevented from doing so by wrapping in a IdentityValue conversion node.

…only purpose of preventing passing by reference.

VSadov · 2018-02-23T01:59:12Z

@dotnet-bot test ubuntu_16_mono_debug_prtest please

VSadov · 2018-02-23T01:59:25Z

something with apt-get again ...

VSadov · 2018-02-23T06:10:58Z

@dotnet-bot test windows_release_vs-integration_prtest please

VSadov requested a review from a team as a code owner February 20, 2018 22:41

VSadov added the Area-Compilers label Feb 20, 2018

gafter self-requested a review February 21, 2018 22:10

gafter reviewed Feb 21, 2018

View reviewed changes

gafter approved these changes Feb 21, 2018

View reviewed changes

VSadov force-pushed the fix24806_2 branch from e37413f to dce358a Compare February 22, 2018 00:07

jcouv reviewed Feb 22, 2018

View reviewed changes

jcouv mentioned this pull request Feb 22, 2018

Allow ref Deconstruct extensions #24396

Closed

jcouv reviewed Feb 22, 2018

View reviewed changes

jcouv approved these changes Feb 22, 2018

View reviewed changes

jaredpar added the Approved to merge label Feb 23, 2018

VSadov added 4 commits February 22, 2018 16:51

Enforce that every case where an RValue was turned into something tha…

0a3a0bf

…t can be passed by reference is prevented from doing so by wrapping in a IdentityValue conversion node.

Added a special small wrapper node specifically for the explicit and …

a1ed359

…only purpose of preventing passing by reference.

Removed IdentityValue conversion since it is no longer needed.

cbf7770

PR feedback - prevent a redundant store/reload in a specific case.

4747493

VSadov added 2 commits February 22, 2018 16:51

PR feedback

c7d33e4

Fix after rebasing

88425cd

VSadov force-pushed the fix24806_2 branch from 9e333ca to 88425cd Compare February 23, 2018 01:32

VSadov merged commit fcceec9 into dotnet:dev15.7.x Feb 23, 2018

VSadov deleted the fix24806_2 branch February 23, 2018 18:44

VSadov mentioned this pull request Mar 23, 2018

in parameter aliasing for non-l-values #24806

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent accidental passing by reference of reduced expressions when it was not valid originally. #24957

Prevent accidental passing by reference of reduced expressions when it was not valid originally. #24957

VSadov commented Feb 20, 2018 •

edited

Loading

VSadov commented Feb 20, 2018

gafter Feb 21, 2018 •

edited by VSadov

Loading

VSadov Feb 21, 2018

VSadov Feb 22, 2018

gafter left a comment

gafter commented Feb 22, 2018

VSadov commented Feb 22, 2018

jcouv commented Feb 22, 2018 •

edited by VSadov

Loading

VSadov commented Feb 22, 2018

VSadov commented Feb 22, 2018

jcouv Feb 22, 2018 •

edited by VSadov

Loading

jcouv Feb 22, 2018 •

edited by VSadov

Loading

VSadov Feb 22, 2018

jcouv Feb 22, 2018 •

edited by VSadov

Loading

VSadov Feb 22, 2018

jcouv Feb 22, 2018 •

edited by VSadov

Loading

jcouv Feb 22, 2018 •

edited by VSadov

Loading

jcouv left a comment

jcouv left a comment

VSadov commented Feb 22, 2018

VSadov commented Feb 23, 2018

VSadov commented Feb 23, 2018

jaredpar commented Feb 23, 2018

VSadov commented Feb 23, 2018

VSadov commented Feb 23, 2018

VSadov commented Feb 23, 2018

VSadov commented Feb 23, 2018

Prevent accidental passing by reference of reduced expressions when it was not valid originally. #24957

Prevent accidental passing by reference of reduced expressions when it was not valid originally. #24957

Conversation

VSadov commented Feb 20, 2018 • edited Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

VSadov commented Feb 20, 2018

gafter Feb 21, 2018 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Feb 21, 2018

Choose a reason for hiding this comment

VSadov Feb 22, 2018

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

gafter commented Feb 22, 2018

VSadov commented Feb 22, 2018

jcouv commented Feb 22, 2018 • edited by VSadov Loading

VSadov commented Feb 22, 2018

VSadov commented Feb 22, 2018

jcouv Feb 22, 2018 • edited by VSadov Loading

Choose a reason for hiding this comment

jcouv Feb 22, 2018 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Feb 22, 2018

Choose a reason for hiding this comment

jcouv Feb 22, 2018 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Feb 22, 2018

Choose a reason for hiding this comment

jcouv Feb 22, 2018 • edited by VSadov Loading

Choose a reason for hiding this comment

jcouv Feb 22, 2018 • edited by VSadov Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

VSadov commented Feb 22, 2018

VSadov commented Feb 23, 2018

VSadov commented Feb 23, 2018

jaredpar commented Feb 23, 2018

VSadov commented Feb 23, 2018

VSadov commented Feb 23, 2018

VSadov commented Feb 23, 2018

VSadov commented Feb 23, 2018

VSadov commented Feb 20, 2018 •

edited

Loading

gafter Feb 21, 2018 •

edited by VSadov

Loading

jcouv commented Feb 22, 2018 •

edited by VSadov

Loading

jcouv Feb 22, 2018 •

edited by VSadov

Loading

jcouv Feb 22, 2018 •

edited by VSadov

Loading

jcouv Feb 22, 2018 •

edited by VSadov

Loading

jcouv Feb 22, 2018 •

edited by VSadov

Loading

jcouv Feb 22, 2018 •

edited by VSadov

Loading