-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Report RhpInitialDynamicInterfaceDispatch
reference in gfids
#103948
Conversation
Some subset of tests is failing with control flow guard enabled because we're not able to indirectly call `RhpInitialDynamicInterfaceDispatch`. It's not clear why since we address take this method in C++ source files as well, but we should be reporting this from our object file too and doing that fixes it.
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-nativeaot-outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
src/coreclr/jit/hwintrinsicxarch.cpp
Outdated
@@ -267,17 +267,17 @@ static CORINFO_InstructionSet lookupInstructionSet(const char* className) | |||
{ | |||
if (strncmp(className + 6, "128", 3) == 0) | |||
{ | |||
assert((className[9] == '\0') || (strcmp(className + 9, "`1") == 0)); | |||
//assert((className[9] == '\0') || (strcmp(className + 9, "`1") == 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unintended change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See commit title f71b073
Outerloop is on the floor again and I'm getting really tired being the janitor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub UI only showed me the first two commits in the list and thus I didn't see the title. Must have been some glitch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A fix for this up here: https://github.com/dotnet/runtime/pull/103995/files
However, the fact that the assert triggered looks to be a bug in NAOT, as there are types being marked as [Intrinsic]
which should not be marked [Intrinsic]
and which are not marked as such in the actual managed implementation.
@@ -44,6 +44,11 @@ public abstract partial class NodeFactory | |||
ObjectDataInterner dataInterner) | |||
{ | |||
_target = context.Target; | |||
|
|||
InitialInterfaceDispatchStub = _target.Architecture == TargetArchitecture.ARM | |||
? new InitialInterfaceDispatchStubNode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering whether the special ARM dispatch stub wrapper is still necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try to delete that in a different PR and see what the CI says, but I don't have hardware to test it on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing it in CI should be good enough if you would like to give it a try.
I think this was only needed for multi-.dll Windows arm32 support that we do not have anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (with workarounds reverted)
/ba-g only deleted old workarounds and #103215 needs this and I'm going to bed |
Some subset of tests is failing with control flow guard enabled because we're not able to indirectly call
RhpInitialDynamicInterfaceDispatch
. It's not clear why since we address take this method in C++ source files as well, but we should be reporting this from our object file too and doing that fixes it.Fixes #103798
Cc @dotnet/ilc-contrib