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

Load C++/CLI DLLs built against .NET 7.0+ in default ALC #66486

Merged
merged 6 commits into from
Mar 18, 2022

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Mar 11, 2022

Make ijwhost load C++/CLI components into default ALC

  • Components targeting .NET 7.0+ (so using the updated ijwhost) will load in the default ALC
  • Older components (using older ijwhost) will keep their existing behaviour of loading into an isolated context.
  • No configuration to go back to using the isolated context in this change. We have the option of adding a check for a runtime config property that will allow each component to specify whether or not it should be loaded in the default ALC.
    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.

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:

  1. Always load into default ALC (current change)
    • C++/CLI library built using an older ijwhost would also be loaded in the default ALC when loaded in 7.0+ without a rebuild/retarget
      • For pre-7.0, they would have to have set RollForward to Major or LatestMajor (non-default values)
    • No way to override on a per-component basis
  2. Overload the meaning / do some re-interpretation of the parameters (two IntPtrs) currently passed to InMemoryAssemblyLoader
    • For example, something like IntPtr.Zero for one (currently an invalid value) indicating that the other IntPtr should be interpreted as some struct with the info for what to load and in what ALC
    • Would allow C++/CLI libraries built with an older ijwhost to keep the old behaviour (isolated ALC)
    • Would allow overriding on a per-component basis
    • Feels weird/hacky
  3. Add load_assembly and make ijwhost go through it, using the void* reserved parameter
    • Would allow C++/CLI libraries built with an older ijwhost to keep the old behaviour (isolated ALC)
    • Would allow overriding on a per-component basis
    • Not too keen on immediately making a new API use the reserved/extensibility parameter

Thoughts?

@ghost
Copy link

ghost commented Mar 11, 2022

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

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 load_assembly API like @vitek-karas mentioned would be something like the below and fit all our component loads.

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:

  1. Always load into default ALC (current change)
    • C++/CLI library built using an older ijwhost would also be loaded in the default ALC when loaded in 7.0+ without a rebuild/retarget
      • For pre-7.0, they would have to have set RollForward to Major or LatestMajor (non-default values)
    • No way to override on a per-component basis
  2. Overload the meaning / do some re-interpretation of the parameters (two IntPtrs) currently passed to InMemoryAssemblyLoader
    • For example, something like IntPtr.Zero for one (currently an invalid value) indicating that the other IntPtr should be interpreted as some struct with the info for what to load and in what ALC
    • Would allow C++/CLI libraries built with an older ijwhost to keep the old behaviour (isolated ALC)
    • Would allow overriding on a per-component basis
    • Feels weird/hacky
  3. Add load_assembly and make ijwhost go through it, using the void* reserved parameter
    • Would allow C++/CLI libraries built with an older ijwhost to keep the old behaviour (isolated ALC)
    • Would allow overriding on a per-component basis
    • Not too keen on immediately making a new API use the reserved/extensibility parameter
Author: elinor-fung
Assignees: -
Labels:

area-Host

Milestone: -

@ghost ghost assigned elinor-fung Mar 11, 2022
@AaronRobinsonMSFT
Copy link
Member

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.

@vitek-karas
Copy link
Member

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.
But I do see the argument that almost all C++/CLI usage is migration from .NET Framework which is likely to expect the default ALC (or it doesn't care). So if others are OK with just 1, I'll be fine with it as well.

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 .runtimeconfig.json knob to go to isolated ALC as well.

While 3 looks sort of clean - I do agree that IJW is different enough that it might as well have its own codepath. The load_assembly should be a new "public" API, and polluting it with some internal IJW behavior doesn't feel right (we would not want anybody else to use the ability to load assembly from handle).

@elinor-fung
Copy link
Member Author

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.

My concern with that was adding/exposing another hostfxr delegate type for ijwhost to get and use. I agree it is not hard, but I guess I just don't like the idea of adding something like hdt_load_in_memory_assembly_in_load_context as another thing just for IJW.

@vitek-karas
Copy link
Member

I don't "like" it either, but it feels like the least bad solution.

Actually now thinking about it - do we need the hdt stuff anymore - since we now have get_function_pointer? I just realized that all of the IJW internal managed support is in Default ALC (CoreLib). Just like all the other hdt functions. Before we had get_function_pointer, the hdt enum was basically the only way to get to CoreLib/Default ALC. Now with get_function_pointer anybody can ask for anything from there. So we could just add a new managed entrypoint, and use get_function_pointer from ijwhost directly to get to it - no need to introduce a new hdt or anything like that. In fact the get_function_pointer is basically the only hdt we really need, everything else can be done through it...

Honestly this sounds too easy... I guess I'm missing something 😇

@elinor-fung
Copy link
Member Author

In fact the get_function_pointer is basically the only hdt we really need, everything else can be done through it...

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.

@vitek-karas
Copy link
Member

I bet there will be some crazy corner case/thing and it won't work ... but I definitely hope it's not the case.

@vitek-karas
Copy link
Member

This looks good.

Just trying to clarify if we're OK what we're getting with this:

  • All C++/CLI components built while targeting net7.0 (or higher) will load to Default load context.
  • Currently there's no way to disable this (go back to the previous behavior) - other than the hacky way of copying .NET 6 version of ijwhost which is definitely not supported.
  • Trying to use C++/CLI component which targets net7.0+ on .NET 6 or below will fail (I think this is OK, but I'm curious what happens if somebody tries to load such assembly via Assembly.LoadFrom (or similar) - technically it's unsupported and will probably fail due to CoreLib version mismatch).

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 - ijwhost doesn't have a good way to access .runtimeconfig.json (without including its own JSON parser)...
Well - if we added it as a runtime property we could access it via hostfxr_get_runtime_property_value without a need for JSON parser. It's not super clean (since for the case where the component ends up starting the runtime, this runtime property will make it all the way to managed code), but it avoids all kinds of other problems and doesn't require changes to anything in hostfxr/...
Since it's a runtime property, it could/should have a .NET-like name, so more descriptive, something like Internal.Runtime.InteropServices.InMemoryAssemblyLoader.LoadIntoDefaultLoadContext.

@elinor-fung
Copy link
Member Author

what happens if somebody tries to load such assembly via Assembly.LoadFrom (or similar) - technically it's unsupported and will probably fail due to CoreLib version mismatch

Yep, you get the version mismatch like if you try to load a regular managed assembly that depends on a higher version:

System.IO.FileNotFoundException: Could not load file or assembly 'System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.
File name: 'System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'

I'd still like for us to have the option to go back to the previous behavior

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.

Well - if we added it as a runtime property we could access it via hostfxr_get_runtime_property_value without a need for JSON parser.

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:

  • We have the System.Runtime.InteropServices.EnableCppCLIHostActivation property for trimming, so maybe something with similar terminology like System.Runtime.InteropServices.CppCLI.LoadIntoDefaultContext
  • Would we want the same/different property names for C++/CLI and (eventually) COM components? Having different ones seems a bit specialized, but maybe it makes sense, since we want the defaults to be different.
  • If we add a config property that the host itself could set, would we want it to be the same/different? If we use different names, we probably want the name to indicate it is component-specific. If we use the same one, it could be reasoned as the host can set the default and each component can override for itself, but that would get weird when the component is the one starting the runtime)

@vitek-karas
Copy link
Member

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 ijwhost on its own.

Naming:

  • What about System.Runtime.InteropServices.LoadCppCLIIntoDefaultContext?
  • I do prefer to have different properties for comhost and for native hosting. Mostly because of the different defaults.
  • The setting is definitely per-component - regardless if it starts the runtime or not - the naming/presence of the property will be a bit tricky. Especially if we also go with the ability for the host to set the default (another property - different name). But I think that's overall a good solution. In terms of priority - we'll have to think about that - I can see both solutions. At least for the native host, it will have a last say for sure (can change runtime properties manually via the hostfxr APIs).

@elinor-fung
Copy link
Member Author

What about System.Runtime.InteropServices.LoadCppCLIIntoDefaultContext?

I was kind of hoping/trying to avoid the II just because it looks weird to me. =P
(Also, somewhat unfortunate that the existing one is CLI instead of Cli, since I believe our own guidance is to only capitalize the first character of acronyms that are three or more characters).

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 (LoadCppCLIComponentIntoDefaultContext) really helps. Or maybe tacking it onto the namespace ...LoadCppCLIIntoDefaultContext.Component vs ...LoadCppCLIIntoDefaultContext.Default ?

