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

Complete warnings for AOT unsafe interop #74697

Open
4 tasks
jkotas opened this issue Aug 27, 2022 · 6 comments
Open
4 tasks

Complete warnings for AOT unsafe interop #74697

jkotas opened this issue Aug 27, 2022 · 6 comments

Comments

@jkotas
Copy link
Member

jkotas commented Aug 27, 2022

Warnings for AOT unsafe interop are incomplete best effort currently: #74630 (comment)

  • Generate warnings for unsafe interop in marshalled structs
  • Generate warnings for AOT unsafe marshallers like AsAny and LayoutClass
  • Use actual marshaller kind to produce the warning instead of the approximation at
    static bool IsComInterop(MarshalAsDescriptor? marshalInfoProvider, TypeDesc parameterType)
    {
    // This is best effort. One can likely find ways how to get COM without triggering these alarms.
    // AsAny marshalling of a struct with an object-typed field would be one, for example.
    // This logic roughly corresponds to MarshalInfo::MarshalInfo in CoreCLR,
    // not trying to handle invalid cases and distinctions that are not interesting wrt
    // "is this COM?" question.
    NativeTypeKind nativeType = NativeTypeKind.Default;
    if (marshalInfoProvider != null)
    {
    nativeType = marshalInfoProvider.Type;
    }
    if (nativeType == NativeTypeKind.IUnknown || nativeType == NativeTypeKind.IDispatch || nativeType == NativeTypeKind.Intf)
    {
    // This is COM by definition
    return true;
    }
    if (nativeType == NativeTypeKind.Default)
    {
    TypeSystemContext context = parameterType.Context;
    if (parameterType.IsPointer)
    return false;
    while (parameterType.IsParameterizedType)
    parameterType = ((ParameterizedType)parameterType).ParameterType;
    if (parameterType.IsWellKnownType(Internal.TypeSystem.WellKnownType.Array))
    {
    // System.Array marshals as IUnknown by default
    return true;
    }
    else if (parameterType.IsWellKnownType(Internal.TypeSystem.WellKnownType.String) ||
    InteropTypes.IsStringBuilder(context, parameterType))
    {
    // String and StringBuilder are special cased by interop
    return false;
    }
    if (parameterType.IsValueType)
    {
    // Value types don't marshal as COM
    return false;
    }
    else if (parameterType.IsInterface)
    {
    // Interface types marshal as COM by default
    return true;
    }
    else if (parameterType.IsDelegate || parameterType.IsWellKnownType(Internal.TypeSystem.WellKnownType.MulticastDelegate)
    || parameterType == context.GetWellKnownType(Internal.TypeSystem.WellKnownType.MulticastDelegate).BaseType)
    {
    // Delegates are special cased by interop
    return false;
    }
    else if (InteropTypes.IsCriticalHandle(context, parameterType))
    {
    // Subclasses of CriticalHandle are special cased by interop
    return false;
    }
    else if (InteropTypes.IsSafeHandle(context, parameterType))
    {
    // Subclasses of SafeHandle are special cased by interop
    return false;
    }
    else if (parameterType is MetadataType mdType && !mdType.IsSequentialLayout && !mdType.IsExplicitLayout)
    {
    // Rest of classes that don't have layout marshal as COM
    return true;
    }
    }
    return false;
    }
  • Steer people towards P/Invoke source generator by the warning text
@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 Aug 27, 2022
@MichalStrehovsky
Copy link
Member

Steer people towards P/Invoke source generator by the warning text

If we tell people to use the source generator for AsAny/LayoutClass/System.Delegate marshalling, the source generator will then tell them it's unsupported. It will shift the blame, but the end result for the user is the same if we don't send them on a hike first.

@jkotas
Copy link
Member Author

jkotas commented Aug 27, 2022

Yes, it is the case with retrofitting existing AOT unsafe code.

Steering people towards source generated interop is more interesting for new code. Source generated interop has natural AOT safety guiderails and prevents the AOT unsafe code to be written in the first place.

@hez2010
Copy link
Contributor

hez2010 commented Aug 27, 2022

towards P/Invoke source generator

Do source generated P/Invokes support direct P/Invoke and static linking?

If not then I think it's not a replacement of current P/Invokes.

@jkotas
Copy link
Member Author

jkotas commented Aug 27, 2022

Do source generated P/Invokes support direct P/Invoke and static linking?

Yes. This aspect is not different between source generated and built-in marshalling.

All P/Invokes (except a few outliers) in this repo have been switched to be source generated, and native aot publishing uses direct P/Invokes and static linking extensively.

@jkotas jkotas added this to the 8.0.0 milestone Aug 27, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 27, 2022
@MichalStrehovsky
Copy link
Member

Use actual marshaller kind to produce the warning instead of the approximation at

This logic is shared with IL Linker and IL Linker doesn't have access to this unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

3 participants