-
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
Changes from 3 commits
dcddc2e
31774eb
f71b073
1758811
a08f889
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,11 @@ public 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 commentThe 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 commentThe 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 commentThe 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. |
||
: new AddressTakenExternSymbolNode("RhpInitialDynamicInterfaceDispatch"); | ||
|
||
_context = context; | ||
_compilationModuleGroup = compilationModuleGroup; | ||
_vtableSliceProvider = vtableSliceProvider; | ||
|
@@ -102,6 +107,11 @@ public NameMangler NameMangler | |
get; | ||
} | ||
|
||
public ISymbolNode InitialInterfaceDispatchStub | ||
{ | ||
get; | ||
} | ||
|
||
public PreinitializationManager PreinitializationManager | ||
{ | ||
get; | ||
|
This file was deleted.
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.