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

Convert OAVariant interop to managed #100176

Merged
merged 37 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
aa612d1
Convert BoxEnum to managed
huoyaoyuan Jun 8, 2022
a3dfc8e
Temp commit
huoyaoyuan Jun 8, 2022
6ceec59
SetFieldsObject to managed
huoyaoyuan Mar 23, 2024
5eb3f80
Color call
huoyaoyuan Mar 23, 2024
f82d6b6
Cleanup FCall
huoyaoyuan Mar 23, 2024
e8f3f1f
Handle decimal and other CV_OBJECT
huoyaoyuan Mar 23, 2024
88f9e55
Fix BoxEnum
huoyaoyuan Mar 25, 2024
4013082
Body of OAVariant_ChangeType
huoyaoyuan Mar 25, 2024
f12f8f6
Avoid converting through Variant in OAVariantLib
huoyaoyuan Mar 25, 2024
f2c96e2
Managed ToOAVariant and FromOAVariant
huoyaoyuan Mar 26, 2024
9787743
Marshal for IUnknown and IDispatch
huoyaoyuan Mar 26, 2024
2164865
Fullfill interop for OAVariant
huoyaoyuan Mar 26, 2024
aa963e0
VariantChangeTypeEx interop
huoyaoyuan Mar 26, 2024
568ba08
Complete GetObjectRefFromComIP
huoyaoyuan Mar 26, 2024
e6338a1
System.Color interop
huoyaoyuan Mar 26, 2024
6cda5f3
Use managed reflection for System.Drawing.Color conversion
huoyaoyuan Mar 27, 2024
80c9f56
Use MarshalNative for IDispatch/IUnknown marshalling
huoyaoyuan Mar 27, 2024
d4addb9
Fulfill behavior of ToOAVariant
huoyaoyuan Mar 27, 2024
d17d427
Delete unreachable special type handling
huoyaoyuan Mar 27, 2024
b4b86bb
Revert System.Drawing.Color reflection
huoyaoyuan Mar 27, 2024
756ad96
Revert changes around Variant
huoyaoyuan Mar 28, 2024
de55154
Move Color conversion to Variant
huoyaoyuan Mar 28, 2024
1cfc19b
Respect VTToCV mapping
huoyaoyuan Mar 28, 2024
539040c
Update src/coreclr/vm/marshalnative.cpp
huoyaoyuan Mar 28, 2024
f989fce
Update src/coreclr/classlibnative/bcltype/variant.cpp
huoyaoyuan Mar 29, 2024
3c61dd4
Merge branch 'main' into variant
huoyaoyuan May 5, 2024
434fff6
Cleanup impossible types and unused constants
huoyaoyuan May 5, 2024
24bada7
Fix HResult
huoyaoyuan May 5, 2024
9c1e96f
Use constants for HRESULT
huoyaoyuan May 6, 2024
578b028
Improve test type coverage
huoyaoyuan May 7, 2024
00a0a61
Remove more impossible types
huoyaoyuan May 7, 2024
4256710
Merge branch 'main' into variant
huoyaoyuan May 7, 2024
ca55c17
Apply suggestions from code review
huoyaoyuan May 8, 2024
9d79d19
Don't test for types that were not requested
huoyaoyuan May 8, 2024
4334c40
Fix compilation of NETServer
huoyaoyuan May 8, 2024
e4c4994
Test for values in ReturnToManaged
huoyaoyuan May 12, 2024
ae0b4f8
Add value test in managed arg
huoyaoyuan May 13, 2024
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
189 changes: 126 additions & 63 deletions src/coreclr/System.Private.CoreLib/src/Microsoft/Win32/OAVariantLib.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,15 @@
===========================================================*/

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Runtime.InteropServices.Marshalling;

#pragma warning disable CA1416 // COM interop is only supported on Windows

namespace Microsoft.Win32
{
Expand All @@ -26,102 +30,161 @@ internal static unsafe partial class OAVariantLib
#region Constants

// Constants for VariantChangeType from OleAuto.h
public const int NoValueProp = 0x01;
public const int AlphaBool = 0x02;
public const int NoUserOverride = 0x04;
public const int CalendarHijri = 0x08;
public const int LocalBool = 0x10;

internal static readonly Type?[] ClassTypes = {
typeof(Empty),
typeof(void),
typeof(bool),
typeof(char),
typeof(sbyte),
typeof(byte),
typeof(short),
typeof(ushort),
typeof(int),
typeof(uint),
typeof(long),
typeof(ulong),
typeof(float),
typeof(double),
typeof(string),
typeof(void),
typeof(DateTime),
typeof(TimeSpan),
typeof(object),
typeof(decimal),
null, // Enums - what do we do here?
typeof(Missing),
typeof(DBNull),
private static readonly Dictionary<Type, VarEnum> ClassTypes = new Dictionary<Type, VarEnum>
Copy link
Member

Choose a reason for hiding this comment

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

I don't see currency or null in this list. How are we handling those?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currency: The previous ClassTypes doesn't contain an entry for CurrencyWrapper or something. It's not mapped.
Null: The misplace mentioned in #100176 (comment) cause it to be mapped to CV_MISSING, and rejected in OAVaraint::CVToVT.

{
{ typeof(bool), VarEnum.VT_BOOL },
{ typeof(char), VarEnum.VT_I2 },
{ typeof(sbyte), VarEnum.VT_I1 },
{ typeof(byte), VarEnum.VT_UI1 },
{ typeof(short), VarEnum.VT_I2 },
{ typeof(ushort), VarEnum.VT_UI2 },
{ typeof(int), VarEnum.VT_I4 },
{ typeof(uint), VarEnum.VT_UI4 },
{ typeof(long), VarEnum.VT_I8 },
{ typeof(ulong), VarEnum.VT_UI8 },
{ typeof(float), VarEnum.VT_R4 },
{ typeof(double), VarEnum.VT_R8 },
{ typeof(string), VarEnum.VT_BSTR },
{ typeof(DateTime), VarEnum.VT_DATE },
{ typeof(decimal), VarEnum.VT_DECIMAL },
};

// Keep these numbers in sync w/ the above array.
private const int CV_OBJECT = 0x12;

#endregion


#region Internal Methods

#pragma warning disable CS8500

/**
* Changes a Variant from one type to another, calling the OLE
* Automation VariantChangeTypeEx routine. Note the legal types here are
* restricted to the subset of what can be legally found in a VB
* Variant and the types that CLR supports explicitly in the
* CLR Variant class.
*/
internal static Variant ChangeType(Variant source, Type targetClass, short options, CultureInfo culture)
internal static object? ChangeType(object source, Type targetClass, short options, CultureInfo culture)
{
ArgumentNullException.ThrowIfNull(targetClass);
ArgumentNullException.ThrowIfNull(culture);

Variant result = default;
ChangeType(
&result,
&source,
culture.LCID,
targetClass.TypeHandle.Value,
GetCVTypeFromClass(targetClass),
options);
return result;
}
object? result = null;

[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "OAVariant_ChangeType")]
private static partial void ChangeType(Variant* result, Variant* source, int lcid, IntPtr typeHandle, int cvType, short flags);
if (Variant.IsSystemDrawingColor(targetClass))
{
if (source.GetType() == typeof(int) || source.GetType() == typeof(uint))
huoyaoyuan marked this conversation as resolved.
Show resolved Hide resolved
{
uint sourceData = source.GetType() == typeof(int) ? (uint)(int)source : (uint)source;
huoyaoyuan marked this conversation as resolved.
Show resolved Hide resolved
// Int32/UInt32 can be converted to System.Drawing.Color
Variant.ConvertOleColorToSystemColor(ObjectHandleOnStack.Create(ref result), sourceData, targetClass.TypeHandle.Value);
Debug.Assert(result != null);
return result;
}
}

#pragma warning restore CS8500
if (!ClassTypes.TryGetValue(targetClass, out VarEnum vt))
throw new NotSupportedException(SR.NotSupported_ChangeType);
huoyaoyuan marked this conversation as resolved.
Show resolved Hide resolved

#endregion
ComVariant vOp = ToOAVariant(source);
ComVariant ret = default;

int hr = Interop.OleAut32.VariantChangeTypeEx(&ret, &vOp, culture.LCID, options, (ushort)vt);

#region Private Helpers
using (vOp)
using (ret)
{
if (hr < 0)
OAFailed(hr);
huoyaoyuan marked this conversation as resolved.
Show resolved Hide resolved

private static int GetCVTypeFromClass(Type ctype)
{
Debug.Assert(ctype != null);
Debug.Assert(ClassTypes[CV_OBJECT] == typeof(object), "OAVariantLib::ClassTypes[CV_OBJECT] == Object.class");
result = FromOAVariant(ret);
if (targetClass == typeof(char))
{
result = (char)(uint)result!;
}
}

// OleAut Binder works better if unrecognized
// types were changed to Object.
int cvtype = CV_OBJECT;
return result;
}

for (int i = 0; i < ClassTypes.Length; i++)
private static void OAFailed(int hr)
{
switch (hr)
{
if (ctype.Equals(ClassTypes[i]))
{
cvtype = i;
break;
}
case HResults.COR_E_OUTOFMEMORY:
throw new OutOfMemoryException();
case HResults.DISP_E_BADVARTYPE:
throw new NotSupportedException(SR.NotSupported_OleAutBadVarType);
case HResults.DISP_E_DIVBYZERO:
throw new DivideByZeroException();
case HResults.DISP_E_OVERFLOW:
throw new OverflowException();
case HResults.DISP_E_TYPEMISMATCH:
throw new InvalidCastException(SR.InvalidCast_OATypeMismatch);
case HResults.E_INVALIDARG:
throw new ArgumentException();
default:
Debug.Fail("Unrecognized HResult - OAVariantLib routine failed in an unexpected way!");
throw Marshal.GetExceptionForHR(hr);
}
}

return cvtype;
private static ComVariant ToOAVariant(object input)
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 need to understand more about OAVaraintLib and OleAutBinder. This path does not use the same variant marshal methods and does not handle some cases like wrapped VT types. How indeed would this path be invoked?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's called by CLRToCOMLateBoundWorker. Perhaps we should make it's behavior consistent with other COM interop scenario? This PR is trying keep things as-is.

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 legacy COM interop support. We are not interested in changing any observable behavior. We have learned hard way multiple times that the applications out there depend on every observable behavior detail of legacy COM interop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well how about the fix of misplacement in CV_TYPE array? Should such bug-to-bug behavior be preserved? Did it fail anyway in caller?

Copy link
Member

Choose a reason for hiding this comment

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

If there are cases that are 100% bugs - like something that would always crash and there is no way that a working code can depend on it, it should be ok to fix those. We should have a test case added for any cases like that.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Mar 28, 2024

Choose a reason for hiding this comment

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

Should we change the behavior (of new mechanism) to align, or keep the inconsistency forever?

Sadly yes. The built-in behavior is "correct" and should remain as it helps porting from .NET Framework to .NET 8+. Once teams are on .NET 8+, the highest priority, they can then port to the source generated solution with the consistent COM behavior. As @jkotas has mentioned the built-in COM behavior should remain static and unchanged whenever possible. Porting large products from .NET Framework are impacted enough by newer .NET semantics as is and forcing them to also take on COM interop semantics changes is a burden we need to avoid. See #100377 for an example affordance we are considering.

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 mean that changing the other routines to match built-in behavior. Specifically speaking, BuiltInInteropVariantExtensions.ToObject is missing a case of VT_ERROR. Should BuiltInInteropVariantExtensions.ToObject be changed?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem to be missing a case for VT_ERROR. Am I missing something?

public static object? ToObject(this ref ComVariant variant)
{
return variant.VarType switch
{
VarEnum.VT_EMPTY => null,
VarEnum.VT_NULL => DBNull.Value,
VarEnum.VT_I1 => variant.As<sbyte>(),
VarEnum.VT_I2 => variant.As<short>(),
VarEnum.VT_I4 => variant.As<int>(),
VarEnum.VT_I8 => variant.As<long>(),
VarEnum.VT_UI1 => variant.As<byte>(),
VarEnum.VT_UI2 => variant.As<ushort>(),
VarEnum.VT_UI4 => variant.As<uint>(),
VarEnum.VT_UI8 => variant.As<ulong>(),
VarEnum.VT_INT => variant.As<int>(),
VarEnum.VT_UINT => variant.As<uint>(),
VarEnum.VT_BOOL => variant.As<bool>(),
VarEnum.VT_ERROR => variant.As<int>(),
VarEnum.VT_R4 => variant.As<float>(),
VarEnum.VT_R8 => variant.As<double>(),
VarEnum.VT_DECIMAL => variant.As<decimal>(),
VarEnum.VT_CY => decimal.FromOACurrency(variant.GetRawDataRef<long>()),
VarEnum.VT_DATE => variant.As<DateTime>(),
VarEnum.VT_BSTR => variant.GetRawDataRef<nint>() is 0 ? null : Marshal.PtrToStringBSTR(variant.GetRawDataRef<nint>()),
VarEnum.VT_UNKNOWN or VarEnum.VT_DISPATCH => variant.GetRawDataRef<nint>() is 0 ? null : Marshal.GetObjectForIUnknown(variant.GetRawDataRef<nint>()),
_ => GetObjectFromNativeVariant(ref variant),
};

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not covered by OAVariant, but for the early bound Variant case:

void OleVariant::MarshalErrorVariantOleToCom(VARIANT *pOleVariant,
VariantData *pComVariant)
{
CONTRACTL
{
NOTHROW;
GC_NOTRIGGER;
MODE_ANY;
PRECONDITION(CheckPointer(pOleVariant));
PRECONDITION(CheckPointer(pComVariant));
}
CONTRACTL_END;
// Check to see if the variant represents a missing argument.
if (V_ERROR(pOleVariant) == DISP_E_PARAMNOTFOUND)
{
pComVariant->SetType(CV_MISSING);
}
else
{
pComVariant->SetDataAsInt32(V_ERROR(pOleVariant));
}
}

Certain ERROR is converted to CV_MISSING.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Mar 29, 2024

Choose a reason for hiding this comment

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

It's not covered by OAVariant, but for the early bound Variant case:

I don't understand this response. The following statement was made above at #100176 (comment).

Specifically speaking, BuiltInInteropVariantExtensions.ToObject is missing a case of VT_ERROR. Should BuiltInInteropVariantExtensions.ToObject be changed?

Was this statement a mistake? I don't see how this is relates to the reply regarding MarshalErrorVariantOleToCom().

{
return input switch
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
{
string str => ComVariant.Create(str),
DateTime dateTime => ComVariant.Create(dateTime),
bool b => ComVariant.Create(b),
decimal d => ComVariant.Create(d),
sbyte i1 => ComVariant.Create(i1),
byte u1 => ComVariant.Create(u1),
short i2 => ComVariant.Create(i2),
ushort u2 => ComVariant.Create(u2),
int i4 => ComVariant.Create(i4),
uint u4 => ComVariant.Create(u4),
long i8 => ComVariant.Create(i8),
ulong u8 => ComVariant.Create(u8),
float r4 => ComVariant.Create(r4),
double r8 => ComVariant.Create(r8),
null => default,
Missing => throw new NotSupportedException(SR.NotSupported_ChangeType),
DBNull => ComVariant.Null,
_ => GetComIPFromObjectRef(input) // Convert the object to an IDispatch/IUnknown pointer.
};
}

private static ComVariant GetComIPFromObjectRef(object? obj)
{
IntPtr pUnk = GetIUnknownOrIDispatchForObject(ObjectHandleOnStack.Create(ref obj), out bool isIDispatch);
return ComVariant.CreateRaw(isIDispatch ? VarEnum.VT_DISPATCH : VarEnum.VT_UNKNOWN, pUnk);
}

[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "MarshalNative_GetIUnknownOrIDispatchForObject")]
private static partial IntPtr GetIUnknownOrIDispatchForObject(ObjectHandleOnStack o, [MarshalAs(UnmanagedType.Bool)] out bool isIDispatch);

private static object? FromOAVariant(ComVariant input) =>
input.VarType switch
{
VarEnum.VT_BSTR => input.As<string>(),
VarEnum.VT_DATE => input.As<DateTime>(),
VarEnum.VT_BOOL => input.As<bool>(),
VarEnum.VT_DECIMAL => input.As<decimal>(),
VarEnum.VT_I1 => input.As<sbyte>(),
VarEnum.VT_UI1 => input.As<byte>(),
VarEnum.VT_I2 => input.As<short>(),
VarEnum.VT_UI2 => input.As<ushort>(),
VarEnum.VT_I4 or VarEnum.VT_INT => input.As<int>(),
VarEnum.VT_UI4 or VarEnum.VT_UINT => input.As<uint>(),
VarEnum.VT_I8 => input.As<long>(),
VarEnum.VT_UI8 => input.As<ulong>(),
VarEnum.VT_R4 => input.As<float>(),
VarEnum.VT_R8 => input.As<double>(),
VarEnum.VT_EMPTY => null,
VarEnum.VT_UNKNOWN or VarEnum.VT_VARIANT => Marshal.GetObjectForIUnknown(input.GetRawDataRef<IntPtr>()),
VarEnum.VT_VOID => null,
_ => throw new NotSupportedException(SR.NotSupported_ChangeType),
};

#endregion
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ internal sealed class OleAutBinder : DefaultBinder
// This binder uses OLEAUT to change the type of the variant.
public override object ChangeType(object value, Type type, CultureInfo? cultureInfo)
{
Variant myValue = new Variant(value);
cultureInfo ??= CultureInfo.CurrentCulture;

#if DISPLAY_DEBUG_INFO
Expand Down Expand Up @@ -62,7 +61,7 @@ public override object ChangeType(object value, Type type, CultureInfo? cultureI
#endif
// Specify the LocalBool flag to have BOOL values converted to local language rather
// than 0 or -1.
object RetObj = OAVariantLib.ChangeType(myValue, type, OAVariantLib.LocalBool, cultureInfo).ToObject()!;
object RetObj = OAVariantLib.ChangeType(value, type, OAVariantLib.LocalBool, cultureInfo)!;

#if DISPLAY_DEBUG_INFO
Console.WriteLine("Object returned from ChangeType is of type: " + RetObj.GetType().Name);
Expand Down
10 changes: 9 additions & 1 deletion src/coreclr/System.Private.CoreLib/src/System/Variant.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

namespace System
{
internal struct Variant
internal partial struct Variant
{
// Do Not change the order of these fields.
// They are mapped to the native VariantData * data structure.
Expand Down Expand Up @@ -70,6 +70,14 @@ internal struct Variant
internal static Variant Missing => new Variant(CV_MISSING, Type.Missing, 0);
internal static Variant DBNull => new Variant(CV_NULL, System.DBNull.Value, 0);

internal static bool IsSystemDrawingColor(Type type) => type.FullName == "System.Drawing.Color"; // Matches the behavior of IsTypeRefOrDef

[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Variant_ConvertSystemColorToOleColor")]
internal static partial uint ConvertSystemColorToOleColor(ObjectHandleOnStack obj);

[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "Variant_ConvertOleColorToSystemColor")]
internal static partial void ConvertOleColorToSystemColor(ObjectHandleOnStack objret, uint value, IntPtr pMT);

//
// Native Methods
//
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/classlibnative/bcltype/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ set(CMAKE_INCLUDE_CURRENT_DIR ON)

set(BCLTYPE_SOURCES
arraynative.cpp
oavariant.cpp
objectnative.cpp
system.cpp
varargsnative.cpp
Expand Down
Loading
Loading