-
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
Disable array support for the COM variant wrapper classes when built-in COM is disabled. #55756
Conversation
…in COM is disabled. Fixes dotnet#55600
// VariantWrappers cannot be stored in VARIANT's. | ||
if (CoreLibBinder::IsClass(pMT, CLASS__VARIANT_WRAPPER)) | ||
COMPlusThrow(kArgumentException, IDS_EE_COM_UNSUPPORTED_SIG); | ||
} | ||
#endif // FEATURE_COMINTEROP | ||
|
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.
This is a more surgical fix than I was thinking and thanks for doing this!
I assume the code under the other #FEATURE_COMINTEROP in this method will not cause a problem?
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.
The other ones won't cause problems.
We'll always have SafeHandle and CriticalHandle (which arrays of are always unsupported in interop).
And then the rest is based on RCW/CCW support having been used to create a wrapper, which is already blocked previously.
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.
@LakshanF - is there some sort of trimming test in libraries that could be added that would have caught this? (Those actually create a separate app and trim, right?)
It doesn't look like we have that many library tests that have a trim mode as well. But we should do more test runs with built-in COM disabled
|
These failures only show up when the linker has run, not when the feature flag has been flipped. |
The issue here wasn't having it disabled though - it was actually trimming. |
I was talking about having/adding something for this in the tests in the |
It might be worth adding static constructors on these wrapper types that makes them throw when built-in COM is disabled by the feature flag. Otherwise we can get into the following situation:
|
Then again, the changed behavior is the same as the non-Windows behavior, so I'd be ok saying that it's the expected behavior. |
Yes, you and @jkoritzinsky are right in that the I wonder though if it is possible to have a test mode in Windows where built-in COM is disabled without going the trimming route. In some sense, this is what is happening in non-Windows platforms where built-in COM is disabled and runtime 'knows' that and it doesn't ask for COM types inside the native code. Currently, Windows have both modes and runtime doesn't have a good way to distinguish that. |
Yes, this bug was caught by our TrimmingTests - see #55283 (comment). In our current infra, we are using the 6.0-preview4 SDK which doesn't disable COM by default. In that PR, we are upgrading to 6.0-preview5 which is disabling COM by default, and exposed the bug. So once #55283 is merged, we will have all our TrimmingTests running with COM disabled, like a normal user will be. |
Can we add a new TrimmingTest that tests the repro scenario in #55600? I believe that will ensure this doesn't regress in the future. It can be added to or |
If they have Yeah, I was also wondering if we should make the
I think that would basically be not setting
Ah, missed that it was found from a test failure - glad there is some testing that hit this. I agree we should add one specifically for Maybe this is a point-in-time thing, but would it be useful to have some way to set additional runtime config / trimmer args for all the trimmer tests? At least to be able to easily kick off a run (not necessarily checking in the specific values) with changes/defaults that aren't in the repo's SDK version yet. |
I've added a trimming test to validate this behavior. @eerhardt can you check that I configured it correctly? |
@@ -35,6 +35,9 @@ | |||
as this test will ensure exceptions are using Resource keys --> | |||
<ExtraTrimmerArgs>--feature System.Resources.UseSystemResourceKeys true</ExtraTrimmerArgs> | |||
</TestConsoleAppSourceFiles> | |||
<TestConsoleAppSourceFiles Include="TypeBuilderComDisabled.cs"> | |||
<ExtraTrimmerArgs>--feature System.Runtime.InteropServices.BuiltInComInterop.IsSupported false</ExtraTrimmerArgs> |
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 assume this is run against the live runtime we just built?
If so, I think we should consider moving the default feature switch configuration for the linker into the runtime and try to insert it into the SDK. That would help catch issues like this much faster than waiting for the next SDK update.
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 assume this is run against the live runtime we just built?
correct.
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.
consider moving the default feature switch configuration for the linker into the runtime and try to insert it into the SDK
I don't think that is fully worth the engineering and servicing effort of shipping "tooling" out of the runtime. The defaulting belongs in the dotnet/sdk
IMO - that is the SDK for the runtime. It would just be great if we weren't 2-3 previews behind in the SDK we are using to build the dotnet/runtime.
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.
+1 on the defaults belonging to SDK. Even more so because different SDKs have different defaults.
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.
In that case, is the proposal to upgrade the SDK used in runtime much more frequently? This could cause a lot of unrelated churn, and we would probably have to be on nightly builds in order to catch things quickly.
If we could somehow enable feature switches faster though, then we wouldn't be dependent on SDK updates.
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 could somehow enable feature switches faster though
We can set feature switches in the tests, like this new test is doing. If required, we could set some feature switches for all tests. But for BuiltInCOMInterop, we will get it for free once #55283 is merged.
Here are the other switches that are defaulted based on PublishTrimmed:
<PropertyGroup Condition="'$(PublishTrimmed)' == 'true' And
$([MSBuild]::VersionGreaterThanOrEquals($(_TargetFrameworkVersionWithoutV), '6.0'))">
<StartupHookSupport Condition="'$(StartupHookSupport)' == ''">false</StartupHookSupport>
<CustomResourceTypesSupport Condition="'$(CustomResourceTypesSupport)' == ''">false</CustomResourceTypesSupport>
<EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization Condition="'$(EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization)' == ''">false</EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization>
<EnableUnsafeBinaryFormatterSerialization Condition="'$(EnableUnsafeBinaryFormatterSerialization)' == ''">false</EnableUnsafeBinaryFormatterSerialization>
<EnableUnsafeUTF7Encoding Condition="'$(EnableUnsafeUTF7Encoding )' == ''">false</EnableUnsafeUTF7Encoding >
<BuiltInComInteropSupport Condition="'$(BuiltInComInteropSupport)' == ''">false</BuiltInComInteropSupport>
<AutoreleasePoolSupport Condition="'$(AutoreleasePoolSupport)' == ''">false</AutoreleasePoolSupport>
<EnableCppCLIHostActivation Condition="'$(EnableCppCLIHostActivation)' == ''">false</EnableCppCLIHostActivation>
<_EnableConsumingManagedCodeFromNativeHosting Condition="'$(_EnableConsumingManagedCodeFromNativeHosting)' == ''">false</_EnableConsumingManagedCodeFromNativeHosting>
BuiltInCOM is definitely the most likely to cause issues across the stack (like we saw here).
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.
@Anipik @ViktorHofer - thoughts on upgrading the SDK more frequently? preview6 shipped yesterday and we are just trying to update to preview5 now.
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.
Right we could update the feature switches manually like we did here, but it seems like we would want them all to be enabled ASAP, which is why I proposed moving the feature switch code into the runtime. Otherwise it seems too easy to forget to change the tests.
src/libraries/System.Runtime/tests/TrimmingTests/TypeBuilderComDisabled.cs
Show resolved
Hide resolved
Co-authored-by: Eric Erhardt <[email protected]>
src/libraries/System.Runtime/tests/TrimmingTests/TypeBuilderComDisabled.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.
Looks good. Thanks for fixing this.
I'm all for ingesting fresh SDK bits more frequently but it comes with a significant burden. In Tactics we agreed on that we want to depend on publicly available, singed SDK bits only when building our repositories. As we don't sign nightly builds and we don't want to force community members and us to install nightly SDKs on our machines we want to depend on publicly announced and released SDKs only which usually release on a monthly basis until a major version ships. I discussed with @Anipik in the past that it would be great to have a validation job running which ingests newer SDK bits to detect regressions / changes that need reaction earlier. The current process is very manual and requires significant amount of work. cc @marek-safar and @mmitche as we had a meeting for this some months ago. Unsure where we currently stand with automatically updating Arcade's SDK when a new SDK is released? |
There's no automatic update, and no plans to implement the necessary workflow to get it done. It looks like the last update was for p4. dotnet/arcade#7657 |
Fixes #55600