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 clsHnd in impImportCall #93176

Closed
wants to merge 1 commit into from
Closed

Fix clsHnd in impImportCall #93176

wants to merge 1 commit into from

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Oct 7, 2023

Fixes #93174

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 7, 2023
@ghost ghost assigned EgorBo Oct 7, 2023
@ghost
Copy link

ghost commented Oct 7, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #93174

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo
Copy link
Member Author

EgorBo commented Oct 7, 2023

@MihuBot

@EgorBo
Copy link
Member Author

EgorBo commented Oct 7, 2023

@MihuBot

@EgorBo
Copy link
Member Author

EgorBo commented Oct 7, 2023

/azp list

@azure-pipelines

This comment was marked as resolved.

@EgorBo
Copy link
Member Author

EgorBo commented Oct 7, 2023

/azp run runtime-coreclr outerloop, runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

clsFlags = callInfo->classFlags;

// if clsHnd is an interface and method is static, try to get the actual implementation class
if ((clsFlags & CORINFO_FLG_INTERFACE) != 0 && (clsFlags & CORINFO_FLG_INTERFACE) != 0)
Copy link
Member

@jkotas jkotas Oct 8, 2023

Choose a reason for hiding this comment

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

There are other situations where getCallInfo can resolve the method to a method on a different class. Would it be better to return the class handle of the resolved method in CORINFO_CALL_INFO?

Copy link
Member

Choose a reason for hiding this comment

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

(Also, the condition has typo - it checks CORINFO_FLG_INTERFACE twice.)

Copy link
Member Author

Choose a reason for hiding this comment

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

@jkotas do you mean getCallInfo should patch input resolveToken->hClass? Because it seems that JIT mostly access hClass directly from resolveToken for methods in importer

Copy link
Member

Choose a reason for hiding this comment

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

I meant that there should be a new CORINFO_CLASS_HANDLE hClass; //target class handle field in CORINFO_CALL_INFO.

Patching CORINFO_RESOLVED_TOKEN would be a hack. CORINFO_RESOLVED_TOKEN is meant to be "in" argument for getCallInfo.

it seems that JIT mostly access hClass directly from resolveToken for methods in importer

I would expect that some places want the hClass of the method referenced by the IL and some places want the hClass of the method that the VM resolved the method to. I would not be surprised if there are subtle bugs today like the one you are trying to fix.

@ghost
Copy link

ghost commented Feb 4, 2024

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 6, 2024
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ISimdVector issue (GVM resolution doesn't report [Intrinsic]?)
2 participants