(Moving this out of draft, as we continue with config/naming for a separate change.)

@elinor-fung elinor-fung marked this pull request as ready for review March 14, 2022 22:34
@elinor-fung elinor-fung changed the title Load C++/CLI DLLs in default ALC Load C++/CLI DLLs built against .NET 7.0+ in default ALC Mar 14, 2022
@vitek-karas
Copy link
Member

We could always go with LoadThisCppCLIComponentIntoDefaultContext - it's a bit weird for a property name, but it is VERY descriptive and hard to confuse the meaning. Given that this should be used rarely and it's for advanced cases anyway... I would not mind it.

/// </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")]
Copy link
Member

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.

Copy link
Member Author

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:

[RequiresUnreferencedCode("Built-in COM support is not trim compatible", Url = "https://aka.ms/dotnet-illink/com")]
private static object GetClassFactoryForType(ComActivationContext cxt)

<!-- 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 😅

Copy link
Member Author

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):

  1. Take one of the trimming tests (TypeBuilderComDisabled)
  2. Explicitly set EnableTrimAnalyzer=true and SuppressTrimAnalysisWarnings=false
  3. Publish trimmed
  4. [no warnings]
  5. Set BuiltInComInteropSupport=true
  6. [no warnings]
  7. Set EnableCppCLIHostActivation=true
  8. [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.

Copy link
Member

@agocke agocke Mar 15, 2022

Choose a reason for hiding this comment

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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).

Copy link
Member Author

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)
Copy link
Member

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).

Copy link
Member Author

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?

Copy link
Member Author

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.

Copy link
Member Author

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


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.

Copy link
Member

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.

Copy link
Member Author

@elinor-fung elinor-fung Mar 15, 2022

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.

🐉 🐉 🐉

Copy link
Member

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.

Copy link
Member Author

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.

src/native/corehost/ijwhost/ijwhost.cpp Outdated Show resolved Hide resolved
@elinor-fung
Copy link
Member Author

I think the latest iteration is more reasonable trimming-wise:

  • ComponentActivator.GetFunctionPointer is rooted always
  • InMemoryAssemblyLoader.LoadInMemoryAssemblyInContext is rooted if the C++/CLI feature switch is enabled
  • For coreclr, ComponentActivator.GetFunctionPointer has a hook for when it is called and the feature is disabled, so that we can give a more useful error if the C++/CLI entry point is being requested
    • Not exactly nice - happy to hear better ways to do this

Trimming experience (console app):

  • Default settings
    • publish: no warnings
    • try to activate C++/CLI:
      System.NotSupportedException: C++/CLI activation has been disabled via a feature switch. See https://aka.ms/dotnet-illink/nativehost for more information.
      
  • EnableCppCLIHostActivation=true (and _EnableConsumingManagedCodeFromNativeHosting=true - will need to update sdk to do this)
    • publish: two warnings - one for native hosting, one for C++/CLI
      Trim analysis error IL2026: Internal.Runtime.InteropServices.ComponentActivator.GetFunctionPointer(IntPtr, IntPtr, IntPtr, IntPtr, IntPtr, IntPtr): Using member 'Internal.Runtime.InteropServices.ComponentActivator.InternalGetFunctionPointer(AssemblyLoadContext, 
      String, String, IntPtr)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Native hosting is not trim compatible and this warning will be seen if trimming is enabled. https://aka.ms/dotnet-illink/nativehost [...]
      Trim analysis error IL2026: Internal.Runtime.InteropServices.InMemoryAssemblyLoader.LoadInMemoryAssembly(IntPtr, IntPtr): Using member 'Internal.Runtime.InteropServices.InMemoryAssemblyLoader.LoadInMemoryAssemblyInContextImpl(IntPtr, IntPtr, AssemblyLoadContext)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. C++/CLI is not trim-compatible. https://aka.ms/dotnet-illink/nativehost [...]
      
    • try to activate C++/CLI: we try... very likely to fail due to dependencies that have been trimmed out

Copy link
Member

@vitek-karas vitek-karas left a 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.

elinor-fung and others added 2 commits March 17, 2022 09:09
Co-authored-by: Vitek Karas <10670590+vitek-karas@users.noreply.github.com>
Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants