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

Implement System.Runtime.InteropServices.CLong/CULong/NFloat #46401

Merged
merged 23 commits into from
Jan 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e8872a4
Implement managed side of CLong/CULong/NFloat.
jkoritzinsky Dec 24, 2020
3b7693b
Make CLong, CULong, and NFloat intrinsically handled correctly by the…
jkoritzinsky Dec 25, 2020
e598892
Add framework tests for CLong, CULong, and NFloat.
jkoritzinsky Dec 25, 2020
c9d8d13
Add interop test of CLong to validate calling convention semantics.
jkoritzinsky Dec 25, 2020
0aeaaf7
Update CULong.cs
jkoritzinsky Dec 25, 2020
2baa10d
Fix implicit conversions.
jkoritzinsky Dec 25, 2020
57ba638
Merge branch 'clong' of github.com:jkoritzinsky/runtime into clong
jkoritzinsky Dec 25, 2020
d2da1d8
Fix overflow and equality test failures.
jkoritzinsky Jan 4, 2021
f141023
Fix formatting.
jkoritzinsky Jan 4, 2021
e587cfc
Fix formatting and add function header.
jkoritzinsky Jan 4, 2021
5b9d7a3
Add doc comments.
jkoritzinsky Jan 4, 2021
62acb9e
Don't throw on float out of range. Rename tests.
jkoritzinsky Jan 4, 2021
a365f26
Rewrite EqualsTest implementations more straightforward.
jkoritzinsky Jan 8, 2021
b114e7e
Fix NFloat tests.
jkoritzinsky Jan 8, 2021
265a914
Use .Equals instead of ==
jkoritzinsky Jan 8, 2021
5404cec
Use ToString directly instead of hard coding the expected value.
jkoritzinsky Jan 8, 2021
cac95b4
Update the emitted assembly stub's thiscall retbuf support for x86 to…
jkoritzinsky Jan 8, 2021
59785f5
Add sizeof tests.
jkoritzinsky Jan 8, 2021
65eddf7
Add test with struct containing CLong.
jkoritzinsky Jan 8, 2021
3e2ded1
Merge branch 'master' of github.com:dotnet/runtime into clong
jkoritzinsky Jan 11, 2021
314e03a
Disable ThisCallTest on Mono due to #46820
jkoritzinsky Jan 11, 2021
b285a8a
Merge branch 'master' of github.com:dotnet/runtime into clong
jkoritzinsky Jan 11, 2021
19e0063
validate type name.
jkoritzinsky Jan 12, 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
33 changes: 31 additions & 2 deletions src/coreclr/jit/compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,35 @@ bool Compiler::isTrivialPointerSizedStruct(CORINFO_CLASS_HANDLE clsHnd) const
}
#endif // TARGET_X86

//---------------------------------------------------------------------------
// isNativePrimitiveStructType:
// Check if the given struct type is an intrinsic type that should be treated as though
// it is not a struct at the unmanaged ABI boundary.
//
// Arguments:
// clsHnd - the handle for the struct type.
//
// Return Value:
// true if the given struct type should be treated as a primitive for unmanaged calls,
// false otherwise.
//
bool Compiler::isNativePrimitiveStructType(CORINFO_CLASS_HANDLE clsHnd)
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
{
if (!isIntrinsicType(clsHnd))
{
return false;
}
const char* namespaceName = nullptr;
const char* typeName = getClassNameFromMetadata(clsHnd, &namespaceName);

if (strcmp(namespaceName, "System.Runtime.InteropServices") != 0)
{
return false;
}

return strcmp(typeName, "CLong") == 0 || strcmp(typeName, "CULong") == 0 || strcmp(typeName, "NFloat") == 0;
}

//-----------------------------------------------------------------------------
// getPrimitiveTypeForStruct:
// Get the "primitive" type that is is used for a struct
Expand Down Expand Up @@ -1013,14 +1042,14 @@ var_types Compiler::getReturnTypeForStruct(CORINFO_CLASS_HANDLE clsHnd,
}
}
#elif UNIX_X86_ABI
if (callConv != CorInfoCallConvExtension::Managed)
if (callConv != CorInfoCallConvExtension::Managed && !isNativePrimitiveStructType(clsHnd))
{
canReturnInRegister = false;
howToReturnStruct = SPK_ByReference;
useType = TYP_UNKNOWN;
}
#elif defined(TARGET_WINDOWS) && !defined(TARGET_ARM)
if (callConvIsInstanceMethodCallConv(callConv))
if (callConvIsInstanceMethodCallConv(callConv) && !isNativePrimitiveStructType(clsHnd))
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, it looks like this call is expensive, should we measure TP on x64 windows?

Copy link
Member

Choose a reason for hiding this comment

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

Would this be expensive since its initially filtered on isIntrinsicType, which should just be a simple cached flag check?

{
canReturnInRegister = false;
howToReturnStruct = SPK_ByReference;
Expand Down
34 changes: 19 additions & 15 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -5046,6 +5046,10 @@ class Compiler
// Convert a BYTE which represents the VM's CorInfoGCtype to the JIT's var_types
var_types getJitGCType(BYTE gcType);

// Returns true if the provided type should be treated as a primitive type
// for the unmanaged calling conventions.
bool isNativePrimitiveStructType(CORINFO_CLASS_HANDLE clsHnd);

enum structPassingKind
{
SPK_Unknown, // Invalid value, never returned
Expand Down Expand Up @@ -8045,6 +8049,21 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
#endif
}

bool isIntrinsicType(CORINFO_CLASS_HANDLE clsHnd)
Copy link
Contributor

Choose a reason for hiding this comment

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

was it just moved or do I miss an actual change for these 3 functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was just moved out of the FEATURE_SIMD block.

{
return info.compCompHnd->isIntrinsicType(clsHnd);
}

const char* getClassNameFromMetadata(CORINFO_CLASS_HANDLE cls, const char** namespaceName)
{
return info.compCompHnd->getClassNameFromMetadata(cls, namespaceName);
}

CORINFO_CLASS_HANDLE getTypeInstantiationArgument(CORINFO_CLASS_HANDLE cls, unsigned index)
{
return info.compCompHnd->getTypeInstantiationArgument(cls, index);
}

#ifdef FEATURE_SIMD

// Should we support SIMD intrinsics?
Expand Down Expand Up @@ -8263,21 +8282,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
return false;
}

bool isIntrinsicType(CORINFO_CLASS_HANDLE clsHnd)
{
return info.compCompHnd->isIntrinsicType(clsHnd);
}

const char* getClassNameFromMetadata(CORINFO_CLASS_HANDLE cls, const char** namespaceName)
{
return info.compCompHnd->getClassNameFromMetadata(cls, namespaceName);
}

CORINFO_CLASS_HANDLE getTypeInstantiationArgument(CORINFO_CLASS_HANDLE cls, unsigned index)
{
return info.compCompHnd->getTypeInstantiationArgument(cls, index);
}

bool isSIMDClass(typeInfo* pTypeInfo)
{
return pTypeInfo->IsStruct() && isSIMDClass(pTypeInfo->GetClassHandleForValueClass());
Expand Down
38 changes: 30 additions & 8 deletions src/coreclr/vm/dllimportcallback.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ VOID UMEntryThunk::CompileUMThunkWorker(UMThunkStubInfo *pInfo,
// push edx - repush the return address
pcpusl->X86EmitPushReg(kEDX);
}

// The native signature doesn't have a return buffer
// but the managed signature does.
// Set up the return buffer address here.
Expand All @@ -188,12 +188,12 @@ VOID UMEntryThunk::CompileUMThunkWorker(UMThunkStubInfo *pInfo,
// Calculate the offset to the return buffer we establish for EAX:EDX below.
// lea edx [esp - offset to EAX:EDX return buffer]
pcpusl->X86EmitEspOffset(0x8d, kEDX, -0xc /* skip return addr, EBP, EBX */ -0x8 /* point to start of EAX:EDX return buffer */ );

// exchange edx (which has the return buffer address)
// with the return address
// xchg edx, [esp]
pcpusl->X86EmitOp(0x87, kEDX, (X86Reg)kESP_Unsafe);
pcpusl->X86EmitOp(0x87, kEDX, (X86Reg)kESP_Unsafe);

// push edx
pcpusl->X86EmitPushReg(kEDX);
}
Expand Down Expand Up @@ -497,7 +497,7 @@ VOID UMEntryThunk::CompileUMThunkWorker(UMThunkStubInfo *pInfo,
pcpusl->X86EmitIndexRegStore(kEBX, -0x8 /* to outer EBP */ -0x8 /* skip saved EBP, EBX */, kEDX);
}
// In the umtmlBufRetValToEnreg case,
// we set up the return buffer to output
// we set up the return buffer to output
// into the EDX:EAX buffer we set up for the register return case.
// So we don't need to do more work here.
else if ((pInfo->m_wFlags & umtmlBufRetValToEnreg) == 0)
Expand Down Expand Up @@ -684,7 +684,7 @@ Stub *UMThunkMarshInfo::CompileNExportThunk(LoaderHeap *pLoaderHeap, PInvokeStat
UINT nOffset = 0;
int numRegistersUsed = 0;
int numStackSlotsIndex = nStackBytes / STACK_ELEM_SIZE;

// This could have been set in the UnmanagedCallersOnly scenario.
if (m_callConv == UINT16_MAX)
m_callConv = static_cast<UINT16>(pSigInfo->GetCallConv());
Expand All @@ -699,8 +699,30 @@ Stub *UMThunkMarshInfo::CompileNExportThunk(LoaderHeap *pLoaderHeap, PInvokeStat
numRegistersUsed++;
}

bool hasReturnBuffer = argit.HasRetBuffArg() || (m_callConv == pmCallConvThiscall && argit.HasValueTypeReturn());
bool hasNativeExchangeTypeReturn = false;

if (hasReturnBuffer)
{
// If think we have a return buffer, lets make sure that we aren't returning one of the intrinsic native exchange types.
TypeHandle returnType = pMetaSig->GetRetTypeHandleThrowing();
if (returnType.GetMethodTable()->IsIntrinsicType())
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved
{
LPCUTF8 pszNamespace;
LPCUTF8 pszTypeName = returnType.GetMethodTable()->GetFullyQualifiedNameInfo(&pszNamespace);
if ((strcmp(pszNamespace, g_InteropServicesNS) == 0)
&& (strcmp(pszTypeName, "CLong") == 0 || strcmp(pszTypeName, "CULong") == 0 || strcmp(pszTypeName, "NFloat") == 0))
{
// We have one of the intrinsic native exchange types.
// As a result, we don't have a return buffer.
hasReturnBuffer = false;
hasNativeExchangeTypeReturn = true;
}
}
}

// process the return buffer parameter
if (argit.HasRetBuffArg() || (m_callConv == pmCallConvThiscall && argit.HasValueTypeReturn()))
if (hasReturnBuffer)
{
// Only copy the retbuf arg from the src call when both the managed call and native call
// have a return buffer.
Expand Down Expand Up @@ -808,7 +830,7 @@ Stub *UMThunkMarshInfo::CompileNExportThunk(LoaderHeap *pLoaderHeap, PInvokeStat
{
stubInfo.m_wFlags |= umtmlThisCallHiddenArg;
}
else if (argit.HasValueTypeReturn())
else if (argit.HasValueTypeReturn() && !hasNativeExchangeTypeReturn)
{
stubInfo.m_wFlags |= umtmlThisCallHiddenArg | umtmlEnregRetValToBuf;
// When the native signature has a return buffer but the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public static partial class PlatformDetection
public static bool IsArgIteratorSupported => IsMonoRuntime || (IsWindows && IsNotArmProcess);
public static bool IsArgIteratorNotSupported => !IsArgIteratorSupported;
public static bool Is32BitProcess => IntPtr.Size == 4;
public static bool Is64BitProcess => IntPtr.Size == 8;
public static bool IsNotWindows => !IsWindows;

public static bool IsThreadingSupported => !IsBrowser;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CharSet.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ClassInterfaceAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ClassInterfaceType.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CLong.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CoClassAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CollectionsMarshal.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ComDefaultInterfaceAttribute.cs" />
Expand Down Expand Up @@ -735,6 +736,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ComTypes\ITypeLib2.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ComVisibleAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CriticalHandle.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CULong.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CurrencyWrapper.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CustomQueryInterfaceMode.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CustomQueryInterfaceResult.cs" />
Expand Down Expand Up @@ -769,6 +771,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\MemoryMarshal.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\UnmanagedCallersOnlyAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\NativeLibrary.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\NFloat.cs" />
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\OptionalAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\OutAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\PreserveSigAttribute.cs" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;

#pragma warning disable SA1121 // We use our own aliases since they differ per platform
#if TARGET_WINDOWS
using NativeType = System.Int32;
#else
using NativeType = System.IntPtr;
#endif

namespace System.Runtime.InteropServices
{
/// <summary>
/// <see cref="CLong"/> is an immutable value type that represents the <c>long</c> type in C and C++.
/// It is meant to be used as an exchange type at the managed/unmanaged boundary to accurately represent
/// in managed code unmanaged APIs that use the <c>long</c> type.
/// This type has 32-bits of storage on all Windows platforms and 32-bit Unix-based platforms.
/// It has 64-bits of storage on 64-bit Unix platforms.
/// </summary>
[CLSCompliant(false)]
Copy link
Member

Choose a reason for hiding this comment

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

This means this type isn't compliant in VB.NET. Which means APIs that decide to use it will not be consumable by VB.NET. Unsure if that is okay or not. I am fine with it but let's make sure we do our due diligence and confirm that shared understanding.

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 CLong can be CLSCompliant since it is signed and internally an int or nint, which both are CLSCompliant. However, I don't think CLSCompliant means it isn't consumable by VB.NET, using other types like UIntPtr or SByte seem to work just fine: https://sharplab.io/#v2:DYLgbgRgNALiCWwA+BJAtgBwPYCcYGcACAZQE98YBTNAWACgAFAVwmHgGNCBhYAQ3yJd6hEYWasOhAGJMAduxjwsswgFkAjAAoAlIQCCRAKopZMBjBzDR1gEqUYTHCuOnzOAHQAtSjixWRAKKyACbScgpKsv6E0eJsnDLyispqAEw6+kTEAEKkVNG29o4qALTq0UGhiRHK9JXcfAJAA=

I'm no expert in VB though, so it's definitely plausible that I'm unaware of some detail 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

CLSCompilant(false) just means that some or all of the members use types that are not guaranteed to be usable from all languages. The types themselves are still usable, but some members (like the uint constructor on CULong) aren't supported. It doesn't block the types from being used in VB.NET.

Copy link
Member

Choose a reason for hiding this comment

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

Is there anything actually preventing the use of this from other languages given it is just a struct wrapping a CLSCompliant type?

[Intrinsic]
public readonly struct CLong : IEquatable<CLong>
{
private readonly NativeType _value;

/// <summary>
/// Constructs an instance from a 32-bit integer.
/// </summary>
/// <param name="value">The integer vaule.</param>
public CLong(int value)
{
_value = (NativeType)value;
}
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Constructs an instance from a native sized integer.
/// </summary>
/// <param name="value">The integer vaule.</param>
/// <exception cref="OverflowException"><paramref name="value"/> is outside the range of the underlying storage type.</exception>
public CLong(nint value)
{
_value = checked((NativeType)value);
}

/// <summary>
/// The underlying integer value of this instance.
/// </summary>
public nint Value => _value;

/// <summary>
/// Returns a value indicating whether this instance is equal to a specified object.
/// </summary>
/// <param name="o">An object to compare with this instance.</param>
/// <returns><c>true</c> if <paramref name="o"/> is an instance of <see cref="CLong"/> and equals the value of this instance; otherwise, <c>false</c>.</returns>
public override bool Equals(object? o) => o is CLong other && Equals(other);

/// <summary>
/// Returns a value indicating whether this instance is equal to a specified <see cref="CLong"/> value.
/// </summary>
/// <param name="other">A <see cref="CLong"/> value to compare to this instance.</param>
/// <returns><c>true</c> if <paramref name="other"/> has the same value as this instance; otherwise, <c>false</c>.</returns>
public bool Equals(CLong other) => _value == other._value;

/// <summary>
/// Returns the hash code for this instance.
/// </summary>
/// <returns>A 32-bit signed integer hash code.</returns>
public override int GetHashCode() => _value.GetHashCode();

/// <summary>
/// Converts the numeric value of this instance to its equivalent string representation.
/// </summary>
/// <returns>The string representation of the value of this instance, consisting of a negative sign if the value is negative, and a sequence of digits ranging from 0 to 9 with no leading zeroes.</returns>
public override string ToString() => _value.ToString();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;

#pragma warning disable SA1121 // We use our own aliases since they differ per platform
#if TARGET_WINDOWS
using NativeType = System.UInt32;
#else
using NativeType = System.UIntPtr;
#endif

namespace System.Runtime.InteropServices
{
/// <summary>
/// <see cref="CULong"/> is an immutable value type that represents the <c>unsigned long</c> type in C and C++.
/// It is meant to be used as an exchange type at the managed/unmanaged boundary to accurately represent
/// in managed code unmanaged APIs that use the <c>unsigned long</c> type.
/// This type has 32-bits of storage on all Windows platforms and 32-bit Unix-based platforms.
/// It has 64-bits of storage on 64-bit Unix platforms.
/// </summary>
[CLSCompliant(false)]
[Intrinsic]
public readonly struct CULong : IEquatable<CULong>
{
private readonly NativeType _value;

/// <summary>
/// Constructs an instance from a 32-bit unsigned integer.
/// </summary>
/// <param name="value">The integer vaule.</param>
public CULong(uint value)
{
_value = (NativeType)value;
}
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Constructs an instance from a native sized unsigned integer.
/// </summary>
/// <param name="value">The integer vaule.</param>
/// <exception cref="OverflowException"><paramref name="value"/> is outside the range of the underlying storage type.</exception>
public CULong(nuint value)
{
_value = checked((NativeType)value);
}

/// <summary>
/// The underlying integer value of this instance.
/// </summary>
public nuint Value => _value;

/// <summary>
/// Returns a value indicating whether this instance is equal to a specified object.
/// </summary>
/// <param name="o">An object to compare with this instance.</param>
/// <returns><c>true</c> if <paramref name="o"/> is an instance of <see cref="CULong"/> and equals the value of this instance; otherwise, <c>false</c>.</returns>
public override bool Equals(object? o) => o is CULong other && Equals(other);

/// <summary>
/// Returns a value indicating whether this instance is equal to a specified <see cref="CLong"/> value.
/// </summary>
/// <param name="other">A <see cref="CULong"/> value to compare to this instance.</param>
/// <returns><c>true</c> if <paramref name="other"/> has the same value as this instance; otherwise, <c>false</c>.</returns>
public bool Equals(CULong other) => _value == other._value;

/// <summary>
/// Returns the hash code for this instance.
/// </summary>
/// <returns>A 32-bit signed integer hash code.</returns>
public override int GetHashCode() => _value.GetHashCode();

/// <summary>
/// Converts the numeric value of this instance to its equivalent string representation.
/// </summary>
/// <returns>The string representation of the value of this instance, consisting of a sequence of digits ranging from 0 to 9 with no leading zeroes.</returns>
public override string ToString() => _value.ToString();
}
}
Loading