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.

Sign up for GitHub

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

Jump to bottom

Disable array support for the COM variant wrapper classes when built-in COM is disabled. #55756

Merged
merged 10 commits into from
Jul 18, 2021
Merged

Disable array support for the COM variant wrapper classes when built-in COM is disabled. #55756

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5985f54
Disable array support for the COM variant wrapper classes when built-…
jkoritzinsky Jul 15, 2021
69d7d61
Fix config call.
jkoritzinsky Jul 15, 2021
10d3b3d
Fix one more location of wrapper class usage.
jkoritzinsky Jul 15, 2021
6f51a07
Fix method name.
jkoritzinsky Jul 15, 2021
e0e219f
Add trimming test.
jkoritzinsky Jul 15, 2021
19bca14
Fix AV in IsArrayOfWrappers
elinor-fung Jul 16, 2021
801d9c8
Fix trimming test
elinor-fung Jul 16, 2021
8cf985b
Apply suggestions from code review
elinor-fung Jul 16, 2021
ade2649
Set Trim=true
elinor-fung Jul 16, 2021
b1d7638
Add licence header
elinor-fung Jul 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/coreclr/vm/mlinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3863,7 +3863,7 @@ void ArrayMarshalInfo::InitElementInfo(CorNativeType arrayNativeType, MarshalInf
}
}
#ifdef FEATURE_COMINTEROP
else if (m_thElement == TypeHandle(CoreLibBinder::GetClass(CLASS__ERROR_WRAPPER)))
else if (g_pConfig->IsBuiltInCOMSupported() && m_thElement == TypeHandle(CoreLibBinder::GetClass(CLASS__ERROR_WRAPPER)))
{
m_vtElement = VT_ERROR;
}
Expand Down
50 changes: 33 additions & 17 deletions src/coreclr/vm/olevariant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,20 +329,24 @@ VARTYPE OleVariant::GetVarTypeForTypeHandle(TypeHandle type)
#endif

#ifdef FEATURE_COMINTEROP
if (CoreLibBinder::IsClass(pMT, CLASS__DISPATCH_WRAPPER))
return VT_DISPATCH;
if (CoreLibBinder::IsClass(pMT, CLASS__UNKNOWN_WRAPPER))
return VT_UNKNOWN;
if (CoreLibBinder::IsClass(pMT, CLASS__ERROR_WRAPPER))
return VT_ERROR;
if (CoreLibBinder::IsClass(pMT, CLASS__CURRENCY_WRAPPER))
return VT_CY;
if (CoreLibBinder::IsClass(pMT, CLASS__BSTR_WRAPPER))
return VT_BSTR;
// The wrapper types are only available when built-in COM is supported.
if (g_pConfig->IsBuiltInCOMSupported())
{
if (CoreLibBinder::IsClass(pMT, CLASS__DISPATCH_WRAPPER))
return VT_DISPATCH;
if (CoreLibBinder::IsClass(pMT, CLASS__UNKNOWN_WRAPPER))
return VT_UNKNOWN;
if (CoreLibBinder::IsClass(pMT, CLASS__ERROR_WRAPPER))
elinor-fung marked this conversation as resolved.
Show resolved Hide resolved
return VT_ERROR;
if (CoreLibBinder::IsClass(pMT, CLASS__CURRENCY_WRAPPER))
return VT_CY;
if (CoreLibBinder::IsClass(pMT, CLASS__BSTR_WRAPPER))
return VT_BSTR;

// VariantWrappers cannot be stored in VARIANT's.
if (CoreLibBinder::IsClass(pMT, CLASS__VARIANT_WRAPPER))
COMPlusThrow(kArgumentException, IDS_EE_COM_UNSUPPORTED_SIG);
// VariantWrappers cannot be stored in VARIANT's.
if (CoreLibBinder::IsClass(pMT, CLASS__VARIANT_WRAPPER))
COMPlusThrow(kArgumentException, IDS_EE_COM_UNSUPPORTED_SIG);
}
#endif // FEATURE_COMINTEROP

Copy link
Member

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?

Copy link
Member Author

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.

if (pMT->IsEnum())
Expand Down Expand Up @@ -4857,22 +4861,33 @@ BOOL OleVariant::IsArrayOfWrappers(BASEARRAYREF *pArray, BOOL *pbOfInterfaceWrap
}
CONTRACTL_END;

if (!g_pConfig->IsBuiltInCOMSupported())
{
return FALSE;
}

TypeHandle hndElemType = (*pArray)->GetArrayElementTypeHandle();

if (!hndElemType.IsTypeDesc())
{
if (hndElemType == TypeHandle(CoreLibBinder::GetClass(CLASS__DISPATCH_WRAPPER)) ||
hndElemType == TypeHandle(CoreLibBinder::GetClass(CLASS__UNKNOWN_WRAPPER)))
{
*pbOfInterfaceWrappers = TRUE;
if (pbOfInterfaceWrappers)
{
*pbOfInterfaceWrappers = TRUE;
}
return TRUE;
}

if (hndElemType == TypeHandle(CoreLibBinder::GetClass(CLASS__ERROR_WRAPPER)) ||
hndElemType == TypeHandle(CoreLibBinder::GetClass(CLASS__CURRENCY_WRAPPER)) ||
hndElemType == TypeHandle(CoreLibBinder::GetClass(CLASS__BSTR_WRAPPER)))
{
*pbOfInterfaceWrappers = FALSE;
if (pbOfInterfaceWrappers)
{
*pbOfInterfaceWrappers = FALSE;
}
return TRUE;
}
}
Expand All @@ -4889,6 +4904,7 @@ BASEARRAYREF OleVariant::ExtractWrappedObjectsFromArray(BASEARRAYREF *pArray)
GC_TRIGGERS;
MODE_COOPERATIVE;
PRECONDITION(CheckPointer(pArray));
PRECONDITION(IsArrayOfWrappers(pArray, NULL));
}
CONTRACTL_END;

Expand Down Expand Up @@ -5022,6 +5038,7 @@ TypeHandle OleVariant::GetWrappedArrayElementType(BASEARRAYREF *pArray)
GC_TRIGGERS;
MODE_COOPERATIVE;
PRECONDITION(CheckPointer(pArray));
PRECONDITION(IsArrayOfWrappers(pArray, NULL));
}
CONTRACTL_END;

Expand Down Expand Up @@ -5067,8 +5084,7 @@ TypeHandle OleVariant::GetArrayElementTypeWrapperAware(BASEARRAYREF *pArray)
}
CONTRACTL_END;

BOOL bArrayOfInterfaceWrappers;
if (IsArrayOfWrappers(pArray, &bArrayOfInterfaceWrappers))
if (IsArrayOfWrappers(pArray, nullptr))
{
return GetWrappedArrayElementType(pArray);
}
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Runtime/tests/TrimmingTests/System.Runtime.TrimmingTests.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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:

https://github.com/dotnet/sdk/blob/0ce0580f222ff42a1a14eed42f01dcf2f41ea82f/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets#L35-L45

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

Copy link
Member

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.

Copy link
Member

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.

</TestConsoleAppSourceFiles>
</ItemGroup>

<Import Project="$([MSBuild]::GetPathOfFileAbove(Directory.Build.targets))" />
Expand Down
12 changes: 12 additions & 0 deletions src/libraries/System.Runtime/tests/TrimmingTests/TypeBuilderComDisabled.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using System;
elinor-fung marked this conversation as resolved.
Show resolved Hide resolved
using System.Reflection;
using System.Reflection.Emit;

AssemblyBuilder assembly = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("GeneratedAssembly"), AssemblyBuilderAccess.Run);
ModuleBuilder module = assembly.DefineDynamicModule("GeneratedModule");
TypeBuilder genericType = module.DefineType("GeneratedType");
genericType.DefineField("_int", typeof(int), FieldAttributes.Private);
genericType.DefineProperty("Prop", PropertyAttributes.None, typeof(string), null);

Type generatedType = genericType.CreateType();
eerhardt marked this conversation as resolved.
Show resolved Hide resolved
return 100;