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

Don't block specific generic types in field marshalling scenarios. #31679

Merged
merged 9 commits into from Feb 5, 2020

Conversation

@jkoritzinsky
Copy link
Member

jkoritzinsky commented Feb 3, 2020

Bing is using Marshal.SizeOf to calculate the size of structs that contain a Nullable<T> field. In the old system, this would unexpectedly work since we apparently allowed using generic structs as fields in interop types (but not generic classes). See the && ELEMENT_TYPE_CLASS at the end of the condition on line 208: https://github.com/dotnet/coreclr/blob/release/3.1/src/vm/fieldmarshaler.cpp#L208-L211

Semantically, the correct function for users to call here would be Unsafe.SizeOf<T>, but there's a large number of users who use Marshal.SizeOf instead of Unsafe.SizeOf for various reasons.

Fixes #31640

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Feb 3, 2020

Test?

@jkoritzinsky

This comment has been minimized.

Copy link
Member Author

jkoritzinsky commented Feb 3, 2020

Added

@jkotas
jkotas approved these changes Feb 3, 2020
Copy link
Member

jkotas left a comment

Thanks!

…ios and still block Vector types.
@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Feb 4, 2020

Vector64/128/256 need to be blocked even in the field scenario.

.NET Core 3.x shipped without this block. E.g. the following works fine in .NET Core 3.x:

using System;
using System.Runtime.InteropServices;
using System.Runtime.Intrinsics;

struct Foo
{
    Vector64<int> f;
}

class Program
{
    static void Main(string[] args)
    {
        Console.WriteLine(Marshal.SizeOf<Foo>());
    }
}

Is adding the block for these a breaking change?

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Feb 4, 2020

.NET Core 3.x shipped without this block.

Was it allowed to cross the P/Invoke boundary or just allowed in functions like Marshal.SizeOf<T>()?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Feb 4, 2020

It was allowed to cross P/Invoke boundary too.

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Feb 4, 2020

That's unfortunate. The intent was that it shouldn't be allowed as struct Mat4x4 { Vector128<float> x, y, z, w; } has special ABI requirements on some platforms (it can be passed in 4 registers; rather than by reference).

@jkoritzinsky

This comment has been minimized.

Copy link
Member Author

jkoritzinsky commented Feb 4, 2020

I take it based on this feedback that we want to switch this back to only explicitly blocking ByReference<T> in field scenarios?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Feb 4, 2020

For this PR, we should make it do what it did in .NET Core 3.0.

If you would like to propose breaking changes against the .NET Core 3.0 behavior, it should be separate issue / PR.

@jkoritzinsky

This comment has been minimized.

Copy link
Member Author

jkoritzinsky commented Feb 4, 2020

Sounds good to me.

src/coreclr/src/vm/mlinfo.cpp Outdated Show resolved Hide resolved
@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Feb 4, 2020

Could you please add tests for the different cases we have discussed above?

@jkoritzinsky

This comment has been minimized.

Copy link
Member Author

jkoritzinsky commented Feb 4, 2020

Will do!

@MichalStrehovsky

This comment has been minimized.

Copy link
Member

MichalStrehovsky commented Feb 4, 2020

Could you also match this with MarshalHelpers.cs in Crossgen2?

// Blittable generics are allowed to be marshalled with the following exceptions:
// * ByReference<T>: This represents an interior pointer and is not actually blittable
// * Nullable<T>: We don't want to be locked into the default behavior as we may want special handling later
// * Vector64<T>: Represents the __m64 ABI primitive which requires currently unimplemented handling
// * Vector128<T>: Represents the __m128 ABI primitive which requires currently unimplemented handling
// * Vector256<T>: Represents the __m256 ABI primitive which requires currently unimplemented handling
// * Vector<T>: Has a variable size (either __m128 or __m256) and isn't readily usable for inteorp scenarios
if (type.HasInstantiation && (!isBlittable
|| InteropTypes.IsSystemByReference(context, type)
|| InteropTypes.IsSystemNullable(context, type)
|| InteropTypes.IsSystemRuntimeIntrinsicsVector64T(context, type)
|| InteropTypes.IsSystemRuntimeIntrinsicsVector128T(context, type)
|| InteropTypes.IsSystemRuntimeIntrinsicsVector256T(context, type)
|| InteropTypes.IsSystemNumericsVectorT(context, type)))
{
// Generic types cannot be marshaled.
return MarshallerKind.Invalid;
}

If you need to narrow down to field marshalling only, the marshallerType parameter has it.

@jkoritzinsky

This comment has been minimized.

Copy link
Member Author

jkoritzinsky commented Feb 5, 2020

Linux job failure is an intermittent issue tracked with AzDO.
MacOS failure is #2176

I'm going to merge this in so we can unblock Bing.

@jkoritzinsky jkoritzinsky merged commit 7e12819 into dotnet:master Feb 5, 2020
92 of 95 checks passed
92 of 95 checks passed
runtime Build #20200204.77 had test failures
Details
runtime (Libraries Test Run checked coreclr Linux x64 Debug) Libraries Test Run checked coreclr Linux x64 Debug failed
Details
runtime (Libraries Test Run release coreclr OSX x64 Debug) Libraries Test Run release coreclr OSX x64 Debug failed
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
runtime (Checkout) Checkout succeeded
Details
runtime (CoreCLR Pri0 Test Build Linux arm checked) CoreCLR Pri0 Test Build Linux arm checked succeeded
Details
runtime (CoreCLR Pri0 Test Build Linux arm64 checked) CoreCLR Pri0 Test Build Linux arm64 checked succeeded
Details
runtime (CoreCLR Pri0 Test Build Linux x64 checked) CoreCLR Pri0 Test Build Linux x64 checked succeeded
Details
runtime (CoreCLR Pri0 Test Build OSX x64 checked) CoreCLR Pri0 Test Build OSX x64 checked succeeded
Details
runtime (CoreCLR Pri0 Test Build Windows_NT arm checked) CoreCLR Pri0 Test Build Windows_NT arm checked succeeded
Details
runtime (CoreCLR Pri0 Test Build Windows_NT arm64 checked) CoreCLR Pri0 Test Build Windows_NT arm64 checked succeeded
Details
runtime (CoreCLR Pri0 Test Build Windows_NT x64 checked) CoreCLR Pri0 Test Build Windows_NT x64 checked succeeded
Details
runtime (CoreCLR Pri0 Test Build Windows_NT x86 checked) CoreCLR Pri0 Test Build Windows_NT x86 checked succeeded
Details
runtime (CoreCLR Pri0 Test Run Linux arm checked) CoreCLR Pri0 Test Run Linux arm checked succeeded
Details
runtime (CoreCLR Pri0 Test Run Linux arm64 checked) CoreCLR Pri0 Test Run Linux arm64 checked succeeded
Details
runtime (CoreCLR Pri0 Test Run Linux x64 checked) CoreCLR Pri0 Test Run Linux x64 checked succeeded
Details
runtime (CoreCLR Pri0 Test Run OSX x64 checked) CoreCLR Pri0 Test Run OSX x64 checked succeeded
Details
runtime (CoreCLR Pri0 Test Run Windows_NT arm checked) CoreCLR Pri0 Test Run Windows_NT arm checked succeeded
Details
runtime (CoreCLR Pri0 Test Run Windows_NT x64 checked) CoreCLR Pri0 Test Run Windows_NT x64 checked succeeded
Details
runtime (CoreCLR Pri0 Test Run Windows_NT x86 checked) CoreCLR Pri0 Test Run Windows_NT x86 checked succeeded
Details
runtime (CoreCLR Product Build Linux arm checked) CoreCLR Product Build Linux arm checked succeeded
Details
runtime (CoreCLR Product Build Linux arm release) CoreCLR Product Build Linux arm release succeeded
Details
runtime (CoreCLR Product Build Linux arm64 checked) CoreCLR Product Build Linux arm64 checked succeeded
Details
runtime (CoreCLR Product Build Linux arm64 release) CoreCLR Product Build Linux arm64 release succeeded
Details
runtime (CoreCLR Product Build Linux x64 checked) CoreCLR Product Build Linux x64 checked succeeded
Details
runtime (CoreCLR Product Build Linux x64 release) CoreCLR Product Build Linux x64 release succeeded
Details
runtime (CoreCLR Product Build Linux_musl arm64 release) CoreCLR Product Build Linux_musl arm64 release succeeded
Details
runtime (CoreCLR Product Build Linux_musl x64 checked) CoreCLR Product Build Linux_musl x64 checked succeeded
Details
runtime (CoreCLR Product Build Linux_musl x64 release) CoreCLR Product Build Linux_musl x64 release succeeded
Details
runtime (CoreCLR Product Build OSX x64 checked) CoreCLR Product Build OSX x64 checked succeeded
Details
runtime (CoreCLR Product Build OSX x64 release) CoreCLR Product Build OSX x64 release succeeded
Details
runtime (CoreCLR Product Build Windows_NT arm checked) CoreCLR Product Build Windows_NT arm checked succeeded
Details
runtime (CoreCLR Product Build Windows_NT arm release) CoreCLR Product Build Windows_NT arm release succeeded
Details
runtime (CoreCLR Product Build Windows_NT arm64 checked) CoreCLR Product Build Windows_NT arm64 checked succeeded
Details
runtime (CoreCLR Product Build Windows_NT arm64 release) CoreCLR Product Build Windows_NT arm64 release succeeded
Details
runtime (CoreCLR Product Build Windows_NT x64 checked) CoreCLR Product Build Windows_NT x64 checked succeeded
Details
runtime (CoreCLR Product Build Windows_NT x64 release) CoreCLR Product Build Windows_NT x64 release succeeded
Details
runtime (CoreCLR Product Build Windows_NT x86 checked) CoreCLR Product Build Windows_NT x86 checked succeeded
Details
runtime (CoreCLR Product Build Windows_NT x86 release) CoreCLR Product Build Windows_NT x86 release succeeded
Details
runtime (Formatting Linux x64) Formatting Linux x64 succeeded
Details
runtime (Formatting Windows_NT x64) Formatting Windows_NT x64 succeeded
Details
runtime (Installer Build and Test Linux_arm Debug) Installer Build and Test Linux_arm Debug succeeded
Details
runtime (Installer Build and Test Linux_arm64 Debug) Installer Build and Test Linux_arm64 Debug succeeded
Details
runtime (Installer Build and Test Linux_musl_arm64 Debug) Installer Build and Test Linux_musl_arm64 Debug succeeded
Details
runtime (Installer Build and Test Linux_musl_x64 Debug) Installer Build and Test Linux_musl_x64 Debug succeeded
Details
runtime (Installer Build and Test Linux_x64 Debug) Installer Build and Test Linux_x64 Debug succeeded
Details
runtime (Installer Build and Test OSX_x64 Debug) Installer Build and Test OSX_x64 Debug succeeded
Details
runtime (Installer Build and Test Windows_NT_arm Debug) Installer Build and Test Windows_NT_arm Debug succeeded
Details
runtime (Installer Build and Test Windows_NT_arm64 Debug) Installer Build and Test Windows_NT_arm64 Debug succeeded
Details
runtime (Installer Build and Test Windows_NT_x64 Debug) Installer Build and Test Windows_NT_x64 Debug succeeded
Details
runtime (Installer Build and Test Windows_NT_x86 Debug) Installer Build and Test Windows_NT_x86 Debug succeeded
Details
runtime (Libraries Build Linux arm Release) Libraries Build Linux arm Release succeeded
Details
runtime (Libraries Build Linux arm64 Debug) Libraries Build Linux arm64 Debug succeeded
Details
runtime (Libraries Build Linux x64 Debug) Libraries Build Linux x64 Debug succeeded
Details
runtime (Libraries Build Linux_musl arm64 Release) Libraries Build Linux_musl arm64 Release succeeded
Details
runtime (Libraries Build Linux_musl x64 Debug) Libraries Build Linux_musl x64 Debug succeeded
Details
runtime (Libraries Build OSX x64 Debug) Libraries Build OSX x64 Debug succeeded
Details
runtime (Libraries Build WebAssembly wasm Debug) Libraries Build WebAssembly wasm Debug succeeded
Details
runtime (Libraries Build Windows_NT allConfigurations x64 Debug) Libraries Build Windows_NT allConfigurations x64 Debug succeeded
Details
runtime (Libraries Build Windows_NT arm Release) Libraries Build Windows_NT arm Release succeeded
Details
runtime (Libraries Build Windows_NT arm64 Release) Libraries Build Windows_NT arm64 Release succeeded
Details
runtime (Libraries Build Windows_NT net472 x86 Release) Libraries Build Windows_NT net472 x86 Release succeeded
Details
runtime (Libraries Build Windows_NT x64 Debug) Libraries Build Windows_NT x64 Debug succeeded
Details
runtime (Libraries Build Windows_NT x86 Debug) Libraries Build Windows_NT x86 Debug succeeded
Details
runtime (Libraries Build Windows_NT x86 Release) Libraries Build Windows_NT x86 Release succeeded
Details
runtime (Libraries Test Build Linux x64 Debug) Libraries Test Build Linux x64 Debug succeeded
Details
runtime (Libraries Test Build OSX x64 Debug) Libraries Test Build OSX x64 Debug succeeded
Details
runtime (Libraries Test Build Windows_NT x64 Debug) Libraries Test Build Windows_NT x64 Debug succeeded
Details
runtime (Libraries Test Run checked coreclr Linux_musl x64 Debug) Libraries Test Run checked coreclr Linux_musl x64 Debug succeeded
Details
runtime (Libraries Test Run checked coreclr OSX x64 Debug) Libraries Test Run checked coreclr OSX x64 Debug succeeded
Details
runtime (Libraries Test Run checked coreclr Windows_NT x64 Debug) Libraries Test Run checked coreclr Windows_NT x64 Debug succeeded
Details
runtime (Libraries Test Run checked coreclr Windows_NT x86 Release) Libraries Test Run checked coreclr Windows_NT x86 Release succeeded
Details
runtime (Libraries Test Run release coreclr Linux x64 Debug) Libraries Test Run release coreclr Linux x64 Debug succeeded
Details
runtime (Libraries Test Run release coreclr Linux_musl x64 Debug) Libraries Test Run release coreclr Linux_musl x64 Debug succeeded
Details
runtime (Libraries Test Run release coreclr Windows_NT x64 Debug) Libraries Test Run release coreclr Windows_NT x64 Debug succeeded
Details
runtime (Libraries Test Run release coreclr Windows_NT x86 Debug) Libraries Test Run release coreclr Windows_NT x86 Debug succeeded
Details
runtime (Libraries Test Run release coreclr Windows_NT x86 Release) Libraries Test Run release coreclr Windows_NT x86 Release succeeded
Details
runtime (Libraries Test Run release mono Linux x64 Debug) Libraries Test Run release mono Linux x64 Debug succeeded
Details
runtime (Libraries Test Run release mono OSX x64 Debug) Libraries Test Run release mono OSX x64 Debug succeeded
Details
runtime (Mono Product Build Linux x64 debug) Mono Product Build Linux x64 debug succeeded
Details
runtime (Mono Product Build Linux x64 release) Mono Product Build Linux x64 release succeeded
Details
runtime (Mono Product Build Linux_musl x64 debug) Mono Product Build Linux_musl x64 debug succeeded
Details
runtime (Mono Product Build Linux_musl x64 release) Mono Product Build Linux_musl x64 release succeeded
Details
runtime (Mono Product Build OSX x64 debug) Mono Product Build OSX x64 debug succeeded
Details
runtime (Mono Product Build OSX x64 release) Mono Product Build OSX x64 release succeeded
Details
runtime (Mono Product Build Windows_NT x64 debug) Mono Product Build Windows_NT x64 debug succeeded
Details
runtime (Mono Product Build Windows_NT x64 release) Mono Product Build Windows_NT x64 release succeeded
Details
runtime (Test crossgen-comparison Linux arm checked) Test crossgen-comparison Linux arm checked succeeded
Details
runtime-live-build Build #20200204.70 succeeded
Details
runtime-live-build (Build Linux x64 debug RuntimeFlavor_Mono) Build Linux x64 debug RuntimeFlavor_Mono succeeded
Details
runtime-live-build (Build Linux x64 debug Runtime_Release) Build Linux x64 debug Runtime_Release succeeded
Details
runtime-live-build (Build OSX x64 release Runtime_Debug) Build OSX x64 release Runtime_Debug succeeded
Details
runtime-live-build (Build Windows_NT x86 release Runtime_Debug) Build Windows_NT x86 release Runtime_Debug succeeded
Details
runtime-live-build (Checkout) Checkout succeeded
Details
@jkoritzinsky jkoritzinsky deleted the jkoritzinsky:marshal-sizeof-nullable branch Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.