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

Add Marshal API test coverage for generics marshalling #35193

Closed
MichalStrehovsky opened this issue Apr 20, 2020 · 4 comments · Fixed by #55533
Closed

Add Marshal API test coverage for generics marshalling #35193

MichalStrehovsky opened this issue Apr 20, 2020 · 4 comments · Fixed by #55533
Labels
area-Interop-coreclr help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Apr 20, 2020

#103 added support for marshalling generics, but we don't seem to have test coverage for the Marshal APIs with this:

  • Marshal.OffsetOf
  • Marshal.SizeOf
  • Marshal.PtrToStructure
  • Marshal.StructureToPtr
  • Others?

E.g. Marshal.OffsetOf already worked on desktop CLR because the runtime didn't do a good job of blocking it (and it also raises a question as to whether the code paths this is taking are quite different that made it so that blocking didn't take place). We don't have tests that the returned numbers are actually good.

We might as well throw in a test for marshalling as UnmanagedType.AsAny because I'm sure WinForms is keen to use that (/snark).

I'm making this a .NET 5 issue since this is a new feature.

@MichalStrehovsky MichalStrehovsky added this to the 5.0 milestone Apr 20, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Apr 20, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Jun 8, 2020
@elinor-fung
Copy link
Member

Moving to 6.0 as test debt we need to address.

@elinor-fung elinor-fung modified the milestones: 5.0.0, 6.0.0 Aug 17, 2020
@elinor-fung elinor-fung added the help wanted [up-for-grabs] Good issue for external contributors label May 11, 2021
@jkoritzinsky
Copy link
Member

I've validated that we already have coverage for SizeOf, PtrToStructure and StructureToPtr. We never enabled generic support for these APIs.

As mentioned in the original issue, we don't have tests for OffsetOf and it seems to allow generic types. I'll add some tests for OffsetOf since I don't think we can break back-compat here (doesn't meet the bar).

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 12, 2021
@elinor-fung
Copy link
Member

I've validated that we already have coverage for SizeOf, PtrToStructure and StructureToPtr. We never enabled generic support for these APIs.

Does this include versions of SizeOf and PtrToStructure that take the actual object? I think they also work with blittable generics (in Framework too).

public static int SizeOf(object structure)
{
if (structure is null)
{
throw new ArgumentNullException(nameof(structure));
}
return SizeOfHelper(structure.GetType(), throwIfNotMarshalable: true);
}
public static int SizeOf<T>(T structure)
{
if (structure is null)
{
throw new ArgumentNullException(nameof(structure));
}
return SizeOfHelper(structure.GetType(), throwIfNotMarshalable: true);
}

public static void PtrToStructure(IntPtr ptr, object structure)
{
PtrToStructureHelper(ptr, structure, allowValueClasses: false);
}
public static void PtrToStructure<T>(IntPtr ptr, [DisallowNull] T structure)
{
PtrToStructureHelper(ptr, structure, allowValueClasses: false);
}

@jkoritzinsky
Copy link
Member

It looks like we also don't block those. I'll add some testing for those cases 😢

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Interop-coreclr help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants