-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Extend static constructor interpreter to support vtable-like structures #114354
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
Extend static constructor interpreter to support vtable-like structures #114354
Conversation
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.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypePreinit.cs:1169
- [nitpick] Consider providing a more descriptive error message when TryInitialize fails to clarify the expected size or underlying reason for failure, which could aid in debugging.
if (token.IsGCPointer || popped.Value is not ByRefValueBase byrefVal || !byrefVal.TryInitialize(token.GetElementSize().AsInt))
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs:504
- Verify that the updated signature for RhIUnknown_AddRef aligns with the native implementation and that the delegate cast in ComWrappers.NativeAot.cs correctly reflects the intended call convention.
[RuntimeImport(RuntimeLibrary, "RhIUnknown_AddRef")]
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
|
|
||
| static IInspectableImpl() | ||
| { | ||
| *(IUnknownVftbl*)Unsafe.AsPointer(ref Vtbl) = IUnknownImpl.Vtbl; |
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.
Oh interesting, I thought the plan was not to support this. Is the advice for us to just do this in all our vtables then, rather than doing ComWrappers.GetIUnknownImpl for the IUnknown slots every time? Does this mean we can also do the same to copy the IInspectable vtable too as the first 6 slots, for CCW vtables that are inspectables?
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.
Yes, it looked allowable, hopefully Roslyn won't come up with too many ways to emit this in IL. The interpreter can be fragile, this has a happy path around stobj instruction.
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.
(Like I wrote in the issue, we can only make guarantees around IL sequences, not about C# source code. But since this is now a test in the repo, we'll get an advance notice if a break is incoming...)
|
Does it also resolve/help with #83043? |
jkotas
left a comment
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, assuming outer loop passed. Thank you!
| public delegate* unmanaged[MemberFunction]<void*, uint> Release; | ||
| } | ||
|
|
||
| public unsafe struct IInspectableVftbl |
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.
Can we also add a test for:
- A vtable that builds on top of
IInspectable, for good measure? So that we'd have both one that hasIUnknownas base (ie.IInspectable), and one that has another combined vtable as a base (ie.IInspectable). - Having derived vtables copy the base via a property (with a cast), not via the field. We'd like to keep the vtable fields internal and an implementation detail, if possible.
Basically these changes:
// Add this to IInspectableImpl
public static nint AbiToProjectionVftablePtr
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => (nint)Unsafe.AsPointer(ref Unsafe.AsRef(in Vtbl));
}
// Add these new types
internal unsafe struct IStringableVftbl
{
public delegate* unmanaged[MemberFunction]<void*, Guid*, void**, int> QueryInterface;
public delegate* unmanaged[MemberFunction]<void*, uint> AddRef;
public delegate* unmanaged[MemberFunction]<void*, uint> Release;
public delegate* unmanaged[MemberFunction]<void*, uint*, Guid**, int> GetIids;
public delegate* unmanaged[MemberFunction]<void*, nint*, int> GetRuntimeClassName;
public delegate* unmanaged[MemberFunction]<void*, int*, int> GetTrustLevel;
public new delegate* unmanaged[MemberFunction]<void*, nint*, HRESULT> ToString;
}
internal static unsafe class IStringableImpl
{
public static readonly IStringableVftbl Vtbl;
static IStringableImpl()
{
*(IInspectableVftbl*)Unsafe.AsPointer(ref Vtbl) = *(IInspectableVftbl*)IInspectableImpl.AbiToProjectionVftablePtr;
Vtbl.ToString = &ToString;
}
[UnmanagedCallersOnly(CallConvs = [typeof(CallConvMemberFunction)])]
private static HRESULT ToString(void* thisPtr, nint* value)
{
return 0;
}
}With these too, we should pretty much cover all scenarios we'd have in CsWinRT 🙂
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.
Having derived vtables copy the base via a property (with a cast), not via the field. We'd like to keep the vtable fields internal and an implementation detail, if possible.
This will have to be a separate PR, that very likely requires several new features.
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.
We'd like to keep the vtable fields internal and an implementation detail, if possible.
Is it really an implementation detail?
The public contract is that it points to a vtable with slots of specific pointer types. If we were to expose an API like this in .NET runtime, I do not expect that we would try to hide the public contract behind opaque nint. We would use the precise strong types instead.
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.
We can consider making them public if it's necessary to make the work in ILC simpler (in particular for #114355). The main reasons why we'd like to keep them public is that we generally try to minimize the public API surface we need, and also I could see us tweaking the exact signature of those methods over time. Eg. right now I'm not sure how exactly we'd want to encode parameter types in various cases (eg. opaque void* vs. specific synthesized interface pointers, or perhaps we'd like to use explicit extension types in the future, etc.).
Michal mentioned the changes that would be required here and it seems they'd at least in part overlap with those needed for #114355 anyway, that's why I was saying if we might get both, then we'd be happy to keep the vtable types internal and not expose them, as it'd give us more flexibility. But it's not a blocker, so yeah we can consider exposing them if it were absolutely needed to enable those two RVA folding scenarios. I can defer to Michal and then we can follow with what's required for ILC 🙂
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.
"If we were to expose an API like this in .NET runtime, I do not expect that we would try to hide the public contract behind opaque nint. We would use the precise strong types instead."
For context, one of the reasons we use nint for all these vtables is also to match what ComWrappers does. All of these vtables are ultimately assigned to ComInterfaceEntry values, which all just have a nint Vtable field. That's where our design in CsWinRT comes from here.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
No, this is not usable for that. This is a special happy path for structs that look like vtables only. |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/ba-g no need to wait for outerloop, win-x86 build is now good |
Extends preinitialization support to cover scenario in dotnet#114354 (comment). * We only supported `ldsflda` instruction if the field was on the same type as we're preinitializing. This lifts the restriction to support loading addresses of readonly fields on other types, even if they have a static constructor, provided the cctor can be interpreted. One complication here is that this exposes us to correctness issues when address of the same field is loaded multiple times. We need to make sure nested preinit results are interned. The newly added `TestByRefFieldAddressEquality` test tests things that go wrong if we don't do interning. * We didn't support calling methods on types that have a static constructor. This lifts the restriction if the static constructor is side effect free (approximated by "can be interpreted"). * We had limitations around `ldobj` that is used when dereferencing pointers. This lifts some of the restrictions and we can now `ldobj` many valuetypes, including the special vtable ones.
|
@MichalStrehovsky Thanks for making this work without a new API <3 |
Extends preinitialization support to cover scenario in #114354 (comment). * We only supported `ldsflda` instruction if the field was on the same type as we're preinitializing. This lifts the restriction to support loading addresses of readonly fields on other types, even if they have a static constructor, provided the cctor can be interpreted. One complication here is that this exposes us to correctness issues when address of the same field is loaded multiple times. We need to make sure nested preinit results are interned. The newly added `TestByRefFieldAddressEquality` test tests things that go wrong if we don't do interning. * We didn't support calling methods on types that have a static constructor. This lifts the restriction if the static constructor is side effect free (approximated by "can be interpreted"). * We had limitations around `ldobj` that is used when dereferencing pointers. This lifts some of the restrictions and we can now `ldobj` many valuetypes, including the special vtable ones.
This makes it possible to identify
structs that look like vtables (all fields aredelegate*and some extras) and represents them differently within the interpreter. The different representation allows the interpreter to capture relocs within the data structure (the normal byte array approach wouldn't be able to represent these since the numerical value of a function pointer is only known at runtime).I had to extend things so that we can allow byrefs to pass through as nint in places. Hopefully I found all the places where we would try to cast things to
ValueTypeValuewithin the interpreter and we now allownint-typed stack slots to also hold a byref.Fixes #114024
Resolves #114133
Cc @dotnet/ilc-contrib
@Sergio0694 please have a look at the test to see if this is the pattern that would work for CsWinRT.