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

[API Proposal]: Provide BSTR marshaller for LibraryImport source generator #69021

Closed
AaronRobinsonMSFT opened this issue May 8, 2022 · 8 comments · Fixed by #69213
Closed
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Milestone

Comments

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented May 8, 2022

Background and motivation

Add BSTR marshaller in addition to new string marshallers in #67635. Since this marshaller is for adoption purposes it is similar to the existing ANSI marshaller and will require users to manually define it using MarshalUsing attribute or can rely on the UnmanagedType.BStr value in a MarshalAs.

API Proposal

Implementation PR - #69213

namespace System.Runtime.InteropServices.Marshalling
{
    /// <summary>
    /// Marshaller for BSTR strings
    /// </summary>
    [CLSCompliant(false)]
    [CustomTypeMarshaller(typeof(string), BufferSize = 0x100,
        Features = CustomTypeMarshallerFeatures.UnmanagedResources | CustomTypeMarshallerFeatures.TwoStageMarshalling | CustomTypeMarshallerFeatures.CallerAllocatedBuffer)]
    public unsafe ref struct BStrStringMarshaller
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="BStrStringMarshaller"/>.
        /// </summary>
        /// <param name="str">The string to marshal.</param>
        public BStrStringMarshaller(string? str);

        /// <summary>
        /// Initializes a new instance of the <see cref="BStrStringMarshaller"/>.
        /// </summary>
        /// <param name="str">The string to marshal.</param>
        /// <param name="buffer">Buffer that may be used for marshalling.</param>
        /// <remarks>
        /// The <paramref name="buffer"/> must not be movable - that is, it should not be
        /// on the managed heap or it should be pinned.
        /// <seealso cref="CustomTypeMarshallerFeatures.CallerAllocatedBuffer"/>
        /// </remarks>
        public BStrStringMarshaller(string? str, Span<ushort> buffer);

        /// <summary>
        /// Returns the native value representing the string.
        /// </summary>
        /// <remarks>
        /// <seealso cref="CustomTypeMarshallerFeatures.TwoStageMarshalling"/>
        /// </remarks>
        public void* ToNativeValue();

        /// <summary>
        /// Sets the native value representing the string.
        /// </summary>
        /// <param name="value">The native value.</param>
        /// <remarks>
        /// <seealso cref="CustomTypeMarshallerFeatures.TwoStageMarshalling"/>
        /// </remarks>
        public void FromNativeValue(void* value);

        /// <summary>
        /// Returns the managed string.
        /// </summary>
        /// <remarks>
        /// <seealso cref="CustomTypeMarshallerDirection.Out"/>
        /// </remarks>
        public string? ToManaged();

        /// <summary>
        /// Frees native resources.
        /// </summary>
        /// <remarks>
        /// <seealso cref="CustomTypeMarshallerFeatures.UnmanagedResources"/>
        /// </remarks>
        public void FreeNative();
    }
}

API Usage

The following is an example from aspnetcore where this new marshaller could be used.

[LibraryImport(AspNetCoreModuleDll)]
private static partial int http_get_server_variable(
    NativeSafeHandle pInProcessHandler,
    [MarshalAs(UnmanagedType.LPStr)] string variableName,
    [MarshalUsing(typeof(BStrStringMarshaller))] out string value);

// or

[LibraryImport(AspNetCoreModuleDll)]
private static partial int http_get_server_variable(
    NativeSafeHandle pInProcessHandler,
    [MarshalAs(UnmanagedType.LPStr)] string variableName,
    [MarshalAs(UnmanagedType.BStr)] out string value);

Alternative Designs

No response

Risks

None. This is a new opt-in API.

@AaronRobinsonMSFT AaronRobinsonMSFT added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 8, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 8, 2022
@ghost
Copy link

ghost commented May 8, 2022

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

Issue Details

Background and motivation

Add BSTR marshaller in addition to new string marshallers in #67635. Since this marshaller is for adoption purposes it is similar to the existing ANSI marshaller and will require users to manually define it using MarshalUsing attribute.

API Proposal

namespace   namespace System.Runtime.InteropServices.Marshalling
{
    /// <summary>
    /// Marshaller for BSTR strings
    /// </summary>
    [CustomTypeMarshaller(typeof(string),
        Features = CustomTypeMarshallerFeatures.UnmanagedResources | CustomTypeMarshallerFeatures.TwoStageMarshalling)]
    internal ref struct BstrStringMarshaller
    {
        private IntPtr _allocated;

        public BstrStringMarshaller(string? str)
        {
            _allocated = Marshal.StringToBSTR(str);
        }

        public IntPtr ToNativeValue() => _allocated;

        public void FromNativeValue(IntPtr value) => _allocated = value;

        public string? ToManaged() => Marshal.PtrToStringBSTR(_allocated);

        public void FreeNative()
        {
            if (_allocated != IntPtr.Zero)
            {
                Marshal.FreeBSTR(_allocated);
            }
        }
    }
}

API Usage

The following is an example from aspnetcore where this new marshaller could be used.

[LibraryImport(AspNetCoreModuleDll)]
private static partial int http_get_server_variable(
    NativeSafeHandle pInProcessHandler,
    [MarshalAs(UnmanagedType.LPStr)] string variableName,
    [MarshalUsing(typeof(BstrStringMarshaller))] out string value);

Alternative Designs

No response

Risks

None. This is a new opt-in API.

Author: AaronRobinsonMSFT
Assignees: -
Labels:

api-suggestion, area-System.Runtime.InteropServices, untriaged

Milestone: -

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label May 8, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 7.0.0 milestone May 8, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added the blocking Marks issues that we want to fast track in order to unblock other important work label May 8, 2022
@AaronRobinsonMSFT
Copy link
Member Author

I am marking this blocking since there is other work the aspnetcore repo needs and getting all of the work done to fully support consumption of LibraryImport in one shot would be preferred to piecemeal.

See dotnet/aspnetcore#41573

@jkotas
Copy link
Member

jkotas commented May 9, 2022

Does this need to take the stack-allocated buffer to enable optimization for passing small strings into the APIs (the built-in marshalling has this optimization today)? Or is this going to be an intentional take-back from the built-in marshalling?

@jkotas
Copy link
Member

jkotas commented May 9, 2022

BstrStringMarshaller

Nit: This is new 3rd casing variant in the public APIs. We have Marshal.StringToBSTR and UnmanagedType.BStr. I do not think we have Bstr anywhere yet. Just wanted to make sure that we want to introduce 3rd casing.

@elinor-fung
Copy link
Member

it is similar to the existing ANSI marshaller and will require users to manually define it using MarshalUsing attribute.
This is a new opt-in API.

If we think BSTR is important and prevalent enough to warrant adding a public marshaller, I'd argue we should also handle UnmanagedType.BStr - like we do for LPStr->AnsiStringMarshaller, LPWStr->Utf16StringMarshaller, LPUTF8Str->Utf8StringMarshaller.

@elinor-fung
Copy link
Member

Does this need to take the stack-allocated buffer to enable optimization for passing small strings into the APIs (the built-in marshalling has this optimization today)?

I'd say so. I don't see a reason not to.

@AaronRobinsonMSFT
Copy link
Member Author

BStrStringMarshaller proposal implementation has been updated with short string optimization.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 11, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 11, 2022
@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented api-ready-for-review API is ready for review, it is NOT ready for implementation and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation api-approved API was approved in API review, it can be implemented labels May 19, 2022
@bartonjs
Copy link
Member

bartonjs commented May 19, 2022

Video

Looks good as proposed

namespace System.Runtime.InteropServices.Marshalling
{
    /// <summary>
    /// Marshaller for BSTR strings
    /// </summary>
    [CLSCompliant(false)]
    [CustomTypeMarshaller(typeof(string), BufferSize = 0x100,
        Features = CustomTypeMarshallerFeatures.UnmanagedResources | CustomTypeMarshallerFeatures.TwoStageMarshalling | CustomTypeMarshallerFeatures.CallerAllocatedBuffer)]
    public unsafe ref struct BStrStringMarshaller
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="BStrStringMarshaller"/>.
        /// </summary>
        /// <param name="str">The string to marshal.</param>
        public BStrStringMarshaller(string? str);

        /// <summary>
        /// Initializes a new instance of the <see cref="BStrStringMarshaller"/>.
        /// </summary>
        /// <param name="str">The string to marshal.</param>
        /// <param name="buffer">Buffer that may be used for marshalling.</param>
        /// <remarks>
        /// The <paramref name="buffer"/> must not be movable - that is, it should not be
        /// on the managed heap or it should be pinned.
        /// <seealso cref="CustomTypeMarshallerFeatures.CallerAllocatedBuffer"/>
        /// </remarks>
        public BStrStringMarshaller(string? str, Span<ushort> buffer);

        /// <summary>
        /// Returns the native value representing the string.
        /// </summary>
        /// <remarks>
        /// <seealso cref="CustomTypeMarshallerFeatures.TwoStageMarshalling"/>
        /// </remarks>
        public void* ToNativeValue();

        /// <summary>
        /// Sets the native value representing the string.
        /// </summary>
        /// <param name="value">The native value.</param>
        /// <remarks>
        /// <seealso cref="CustomTypeMarshallerFeatures.TwoStageMarshalling"/>
        /// </remarks>
        public void FromNativeValue(void* value);

        /// <summary>
        /// Returns the managed string.
        /// </summary>
        /// <remarks>
        /// <seealso cref="CustomTypeMarshallerDirection.Out"/>
        /// </remarks>
        public string? ToManaged();

        /// <summary>
        /// Frees native resources.
        /// </summary>
        /// <remarks>
        /// <seealso cref="CustomTypeMarshallerFeatures.UnmanagedResources"/>
        /// </remarks>
        public void FreeNative();
    }
}

@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 May 19, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 20, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants