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

Consider adding unmanaged pointer overloads for Marshal APIs #75630

Open
buyaa-n opened this issue Sep 14, 2022 · 7 comments
Open

Consider adding unmanaged pointer overloads for Marshal APIs #75630

buyaa-n opened this issue Sep 14, 2022 · 7 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@buyaa-n
Copy link
Member

buyaa-n commented Sep 14, 2022

I am wondering whether it would make sense to introduce overloads for PtrToStringUTF8 and similar APIs that take void* to avoid nint casts in these situations.

Originally posted by @jkotas in #75557 (comment)

APIs using IntPtr not included:

  • Marked with EditorBrowsableAttribute(EditorBrowsableState.Never) or Obsolete.
  • APIs where the IntPtr exists only in the return type.
  • Related APIs that would make arguments and/or return types inconsistent (for example, IntPtr ReAllocCoTaskMem(IntPtr pv, int cb) and IntPtr AllocCoTaskMem(int cb)).
namespace System.Runtime.InteropServices
{
    public static partial class Marshal
    {
+        public static unsafe int AddRef(void* pUnk);
+        public static unsafe void Copy(byte[] source, int startIndex, void* destination, int length);
+        public static unsafe void Copy(char[] source, int startIndex, void* destination, int length);
+        public static unsafe void Copy(double[] source, int startIndex, void* destination, int length);
+        public static unsafe void Copy(short[] source, int startIndex, void* destination, int length);
+        public static unsafe void Copy(int[] source, int startIndex, void* destination, int length);
+        public static unsafe void Copy(long[] source, int startIndex, void* destination, int length);
+        public static unsafe void Copy(void* source, byte[] destination, int startIndex, int length);
+        public static unsafe void Copy(void* source, char[] destination, int startIndex, int length);
+        public static unsafe void Copy(void* source, double[] destination, int startIndex, int length);
+        public static unsafe void Copy(void* source, short[] destination, int startIndex, int length);
+        public static unsafe void Copy(void* source, int[] destination, int startIndex, int length);
+        public static unsafe void Copy(void* source, long[] destination, int startIndex, int length);
+        public static unsafe void Copy(void* source, void*[] destination, int startIndex, int length);
+        public static unsafe void Copy(void* source, float[] destination, int startIndex, int length);
+        public static unsafe void Copy(void*[] source, int startIndex, void* destination, int length);
+        public static unsafe void Copy(float[] source, int startIndex, void* destination, int length);
+        [System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")]
+        public static unsafe void* CreateAggregatedObject<T>(void* pOuter, T o) where T : notnull;
+        public static unsafe void DestroyStructure<T>(void* ptr);
+        public static unsafe TDelegate GetDelegateForFunctionPointer<TDelegate>(void* ptr);
+        public static unsafe System.Exception? GetExceptionForHR(int errorCode, void* errorInfo);
+        [System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")]
+        public static unsafe object GetObjectForIUnknown(void* pUnk);
+        [System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")]
+        public static unsafe object GetTypedObjectForIUnknown(void* pUnk, System.Type t);
+        [System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")]
+        public static unsafe object GetUniqueObjectForIUnknown(void* unknown);
+        public static unsafe void InitHandle(SafeHandle safeHandle, IntPtr handle);
+        public static unsafe string? PtrToStringAnsi(void* ptr);
+        public static unsafe string PtrToStringAnsi(void* ptr, int len);
+        public static unsafe string? PtrToStringAuto(void* ptr);
+        public static unsafe string? PtrToStringAuto(void* ptr, int len);
+        public static unsafe string PtrToStringBSTR(void* ptr);
+        public static unsafe string? PtrToStringUni(void* ptr);
+        public static unsafe string PtrToStringUni(void* ptr, int len);
+        public static unsafe string? PtrToStringUTF8(void* ptr);
+        public static unsafe string PtrToStringUTF8(void* ptr, int byteLen);
+        public static unsafe T? PtrToStructure<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.NonPublicConstructors | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)]T>(void* ptr);
+        public static unsafe void PtrToStructure<T>(void* ptr, [System.Diagnostics.CodeAnalysis.DisallowNullAttribute] T structure);
+        public static unsafe int QueryInterface(void* pUnk, ref System.Guid iid, out void* ppv);
+        public static unsafe byte ReadByte(void* ptr);
+        public static unsafe byte ReadByte(void* ptr, int ofs);
+        public static unsafe short ReadInt16(void* ptr);
+        public static unsafe short ReadInt16(void* ptr, int ofs);
+        public static unsafe int ReadInt32(void* ptr);
+        public static unsafe int ReadInt32(void* ptr, int ofs);
+        public static unsafe long ReadInt64(void* ptr);
+        public static unsafe long ReadInt64(void* ptr, int ofs);
+        public static unsafe void* ReadIntPtr(void* ptr);
+        public static unsafe void* ReadIntPtr(void* ptr, int ofs);
+        public static unsafe int Release(void* pUnk);
+        public static unsafe void StructureToPtr<T>([System.Diagnostics.CodeAnalysis.DisallowNullAttribute] T structure, void* ptr, bool fDeleteOld);
+        public static unsafe void ThrowExceptionForHR(int errorCode, void* errorInfo);
+        public static unsafe void WriteByte(void* ptr, byte val);
+        public static unsafe void WriteByte(void* ptr, int ofs, byte val);
+        public static unsafe void WriteInt16(void* ptr, char val);
+        public static unsafe void WriteInt16(void* ptr, short val);
+        public static unsafe void WriteInt16(void* ptr, int ofs, char val);
+        public static unsafe void WriteInt16(void* ptr, int ofs, short val);
+        public static unsafe void WriteInt32(void* ptr, int val);
+        public static unsafe void WriteInt32(void* ptr, int ofs, int val);
+        public static unsafe void WriteInt64(void* ptr, int ofs, long val);
+        public static unsafe void WriteInt64(void* ptr, long val);
+        public static unsafe void WriteIntPtr(void* ptr, int ofs, void* val);
+        public static unsafe void WriteIntPtr(void* ptr, void* val);
    }
}
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 14, 2022
@ghost
Copy link

ghost commented Sep 14, 2022

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

I am wondering whether it would make sense to introduce overloads for PtrToStringUTF8 and similar APIs that take void* to avoid nint casts in these situations.

Originally posted by @jkotas in #75557 (comment)

Author: buyaa-n
Assignees: -
Labels:

area-System.Runtime.InteropServices, untriaged

Milestone: -

@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 8.0.0 milestone Sep 14, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Sep 14, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Consider adding Marshal.PtrToStringUTF8(void*); overload Consider adding unmanaged pointer overloads for Marshal APIs Sep 29, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Sep 29, 2022
@terrajobst
Copy link
Member

  • Let's take a more holistic pass on Marshal (and maybe related types) to see if there any other
namespace System.Runtime.InteropServices;

public partial class Marshal
{
    public static string PtrToStringAnsi(void* str);
    public static string PtrToStringAnsi(void* str, int count);
    public static string PtrToStringAuto(void* str);
    public static string PtrToStringAuto(void* str, int count);
    public static string PtrToStringBSTR(void* str);
    public static string PtrToStringUni(void* str);
    public static string PtrToStringUni(void* str, int count);
    public static string PtrToStringUTF8(void* str);
    public static string PtrToStringUTF8(void* str, int byteLength);
}

@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 4, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Oct 4, 2022
@jkotas
Copy link
Member

jkotas commented Oct 18, 2022

Marshal.Copy
Marshal.ReadByte/Int16/Int32/Int64
Marshal.WriteByte/Int16/Int32/Int64

We have better equivalents elsewhere (e.g. direct unsafe pointer operations, Span.CopyTo). Is it worth it to introduce these overloads? Would it better to encourage use of the better equivalents instead?

void InitHandle(SafeHandle safeHandle, IntPtr handle);

Should the argument type be void*? What about all other methods on SafeHandle that take or return IntPtr?

APIs that I find missing in the list:

  • COM memory - Marshal.AllocCoTaskMem, etc. (NativeMemory.Alloc is not full fidelity replacement)
  • Unmanaged string allocations - Marhal.StringToCoTaskMemUTF8, etc.

cc @AaronRobinsonMSFT

@AaronRobinsonMSFT
Copy link
Member

We have better equivalents elsewhere (e.g. direct unsafe pointer operations, Span.CopyTo). Is it worth it to introduce these overloads? Would it better to encourage use of the better equivalents instead?

That is going to be part of the API review. I simply copied all APIs on Marshal that we could potentially update, skipping some based on other constraints - see the list in the description.

APIs that I find missing in the list:

Yep. This was intentionally skipped. I updated the issue with those details. APIs that can't be updated to match a pair weren't. For example, Marshal.AllocCoTaskMem() returns IntPtr, but we can't provide a new API because only the return would change and that isn't considered on overload resolution. This means that Marshal.FreeCoTaskMem() doesn't make the cut because I didn't want to introduce that asymmetry.

@bartonjs
Copy link
Member

bartonjs commented Oct 18, 2022

Video

  • We feel that the Read* and Write* APIs won't ever get called, since someone already working with pointers is almost certainly going to write *(byte*)ptr instead of Marshal.ReadByte(ptr).
  • There's a question of if StructureToPtr does unique work (string marshalling?) or is just a fancy Write. If it's literally the same as *ptr = struct then it should be removed.
  • The rest look good as proposed.
namespace System.Runtime.InteropServices
{
    public static partial class Marshal
    {
         public static unsafe int AddRef(void* pUnk);
         public static unsafe void Copy(byte[] source, int startIndex, void* destination, int length);
         public static unsafe void Copy(char[] source, int startIndex, void* destination, int length);
         public static unsafe void Copy(double[] source, int startIndex, void* destination, int length);
         public static unsafe void Copy(short[] source, int startIndex, void* destination, int length);
         public static unsafe void Copy(int[] source, int startIndex, void* destination, int length);
         public static unsafe void Copy(long[] source, int startIndex, void* destination, int length);
         public static unsafe void Copy(void* source, byte[] destination, int startIndex, int length);
         public static unsafe void Copy(void* source, char[] destination, int startIndex, int length);
         public static unsafe void Copy(void* source, double[] destination, int startIndex, int length);
         public static unsafe void Copy(void* source, short[] destination, int startIndex, int length);
         public static unsafe void Copy(void* source, int[] destination, int startIndex, int length);
         public static unsafe void Copy(void* source, long[] destination, int startIndex, int length);
         public static unsafe void Copy(void* source, void*[] destination, int startIndex, int length);
         public static unsafe void Copy(void* source, float[] destination, int startIndex, int length);
         public static unsafe void Copy(void*[] source, int startIndex, void* destination, int length);
         public static unsafe void Copy(float[] source, int startIndex, void* destination, int length);
         [System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")]
         public static unsafe void* CreateAggregatedObject<T>(void* pOuter, T o) where T : notnull;
         public static unsafe void DestroyStructure<T>(void* ptr);
         public static unsafe TDelegate GetDelegateForFunctionPointer<TDelegate>(void* ptr);
         public static unsafe System.Exception? GetExceptionForHR(int errorCode, void* errorInfo);
         [System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")]
         public static unsafe object GetObjectForIUnknown(void* pUnk);
         [System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")]
         public static unsafe object GetTypedObjectForIUnknown(void* pUnk, System.Type t);
         [System.Runtime.Versioning.SupportedOSPlatformAttribute("windows")]
         public static unsafe object GetUniqueObjectForIUnknown(void* unknown);
         public static unsafe void InitHandle(SafeHandle safeHandle, IntPtr handle);
         public static unsafe string? PtrToStringAnsi(void* ptr);
         public static unsafe string PtrToStringAnsi(void* ptr, int len);
         public static unsafe string? PtrToStringAuto(void* ptr);
         public static unsafe string? PtrToStringAuto(void* ptr, int len);
         public static unsafe string PtrToStringBSTR(void* ptr);
         public static unsafe string? PtrToStringUni(void* ptr);
         public static unsafe string PtrToStringUni(void* ptr, int len);
         public static unsafe string? PtrToStringUTF8(void* ptr);
         public static unsafe string PtrToStringUTF8(void* ptr, int byteLen);
         public static unsafe T? PtrToStructure<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.NonPublicConstructors | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)]T>(void* ptr);
         public static unsafe void PtrToStructure<T>(void* ptr, [System.Diagnostics.CodeAnalysis.DisallowNullAttribute] T structure);
         public static unsafe int QueryInterface(void* pUnk, ref System.Guid iid, out void* ppv);
         public static unsafe int Release(void* pUnk);
         public static unsafe void StructureToPtr<T>([System.Diagnostics.CodeAnalysis.DisallowNullAttribute] T structure, void* ptr, bool fDeleteOld);
         public static unsafe void ThrowExceptionForHR(int errorCode, void* errorInfo);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 18, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT modified the milestones: 8.0.0, 9.0.0 Jul 26, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT added the help wanted [up-for-grabs] Good issue for external contributors label Sep 25, 2023
@Neme12
Copy link

Neme12 commented Apr 1, 2024

Shouldn't PtrToStringUni take char* instead of void*? I mean the length is length in chars, not bytes, I assume. And PtrToStringUTF8 take byte*, etc? The copy ones take the pointer type of the type they're taking as source or destination? And many other APIs similarly as well take the appropriate typed pointer? Otherwise it's unclear what length means.

@jkoritzinsky jkoritzinsky modified the milestones: 9.0.0, 10.0.0 Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices help wanted [up-for-grabs] Good issue for external contributors
Projects
Status: No status
Development

No branches or pull requests

8 participants