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

Add support for IntPtr conversion in the call arguments optimizer. #11752

Merged
merged 2 commits into from
Jun 4, 2016

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Jun 3, 2016

IntPtr conversion can stay in the bound tree past lowering in a case where a COM method is invoked in an Expression Tree lambda.

Fixes: #11751

IntPtr conversion can stay in the bound tree past lowering in a case where a COM method is invoked in an Expression Tree lambda.

Fixes: dotnet#11751
@VSadov
Copy link
Member Author

VSadov commented Jun 3, 2016

@dotnet/roslyn-compiler - please review

@VSadov
Copy link
Member Author

VSadov commented Jun 3, 2016

@MattGertz - for Update3 signoff

The bug results in crashes/watsons when upgrading to VS2015 and rebuilding.
(originally VSO: 222502)

COM + Expression Trees combination is not so uncommon as it might seem. Some mocking frameworks use expression trees extensively and when the tested scenario involves COM methods, it could result in crashing scenarios.

The combination results in IntPtr argument conversions staying in the bound tree past the Call lowering and argument optimizer was not expecting them.
Why COM? - regular invocations inside expression trees do not use named or optional arguments and therefore never need argument optimizing/reshuffling. However COM invocations have additional cases that require them to go through argument temp analysis making it possible to hit the codepath with IntPtr conversions still in the tree. Handling them is trivial, we were just not expecting them.

@VSadov VSadov added this to the 1.3 milestone Jun 3, 2016
@MattGertz
Copy link
Contributor

Approved pending CRs.

@cston
Copy link
Member

cston commented Jun 3, 2016

LGTM

case ConversionKind.ExplicitUserDefined:
case ConversionKind.ImplicitUserDefined:
// expression trees rewrite this later.
// it is a kind of user defined conversions on IntPtr and in some cases can fail
case ConversionKind.IntPtr:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we assert that this is happening only when we are producing an expression tree?

Copy link
Member Author

@VSadov VSadov Jun 4, 2016

Choose a reason for hiding this comment

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

If we are not in an expression tree, IntPtr will cause an assert in codegen anyways. And that one is more reliable since it is not conditional on optimizations being performed.
Also, this method does not have enough context to know we are in an expression tree and I do not want to change that just to get an assert earlier, sometimes.

@AlekseyTs
Copy link
Contributor

LGTM, but do consider adding an assert.

@VSadov
Copy link
Member Author

VSadov commented Jun 3, 2016

Will add an assert.
I also think the default case should be assert (not a crash) as well since there is a reasonable default behavior. It is completely safe to assume that unexpected conversions are not reorderable.

@VSadov
Copy link
Member Author

VSadov commented Jun 4, 2016

I've decided against an assert specifically for IntPtr conversions. There are many conversion kinds there, which can be seen only under certain conditions, so I do not want to single out just IntPtr one. In particular since this method is static and only needs to sort conversions into "pure" ones and others by kind.

I will replace the exception with an assert though, since there is a correct default behavior.

@VSadov VSadov merged commit 87a0126 into dotnet:stabilization Jun 4, 2016
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.

5 participants