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

Support decimal marshalling #1101

Merged
merged 9 commits into from May 12, 2021
Merged

Support decimal marshalling #1101

merged 9 commits into from May 12, 2021

Conversation

hez2010
Copy link

@hez2010 hez2010 commented May 9, 2021

Support decimal marshalling.

Contributes to #175
Fixes #969

@hez2010
Copy link
Author

hez2010 commented May 9, 2021

Oops seems that it only works on Windows. Maybe caused by paddings or endians, need to figure out why it doesn't work on other platforms.

@jkotas
Copy link
Member

jkotas commented May 9, 2021

The layout of native decimal is the same with managed decimal struct,

The alignment requirements of the managed Decimal and the unmanaged DECIMAL are different. CoreCLR solves it using https://github.com/dotnet/runtime/blob/main/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs#L1082

@kant2002
Copy link
Contributor

kant2002 commented May 9, 2021

You can try modify dirs.proj by replacing lines

https://github.com/dotnet/runtimelab/blob/feature/NativeAOT/src/tests/Common/dirs.proj#L36-L38

with

<AllProjects Include="$(TestRoot)Interop/PInvoke/Decimal/DecimalTest.csproj" Exclude="@(DisabledProjects)" />

and run src\tests\build.cmd nativeaot Debug /p:SmokeTestsOnly=true
then you can run existing tests using workflow for individual tests from https://github.com/dotnet/runtimelab/blob/feature/NativeAOT/docs/workflow/building/coreclr/nativeaot.md#running-tests

@hez2010
Copy link
Author

hez2010 commented May 10, 2021

@jkotas PTAL. CI failure is unrelated to this PR.

var mid32 = (int)((lo64 & 0xFFFFFFFF00000000) >> 32);
var lo32 = (int)(lo64 & 0xFFFFFFFF);

return new decimal(lo32, mid32, (int)hi32, sign != 0, scale);
Copy link
Member

Choose a reason for hiding this comment

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

Calling this constructor will do some extra argument validation for scale. I do not see it in the CoreCLR decimal marshalers. CoreCLR marshalers just blasts the bits over. It would be nice to match what CoreCLR does.

@jkotas
Copy link
Member

jkotas commented May 10, 2021

I would expect the NativeDecimal type to show up in more places (e.g. also in GetNativeTypeFromMarshallerKind).

I am actually not sure anymore whether the NativeDecimal type is actually required to deal with the alignment issues. It was required for the old decimal implementation: https://github.com/microsoft/referencesource/blob/5697c29004a34d80acdaf5742d7e699022c64ecd/mscorlib/system/decimal.cs#L145-L148 , but it may not be required for the current one: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Decimal.cs#L106-L108.

What is not going to work if https://github.com/dotnet/runtimelab/pull/1101/files#diff-f8ce84af95c61383c81c774a30efbded7030042bc018027326e1f2c33ad00b22R83 is just changed to return new BlittableValueMarshaller ?

@hez2010
Copy link
Author

hez2010 commented May 11, 2021

Actually I don't see the need of NativeDecimal. In current implementation of System.Decimal, the layout completely matches the unmanaged DECIMAL.
So I think it's okay to use StructMarshaller?

@jkotas
Copy link
Member

jkotas commented May 11, 2021

Yes, I think it should be ok to use StructMarshaller for decimal.

@hez2010
Copy link
Author

hez2010 commented May 11, 2021

Updated.
Does GetNativeTypeFromMarshallerKind also need to be updated to return interopStateManager.GetStructMarshallingNativeType((MetadataType)type) for decimals?
Update: I don't think it's needed.

@hez2010
Copy link
Author

hez2010 commented May 11, 2021

@jkotas I think it's ready for another look.

@jkotas
Copy link
Member

jkotas commented May 11, 2021

Could you please also re-enable the decimal tests by deleting https://github.com/dotnet/runtimelab/blob/feature/NativeAOT/src/tests/issues.targets#L1139-L1141 ?

@hez2010
Copy link
Author

hez2010 commented May 11, 2021

Done.

@hez2010
Copy link
Author

hez2010 commented May 11, 2021

The CI failure seems to be an I/O issue while copying files.

D:\workspace\_work\1\s\.dotnet\sdk\6.0.100-preview.3.21202.5\Microsoft.Common.CurrentVersion.targets(4502,5): error MSB3026: Could not copy "D:\workspace\_work\1\s\artifacts\obj\System.Collections.NonGeneric\ref\net6.0-Release\System.Collections.NonGeneric.dll" to "D:\workspace\_work\1\s\artifacts\bin\System.Collections.NonGeneric\ref\net6.0-Release\System.Collections.NonGeneric.dll". Beginning retry 1 in 1000ms. The requested operation cannot be performed on a file with a user-mapped section open. : 'D:\workspace\_work\1\s\artifacts\bin\System.Collections.NonGeneric\ref\net6.0-Release\System.Collections.NonGeneric.dll'  [D:\workspace\_work\1\s\src\libraries\System.Collections.NonGeneric\ref\System.Collections.NonGeneric.csproj]

@jkotas
Copy link
Member

jkotas commented May 12, 2021

Contributes to #175

Unfortunately, this is not fully fixed yet. (It fails on BlittableStructPtrMarshaller now.)

@@ -353,7 +353,7 @@ public static partial class MarshalHelpers
{
if (nativeType == NativeTypeKind.Struct || nativeType == NativeTypeKind.Default)
return MarshallerKind.Decimal;
else if (nativeType == NativeTypeKind.LPStruct && !isField && !isReturn)
else if (nativeType == NativeTypeKind.LPStruct && !isField)
Copy link
Member

Choose a reason for hiding this comment

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

Minor fix to match CoreCLR behavior that gets us a bit closer.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit 6d85697 into dotnet:feature/NativeAOT May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants