-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Load C++/CLI DLLs built against .NET 7.0+ in default ALC #66486
Conversation
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov Issue Details@vitek-karas @AaronRobinsonMSFT @agocke - I realized (re-discovered?) that the C++/CLI path is a bit more special than I thought. I had been thinking a new typedef int (CORECLR_DELEGATE_CALLTYPE *load_assembly_fn)(const char_t *assembly_path, void *load_context, void *reserved); However, unlike other component hosting, C++/CLI loads using a module handle for a module that has already been loaded into memory (via an internal, coreclr, windows-only API), so it doesn't fit quite so nicely. The changes in this PR just always load C++/CLI libraries into the default ALC. Some options:
|
I would go for (1) and if we need to have more control then we can sit down and design what we need for C++/CLI. The current usage indicates C++/CLI is used almost exclusively in .NET Framework to .NET 5 migration. Let's make the scenario as close to .NET Framework as possible and let the community let us know what is needed. |
I'm not fully convinced we should go all in (so option 1) - it's from one extreme to the other. I think it's OK to do this for a 7 preview, but for 7 GA I think we should go to some more backward compat friendly behavior. As for the technical solution - we could also introduce a new managed entry point which can control the ALC (just for IJW) - would mean to add it to hostpolicy and headers, but that's not hard. Since ijwhost is effectively versioned with the runtime (SDK will use the 7.0 ijwhost only for projects targeting net7), expecting a new entry point should not cause any issues. We would keep the old one for components targeting downlevel runtimes. The existing one would not change its behavior (backward compat), the ijwhost then can pick its default with the new one - we should go with ALC.Default by default, but I would like to have the ability to add a While 3 looks sort of clean - I do agree that IJW is different enough that it might as well have its own codepath. The |
My concern with that was adding/exposing another hostfxr delegate type for |
I don't "like" it either, but it feels like the least bad solution. Actually now thinking about it - do we need the Honestly this sounds too easy... I guess I'm missing something 😇 |
Oooohh, I'm not immediately seeing problems here either. Would be very happy with this. |
I bet there will be some crazy corner case/thing and it won't work ... but I definitely hope it's not the case. |
5bf8c1b
to
61ec887
Compare
This looks good. Just trying to clarify if we're OK what we're getting with this:
I'd still like for us to have the option to go back to the previous behavior - but it's going to be a bit tricky - |
Yep, you get the version mismatch like if you try to load a regular managed assembly that depends on a higher version:
In principle, I want this. With the main usage of .NET Framework migration for C++/CLI and the overwhelming feedback being 'stop putting us in an isolated context' though, I'm kind of inclined to say we should just switch this right now, especially while we are in earlier previews. And if we get feedback otherwise or if/when we allow COM components to choose to be loaded in the default ALC (whichever comes first), we can have ijwhost respect a configuration for loading in the isolated context.
This is what I was thinking we would do. We could probably also do some weirdness to make it not propagate to managed, but that would be, well, weirdness. Naming:
|
I agree we can/should just change the behavior now - in preview. Hopefully we'll be able to add the opt-out into .NET 7 as well, but if not, I don't think it's a big problem for Naming:
|
I was kind of hoping/trying to avoid the I think that is reasonable though. My only (slight) concern is that it doesn't sound component-specific - reading it, I would assume it means load all C++/CLI assemblies into the default context. I'm not sure how to make it clearer though - I don't think throwing 'component' in there ( (Moving this out of draft, as we continue with config/naming for a separate change.) |
We could always go with |
/// </summary> | ||
/// <param name="moduleHandle">The native module handle for the assembly.</param> | ||
/// <param name="assemblyPath">The path to the assembly (as a pointer to a UTF-16 C string).</param> | ||
[RequiresUnreferencedCode("C++/CLI is not trim-compatible", Url = "https://aka.ms/dotnet-illink/nativehost")] |
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'm pretty sure this will generate a warning for an empty console app... since it's rooted always.
The RUC should be left on the LoadInMemoryAssemblyInContextImpl
only. The warning that produces in dotnet/runtime build should be suppressed by pragmas (for the analyzer) and by the XML suppression for the dotnet/runtime trimming during build (see how this is done before the change).
When trimming an application, the IsSupported
is set to false and the trimmer will only see the throw
branch and it will remove the call to LoadInMemoeryAssemblyInContextImpl
, thus not producing any warning because of it.
So in trimmed app we're left with this method body having just throw
.
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.
Looks like it is 'Elinor does not understand trimming' time again...
I tried to follow what ComActivator.GetClassFactoryForType
does with RequiresUnreferencedCode
on the method and the unconditional rooting:
runtime/src/coreclr/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComActivator.cs
Lines 92 to 93 in 84de128
[RequiresUnreferencedCode("Built-in COM support is not trim compatible", Url = "https://aka.ms/dotnet-illink/com")] | |
private static object GetClassFactoryForType(ComActivationContext cxt) |
runtime/src/coreclr/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.Windows.xml
Lines 7 to 9 in 84de128
<!-- ComActivator is needed for native host operations --> | |
<type fullname="Internal.Runtime.InteropServices.ComActivator" > | |
<method name="GetClassFactoryForTypeInternal" /> |
What am I missing that makes the RUC work fine there? Is it from being non-
public
? Or UnmanagedCallersOnly
? If so, can we just change this to match, since it is only in SPCL (I don't know why it is public in the first place)?
I was expecting that one of the *.TrimmingTests
would basically be an empty console and fail for warnings. I guess we don't have that and just always set SuppressTrimAnalysisWarnings=true
with none of the tests overriding it? Seems like it might be a useful basic test to easily catch people like me 😅
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.
So in a continuation of not knowing what I'm doing, I tried (with the changes in this PR):
- Take one of the trimming tests (TypeBuilderComDisabled)
- Explicitly set
EnableTrimAnalyzer=true
andSuppressTrimAnalysisWarnings=false
- Publish trimmed
- [no warnings]
- Set
BuiltInComInteropSupport=true
- [no warnings]
- Set
EnableCppCLIHostActivation=true
- [no warnings]
I don't recall - did we intentionally make COM not warn?
If I set something else like StartupHookSupport=true
, it does warn, so I think I enabled the warnings correctly. And without my changes, setting EnableCppCLIHostActivation=true
does warn, so that is something I broke.
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.
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 was expecting that one of the *.TrimmingTests would basically be an empty console and fail for warnings. I guess we don't have that and just always set SuppressTrimAnalysisWarnings=true with none of the tests overriding it? Seems like it might be a useful basic test to easily catch people like me
This seems like a reasonable expectation. Not sure we have that, but if we don't, we should add it.
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.
Sorry - I obviously don't know linker well enough - even though I write the code involved in this. In short - methods which are kept via the descriptor XML will NOT produce warnings (intentional behavior in linker right now - I remember the reasons only vaguely - the comment says "Don't want to warn as it's intentional choice"... definitely not good enough description).
So the reason why this works for StartupHook for example is because the entry point (ProcessStartupHook
) is actually not marked as RUC (and can be rooted unconditionally), but it will call CallStartupHook
if the feature switch is enabled which is RUC annotated - so that will warn. So we don't need to use features switches in the descriptors, we can include the entry points unconditionally. This has two benefits (which I forgot about)
- If the app does end up calling the entry point - it will get a nice descriptive exception (unlike something like MissingMethodException which is not very helpful)
- It produces trim warning if the feature switch is enabled
If I remember correctly we removed this unconditional rooting for the native host entry point because there you only get an error code back anyway (so the rich exception is lost regardless) and Marek really wanted to remove the potentially unnecessary type/method/exception string.
I guess we could do the same for the rest - they report error codes in reality anyway.
So I would stick to unconditionally rooting the entry point (like before) - not annotate with RUC (it's cleaner, than relying on a weird linker behavior). Annotated the inner method. Suppress the warning via pragma/librarbuildsuppression.xml.
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 think the lack of warning for COM (and why my previous change removed the worningfor C++/CLI) is actually because we have the function marked with RequiresUnreferencedCode
? So there's no warning on those functions themselves for using another function marked with RUC and they would never be called explicitly from managed, so users never get a warning even with BuiltInComInteropSupport=true
.
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.
That might be true... in which case it's definitely broken. Yes - this looks broken - publishing an app with BuiltInComInteropSupport=true
should warn - I think... it doesn't. @LakshanF do you remember the details here?
I would expect to get a warning even if the app itself doesn't actually call any COM APIs - because enabling the feature switch also enables the app to host managed COM components (via the native entry points).
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 guess we could do the same for the rest - they report error codes in reality anyway.
Right now (without this PR), for if you activate C++/CLI in a trimmed managed app, you get
System.NotSupportedException: This API is not enabled in trimmed scenarios. see https://aka.ms/dotnet-illink/nativehost for more details
.
The actual message could be more useful / say something about C++/CLI being disabled, but it is a nicer error than just SEHException
.
I didn't try COM, but I expect/hope it would have System.NotSupportedException: Built-in COM has been disabled via a feature switch. See https://aka.ms/dotnet-illink/com for more information.
/// <param name="loadContext">Load context (currently must be IntPtr.Zero)</param> | ||
[UnmanagedCallersOnly] | ||
[RequiresUnreferencedCode("C++/CLI is not trim-compatible", Url = "https://aka.ms/dotnet-illink/nativehost")] | ||
internal static unsafe void LoadInMemoryAssemblyInContext(IntPtr moduleHandle, IntPtr assemblyPath, IntPtr loadContext) |
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 could go either with the same mechanism as with the LoadInMemoryAssembly
(remove the RUC on this method, suppress it inside).
Or since this is internal API we could root it conditionally based on the same feature switch the IsSupported
uses - in that case linker would remove this entire method in trimmed apps (better for size).
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 thought we had intentionally rooted the native hosting APIs unconditionally, so that users can actually get an error message, since the component doesn't have a say in trimming / the app might not 'know' it is loading a component via hosting APIs?
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, I guess we changed that based on #62903? But just for ComponentActivator and not COM or C++/CLI.
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.
So I'd also need to update the sdk so that if you set EnableCppCLIHostActivation=true
, it also sets _EnableConsumingManagedCodeFromNativeHosting=true
(or EnableNativeHostActivation
since it is being renamed) for an app to at least try to go through C++/CLI activation.
And then with the default settings (EnableCppCLIHostActivation=false
), even with LoadInMemoryAssemblyInContext
rooted unconditionally as I have it right now, users won't get a nice message. Unless I also root ComponentActivator.GetFunctionPointer
when System.Runtime.InteropServices.EnableCppCLIHostActivation=true
and do some sort of handling so that
Lines 117 to 118 in ba962fd
if (!IsSupported) | |
return HostFeatureDisabled; |
also enables support when C++/CLI activation is supported.
The non-nice message is:
Unhandled exception. System.Runtime.InteropServices.SEHException (0x80004005): External component has thrown an exception.
for trying to call a native entry point (which then calls into managed) in a C++/CLI library.
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.
Hmm - that's a good point - I didn't realize that by relying on the get_function_pointer
it breaks the existing assumptions around feature switches... I told you there are some dragons hidden here 😇 .
Is this because C++/CLI is the only hosting which can propagate full error messages?
I guess what you describe would be a solution - enable native hosting if CPP/CLI is enabled. I would probably do it fully - if you want C++/CLI, you also get full native hosting support - I would not try to do magic in the ComponentActivator. Trimming with C++/CLI activation enabled is a very bad idea anyway.
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 think COM also would get the full error message propagated when it is a managed app loading a managed COM component (didn't test it though).
I definitely don't want to do any magic in ComponentActivator
- my thought for potentially having to do something there would purely be for trying to get a nice error message when someone tries to use C++/CLI when it is disabled. Reread my comment above and realized that this is not at all what I actually wrote... I was trying to say that because ComponentActivator
entry points are no longer rooted, we don't get a nice error. So unless we always root them (or at least GetFunctionPointer) on Windows and somehow make it do something other than just return the error code, we will just have the SEHException
when activating C++/CLI when it is trimmed/disabled.
🐉 🐉 🐉
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 think I finally understand it - if the feature switches are off, we remove the main entry point and that leads to bad error experience. I think it would make sense to keep the GetFunctionPointer
always. I like it a lot as the main entry point for all native->managed (even though that's not what it does today), and as such we should keep it. On Windows eventually we could migrate all hosts to use that, in which case we could completely remove COM/C++/CLI entry points if the feature switches are off, since we would only need the GetFunctionPointer
to fail. That would actually be an overall size win. On non-Windows this will be a very small size regression, but I think it's worth it.
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 definitely agree that GetFunctionPointer
is nice as the main entry point. In order to have the nicer error experience (like the managed exception with an informative message), we would probably still need it to have some hook to do something for the COM and C++/CLI cases other than just return an error code if the native hosting feature switch is off.
We could also only keep it for Windows, since those are the provided host scenarios we currently have and only do it for non-Windows when we need to - not sure if it is worth the split/discrepancy .
I'll try always keeping GetFunctionPointer
and working out a decent error experience.
...oreclr/System.Private.CoreLib/src/Internal/Runtime/InteropServices/InMemoryAssemblyLoader.cs
Show resolved
Hide resolved
57fd520
to
73d6bd6
Compare
I think the latest iteration is more reasonable trimming-wise:
Trimming experience (console app):
|
...lr/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.CoreCLR.cs
Show resolved
Hide resolved
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.
Aside from the missing try/catch this looks really good.
I like the experience (as you describe it) with warnings and runtime exceptions.
The mechanism for C++/CLI is a little bit hacky, but I don't mind it too much either. I think it's reasonable.
src/coreclr/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.Windows.xml
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.cs
Outdated
Show resolved
Hide resolved
...lr/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComponentActivator.CoreCLR.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Vitek Karas <10670590+vitek-karas@users.noreply.github.com> Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Make
ijwhost
load C++/CLI components into default ALCijwhost
) will load in the default ALCijwhost
) will keep their existing behaviour of loading into an isolated context.option
Original context/description
@vitek-karas @AaronRobinsonMSFT @agocke - I realized (re-discovered?) that the C++/CLI path is a bit more special than I thought. I had been thinking a new
load_assembly
API like @vitek-karas mentioned would be something like the below and fit all our component loads.However, unlike other component hosting, C++/CLI loads using a module handle for a module that has already been loaded into memory (via an internal, coreclr, windows-only API), so it doesn't fit quite so nicely.
The changes in this PR just always load C++/CLI libraries into the default ALC.
Some options:
ijwhost
would also be loaded in the default ALC when loaded in 7.0+ without a rebuild/retargetRollForward
toMajor
orLatestMajor
(non-default values)IntPtr
s) currently passed toInMemoryAssemblyLoader
IntPtr.Zero
for one (currently an invalid value) indicating that the otherIntPtr
should be interpreted as some struct with the info for what to load and in what ALCijwhost
to keep the old behaviour (isolated ALC)load_assembly
and makeijwhost
go through it, using thevoid* reserved
parameterijwhost
to keep the old behaviour (isolated ALC)Thoughts?