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 DispatchProxy AV when using generic type parameters on methods #19181

Merged
2 commits merged into from May 2, 2017

Conversation

@AustinWise
Copy link
Collaborator

AustinWise commented May 1, 2017

For return types, an assert is triggered in debug mode.

For parameters, an ExecutionEngineException is triggered at run time on x64 Windows when a proxied method has a generic-typed parameter and a value type is passed. The lack of an box instruction causes an AV in JIT_Stelem_Ref when it tries to treat the value type as an Object* .

I added some tests that trigger the problem too.

@stephentoub
Copy link
Member

stephentoub commented May 1, 2017

cc: @roncain, @AtsushiKan

@karelz karelz assigned AustinWise and ghost May 1, 2017
@ghost
Copy link

ghost commented May 2, 2017

Can you clarify which set of types causes the problem? (the term "generic type" is often used loosely.)
Because I'm wondering if Type.ContainsGenericParameters rather than Type.IsGenericParameter is the test you actually want.

AustinWise added 2 commits Apr 30, 2017
Currently the value types cause the CLR to die with an Acess Violation.
For return types, the assert was triggered in debug mode.

For parameters, an ExecutionEngineException was triggered at run time on
x64 Windows when passing a value type. The lack of an box instruction
causes an AV in JIT_Stelem_Ref when it tries to treat the value type as an
Object* .
@AustinWise AustinWise force-pushed the AustinWise:fixDispatchProxy branch from 59ab0c5 to d9ef187 May 2, 2017
@AustinWise AustinWise changed the title Fix DispatchProxy AV when using generic parameters. Fix DispatchProxy AV when using generic type parameters on methods May 2, 2017
@AustinWise
Copy link
Collaborator Author

AustinWise commented May 2, 2017

The problem is specifically with generic type parameters on methods. Type parameters on classes are not a problem, because only a fully instantiated generic type can be used with DispatchProxy. I have updated the commit messages, tests, and ticket title to reflect that.

That DispatchProxyGenerator has to create code that puts all the arguments to a method into an object[] to pass to the DispatachProxy.Invoke method. The problem is that if a method has a generic type parameter, the type parameter is used as an argument type, and at runtime that type is a value type, no box instruction is generated. Instead the value type is put into the array using stelem.ref as if it were a reference type.

Here is a minimal example to reproduce the problem:

public interface IThing
{
    void DoThing<T>(T genericArg);
}
public class Program : DispatchProxy
{
    static void Main(string[] args)
    {
        var p = DispatchProxy.Create<IThing, Program>();
        p.DoThing(42);
    }
    protected override object Invoke(MethodInfo targetMethod, object[] args)
    {
        return null;
    }
}

The problem is current code generator generates code that is roughly like this to put the a generic parameter into an object[] array:

ldc.i4.1
newarr [System.Runtime]System.Object
ldc.i4.0
ldarg.1
stelem.ref

The C# compiler always generates a box !!T before the stelem.ref, even if T is constrained to be a reference type.

@ghost
ghost approved these changes May 2, 2017
@ghost ghost merged commit 866296a into dotnet:master May 2, 2017
19 checks passed
19 checks passed
CROSS Check Build finished.
Details
Innerloop CentOS7.1 Debug x64 Build and Test Build finished.
Details
Innerloop CentOS7.1 Release x64 Build and Test Build finished.
Details
Innerloop OSX10.12 Debug x64 Build and Test Build finished.
Details
Innerloop OSX10.12 Release x64 Build and Test Build finished.
Details
Innerloop PortableLinux Debug x64 Build and Test Build finished.
Details
Innerloop PortableLinux Release x64 Build and Test Build finished.
Details
Innerloop Tizen armel Debug Cross Build Build finished.
Details
Innerloop Tizen armel Release Cross Build Build finished.
Details
Innerloop Ubuntu14.04 Debug x64 Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Release x64 Build and Test Build finished.
Details
Innerloop Ubuntu14.04 arm Release Cross Build Build finished.
Details
Innerloop Ubuntu16.04 arm Debug Cross Build Build finished.
Details
Innerloop Windows_NT Debug x86 Build and Test Build finished.
Details
Innerloop Windows_NT Release x64 Build and Test Build finished.
Details
Vertical netfx Build Build finished.
Details
Vertical uap Build Build finished.
Details
Vertical uapaot Build Build finished.
Details
Windows_NT Debug AllConfigurations Build Build finished.
Details
@AustinWise AustinWise deleted the AustinWise:fixDispatchProxy branch May 2, 2017
@karelz karelz modified the milestone: 2.0.0 May 5, 2017
PaulHigin added a commit to PaulHigin/corefx that referenced this pull request May 23, 2017
…otnet#19181)

* Add tests for DispatchProxy with generic type parameters on methods.

Currently the value types cause the CLR to die with an Acess Violation.

* Fix using a generic type parameter on a method with DispatchProxy.

For return types, the assert was triggered in debug mode.

For parameters, an ExecutionEngineException was triggered at run time on
x64 Windows when passing a value type. The lack of an box instruction
causes an AV in JIT_Stelem_Ref when it tries to treat the value type as an
Object* .
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.