[ILVerify] Handling of assignment of interface to object - with CastingHelper by gregkalapos · Pull Request #3666 · dotnet/corert

gregkalapos · 2017-05-20T22:39:10Z

We had some branching issues on our fork, so I opened a new PR.
Original PR: #3649

One problem I have is this:
On CoreCLR we have lots of type forwardings. E.g. Application code references System.Runtime.dll, which defines System.Object. The application code always references System.Object from System.Runtime, but System.Runtime.dll forwards it to Sysmte.PricateCorLib.dll. When I do curType.Context.GetWellKnownType(WellKnownType.Object) it creates a [System.PriveCorLib]System.Object, but the otherType is [System.Runtime]System.Object (since it comes from the code referencing System.Runtime.dll). How should I handle this?

Additionally I also have a question:
Is it possible to start the unit tests (in this case these: https://github.com/dotnet/corert/blob/master/src/ILCompiler.TypeSystem/tests/CastingTests.cs ) directly in Visual Studio? The VS Test runner does not discover these tests, the error I get is this:

Dependency '"test-runtime": {
  "target": "project",
  "exclude": "compile"
}' has invalid version specification.

As a workaround I do it from the command line like this:
CoreRun.exe xunit.console.netcore.exe TypeSystem.Tests.dll -xml testResults.xml -notrait Benchmark=true -notrait category=nonnetcoreapp1.1tests -notrait category=OuterLoop -notrait category=failing -notrait category=nonwindowstests
I found this in a log from runtest.cmd, but it would be much better to do it in VS, so I can also debug the tests.

Thanks.

merge from dotnet/corert master

Merge dotnet/corert master

jkotas · 2017-05-21T02:38:45Z

curType.Context.GetWellKnownType(WellKnownType.Object) it creates a [System.PriveCorLib]System.Object, but the otherType is [System.Runtime]System.Object (since it comes from the code referencing System.Runtime.dll)

You need to run with a consistent set of assemblies:

Either reference assemblies (i.e. what gets passed as /r to C# compiler): There is no System.Private.CoreLib, System.Runtime defines System.Object.
Or implementation assemblies (i.e. what is used at runtime): System.Private.CoreLib defines System.Object, System.Runtime forwards System.Object to System.Private.CoreLib.

Mixing and matching of reference assemblies and implementation does not work. The problem that you have described occurs when you mix System.Runtime reference assembly with System.Private.CoreLib implementation assembly.

jkotas · 2017-05-21T02:44:00Z

src/Common/src/TypeSystem/Common/CastingHelper.cs

-                    curType = curType.BaseType;
+                    if (curType.IsInterface)
+                    {
+                        curType = curType.Context.GetWellKnownType(WellKnownType.Object);


The other implementation of casting logic implement this case as one-off check before this loop, for example: https://github.com/dotnet/corert/blob/master/src/Runtime.Base/src/System/Runtime/TypeCast.cs#L633

It is a bit more efficient because of you need this check only first time through the loop. It would be nice to do it same way here.

jkotas · 2017-05-21T02:45:49Z

src/ILVerify/src/ILImporter.StackValue.cs

                    return true;

-                return IsAssignable(src.Type, dst.Type);
+                return CastingHelper.CanCastTo(src.Type, dst.Type);


Change IsAssignable(TypeDesc src, TypeDesc dst) to call CastingHelper.CanCastTo as well while you are on it? At this line https://github.com/dotnet/corert/pull/3666/files#diff-ab14902486f8fc54c4fd3c1845fa8babR187

jkotas · 2017-05-21T03:04:55Z

it would be much better to do it in VS, so I can also debug the tests.

Agree. The CoreRT is building with old project.json-based .NET CLI that does not integrate well with current Visual Studio. For example, you might have noticed that the VS project that we use for debugging the CoreRT compiler is actually full .NET Framework project, not .NET Core project.

I have been working on upgrading the build system to recent .NET CLI that is a big step towards fixing this sort of problems. It will take me 1-2 weeks to get it done. The full .NET Framework helper projects should be gone once I am done.

BTW: If you run into this with adding ILVerify tests, feel free to create a vanilla .NET Core projects using dotnet new console, dotnet new xunit, that are not plugged in into the repo build for now. I will take care of plugging them into the repo build.

MichalStrehovsky · 2017-05-21T05:46:09Z

Is it possible to start the unit tests directly in Visual Studio

Yup, this works. Open the src\ILCompiler.TypeSystem\TypeSystem.sln solution and set the TypeSystem.Tests project as the startup project. Hit Debug -> Start debugging in the menu (run it as a normal command line app).

…Assignable(TypeDesc src, TypeDesc dst)

gregkalapos · 2017-05-21T11:50:12Z

You need to run with a consistent set of assemblies:

Now I understand. I will add this to the readme. I just naively passed every reference (direct or indirect) to ILVerify, maybe I won't be the only one doing this.

@jkotas your second comment answers a ton of questions I had. Thanks for that!

@MichalStrehovsky Great! Some tests fail when I start them that way (they are fine from the command line) I will dig deeper if I need to debug, but thanks for the info!

jkotas · 2017-05-21T13:51:22Z

src/Common/src/TypeSystem/Common/CastingHelper.cs

                    return thisType.CanCastTo(otherType.Instantiation[0]);
                }

+                if(curType.IsInterface)


Nit: Add space between if and (

jkotas

Thanks!

gregkalapos and others added 8 commits May 16, 2017 21:34

Merge pull request #3 from dotnet/master

1e3dbc2

merge from dotnet/corert master

Merge pull request #4 from dotnet/master

cc1e078

Merge dotnet/corert master

Merge branch 'master' of https://github.com/dotnet/corert

5b12d89

Merge branch 'master' of https://github.com/Dynatrace/corert

935cc05

Handling assignment of interface to object

0c19ce5

Handling interface->object assignment + test

96d5303

Undo unintended formatting change

ab7e224

Remove new line

a4eb6d3

dnfclas added the cla-already-signed label May 20, 2017

gregkalapos mentioned this pull request May 20, 2017

[ILVerify] Handling of assignment of interface to object #3649

Closed

jkotas reviewed May 21, 2017

View reviewed changes

Adapted logic in CanCastToClass and use CastingHelper.CanCastTo in Is…

cf0cc93

…Assignable(TypeDesc src, TypeDesc dst)

Removed unnecessary spaces

0846615

jkotas reviewed May 21, 2017

View reviewed changes

formatting

b20b267

jkotas approved these changes May 21, 2017

View reviewed changes

jkotas merged commit a80c986 into dotnet:master May 21, 2017

gregkalapos mentioned this pull request May 27, 2017

[ILVerify] Added first tests #3725

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ILVerify] Handling of assignment of interface to object - with CastingHelper#3666

[ILVerify] Handling of assignment of interface to object - with CastingHelper#3666
jkotas merged 11 commits intodotnet:masterfrom
Dynatrace:VerifyInterfaces

gregkalapos commented May 20, 2017 •

edited

Loading

Uh oh!

jkotas commented May 21, 2017 •

edited

Loading

Uh oh!

jkotas May 21, 2017

Uh oh!

jkotas May 21, 2017 •

edited

Loading

Uh oh!

jkotas commented May 21, 2017

Uh oh!

MichalStrehovsky commented May 21, 2017

Uh oh!

gregkalapos commented May 21, 2017

Uh oh!

jkotas May 21, 2017

Uh oh!

jkotas left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

gregkalapos commented May 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented May 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas May 21, 2017

Choose a reason for hiding this comment

Uh oh!

jkotas May 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented May 21, 2017

Uh oh!

MichalStrehovsky commented May 21, 2017

Uh oh!

gregkalapos commented May 21, 2017

Uh oh!

jkotas May 21, 2017

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gregkalapos commented May 20, 2017 •

edited

Loading

jkotas commented May 21, 2017 •

edited

Loading

jkotas May 21, 2017 •

edited

Loading