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.

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

Fix bug in 'this' optimization for classes #21510

Merged
merged 1 commit into from Aug 15, 2017

Conversation

agocke
Copy link
Member

@agocke agocke commented Aug 15, 2017

In the introduction of the new 'this' optimization routine,
one of the things which was changed was to treat 'this' more
like a formal parameter of the method, as opposed to a variable
living in an implicit, higher scope. This has some advantages
in simplicity for analysis, but created a problem when it came
to proxies. The current analysis builds the proxy list by walking
the tree and finding all captured variables and adding them
to the proxy dictionary keyed by the original variable symbol.
For instance, if a local variable is captured to a field, during
rewriting it will be added to the proxy list as (original symbol,
hoisted field). Since most symbols are only ever captured to a
single replacement field, this usually works fine -- all proxies
can exist side-by-side in the proxy list since there is no intersection.

However, this is not true for captured environment pointers. When
a new environment is introduced, a local will be created to point
to that environment. That local may itself be captured by nested
variables, creating a linked list from nested scopes to parent scope.
Most notably, multiple nested environments may capture the same
environment pointer in different hoisted fields. This means that
the proxies dictionary cannot hold all mappings at once, since the
mapping for a given captured environment pointer will depend on the
current scope.

The following code exhibits this problem

using System;
class C
{
    int _x;
    void M()
    {
        {
            int y = 0;
            Func<int> f1 = () => _x + y; // Hoists 'this' to field
        }
        {
            int y = 0;
            Func<int> f2 = () => _x + y; // Hoists 'this' to different field
        }
    }
}

The current code actually accounts for this already by adding a
captured environment pointer to the proxy list on a nested scope's
introduction, and removing it upon leaving that scope.

By changing 'this' to be treated like a formal parameter, I
circumvented this logic, introducing a bug. When two scopes tried
to capture the 'this' pointer, the compiler crashed due to trying
to add two mappings to the same key.

This change fixes this problem by treating the 'this' parameter
like an environment pointer for the purposes of capturing and
hoisting. It's possible that we want to treat it like a formal
parameter, but if so it's probably better to treat all captured
environment pointers the same way and introduce a scope-aware
notion of proxies, rather than having a global dictionary.

Fixes #21506

In the introduction of the new 'this' optimization routine,
one of the things which was changed was to treat 'this' more
like a formal parameter of the method, as opposed to a variable
living in an implicit, higher scope. This has some advantages
in simplicity for analysis, but created a problem when it came
to proxies. The current analysis builds the proxy list by walking
the tree and finding all captured variables and adding them
to the proxy dictionary keyed by the original variable symbol.
For instance, if a local variable is captured to a field, during
rewriting it will be added to the proxy list as (original symbol,
hoisted field). Since most symbols are only ever captured to a
single replacement field, this usually works fine -- all proxies
can exist side-by-side in the proxy list since there is no intersection.

However, this is not true for captured environment pointers. When
a new environment is introduced, a local will be created to point
to that environment. That local may itself be captured by nested
variables, creating a linked list from nested scopes to parent scope.
Most notably, *multiple* nested environments may capture the *same*
environment pointer in *different* hoisted fields. This means that
the proxies dictionary cannot hold all mappings at once, since the
mapping for a given captured environment pointer will depend on the
current scope.

The current code actually accounts for this already by adding a
captured environment pointer to the proxy list on a nested scope's
introduction, and removing it upon leaving that scope.

By changing 'this' to be treated like a formal parameter, I
circumvented this logic, introducing a bug. When two scopes tried
to capture the 'this' pointer, the compiler crashed due to trying
to add two mappings to the same key.

This change fixes this problem by treating the 'this' parameter
like an environment pointer for the purposes of capturing and
hoisting. It's possible that we want to treat it like a formal
parameter, but if so it's probably better to treat all captured
environment pointers the same way and introduce a scope-aware
notion of proxies, rather than having a global dictionary.
@agocke
Copy link
Member Author

agocke commented Aug 15, 2017

cc @tmat

Copy link
Contributor

@khyperia khyperia left a comment

Choose a reason for hiding this comment

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

LGTM. I think that in-person discussion on "the philosophy of how capturing works" (😛) would be good to do soon.

@agocke agocke requested a review from a team August 15, 2017 18:00
@agocke
Copy link
Member Author

agocke commented Aug 15, 2017

ping @jaredpar @VSadov for review

This is blocking insertion, so it needs to go in ASAP.

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM

@agocke agocke merged commit 5d6f76f into dotnet:master Aug 15, 2017
@agocke agocke deleted the fix-this-duplication-bug branch August 15, 2017 20:11
333fred added a commit to 333fred/roslyn that referenced this pull request Aug 17, 2017
…statement-refactor

* dotnet/features/ioperation: (71 commits)
  Rename parameters.
  renamed methods.
  Rename method.
  Move integration test machines to 15.3 RTM (dotnet#21535)
  Update comment
  Fixed unintentional capitalization change, corrected grammar.
  Do a better job with leading trivia on types when moving types to a new file.
  Remove Release subdir from template output path (dotnet#21482)
  Fix bug in 'this' optimization for classes (dotnet#21510)
  Add test.
  Improve find-refs behavior when the user invokes it with a symbol selected.
  Fix regression in VB attribute classification.
  Fixed unintentional capitalization change.
  fix merge issues
  OSS sign debugger binaries. (dotnet#21468)
  More test fixes.
  Fixed misgenerated tests.
  Use default tuple fields in conversion since fields from inferred names are marked not usable in C#7
  Add note that while we could implement dynamic invocations, we chose not to
  Refactor comments in local function binder
  ...
333fred added a commit to 333fred/roslyn that referenced this pull request Aug 17, 2017
…-literal-text

* dotnet/features/ioperation: (77 commits)
  Refactor variable names.
  Rename parameters.
  renamed methods.
  Rename method.
  Move integration test machines to 15.3 RTM (dotnet#21535)
  Update comment
  Fixed unintentional capitalization change, corrected grammar.
  Do a better job with leading trivia on types when moving types to a new file.
  Fixed unintentional capitalization change.
  Remove Release subdir from template output path (dotnet#21482)
  Fix bug in 'this' optimization for classes (dotnet#21510)
  Add test.
  Improve find-refs behavior when the user invokes it with a symbol selected.
  Fix regression in VB attribute classification.
  Fixed unintentional capitalization change.
  fix merge issues
  OSS sign debugger binaries. (dotnet#21468)
  More test fixes.
  Fixed misgenerated tests.
  Use default tuple fields in conversion since fields from inferred names are marked not usable in C#7
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler crash in LambdaRewriter
4 participants