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

Remove legacy behavior around non-virtual interface calls #23032

Merged
merged 6 commits into from Mar 7, 2019

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Mar 5, 2019

For reasons that are not possible to look up in the CLR source history anymore, we treat non-virtual calls to instance interface methods as virtual.

I recently tweaked that behavior to remain backwards compatible with existing code (scoping it down to abstract methods only), but per request from the C# language team, we're going to do a conscious break here. It's likely only obfuscators will be broken.

While doing that, I noticed we don't have a good behavior defined for this situation in general (e.g. calls to abstract non-interface methods would NullRef, assert, and eventually throw a BadImageFormat). So I'm fixing that too to avoid the nullref/assert. I chose to do the throw in getCallInfo because in a way, this check is similar to the other check that getCallInfo is doing on line 5191 (throw for callvirt to a static method). This is subtly different to desktop behavior where we would nullref/assert/throw only when executing the method and not while compiling the callsite. But it's the callsite that has the wrong IL.

Fixes #22407.

@MichalStrehovsky MichalStrehovsky added this to the 3.0 milestone Mar 5, 2019
@@ -5415,6 +5409,14 @@ void CEEInfo::getCallInfo(

if (directCall)
{
// Direct calls to abstract methods are not allowed
if (pTargetMD->IsAbstract() &&
Copy link
Member

@jkotas jkotas Mar 5, 2019

Choose a reason for hiding this comment

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

This will fetch MethodAttrs that is not a free operation. We are fetching the MethodAttrs on all (or almost all) paths with this change, and some of the paths will fetch it more than once. It may be a good idea to fetch it once upfront.

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky commented Mar 5, 2019

Cc @sergiy-k

The test we inherited from the default interface method prototype branch is doing exactly the thing it shouldn't do (rely on the bad behavior) for unexplained reasons.
jkotas
jkotas approved these changes Mar 6, 2019
@jkotas jkotas merged commit 502de2a into dotnet:master Mar 7, 2019
70 checks passed
@MichalStrehovsky MichalStrehovsky deleted the abstractCall branch Mar 7, 2019
@MichalStrehovsky MichalStrehovsky added the breaking-change label May 1, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this issue Feb 18, 2022
…eclr#23032)

* Throw BadImageFormat for direct calls to abstract methods

* Remove legacy behavior around non-virtual interface calls

* Try fixing failing tests

The test we inherited from the default interface method prototype branch is doing exactly the thing it shouldn't do (rely on the bad behavior) for unexplained reasons.


Commit migrated from dotnet/coreclr@502de2a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change
Projects
None yet
2 participants