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

Analyzer for System.Runtime.CompilerServices.DisableRuntimeMarshallingAttribute #63714

Closed
jkoritzinsky opened this issue Jan 13, 2022 · 4 comments · Fixed by dotnet/roslyn-analyzers#5822
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Milestone

Comments

@jkoritzinsky
Copy link
Member

In #60639, we approved the System.Runtime.CompilerSevices.DisableRuntimeMarshallingAttribute API. This API is primarily intended to help enable using source-generated interop and to help simplify some concepts in the interop space by disabling some features of the built-in interop system.

This proposal proposes an analyzer to identify when users are using features of the built-in interop system when runtime marshalling is disabled with [assembly:DisableRuntimeMarshalling]. This analyzer will emit a warning diagnostic in the following scenarios (all of which fail at runtime when the P/Invoke or delegate would be called in an interop scenario):

  • SetLastError is set to true on a P/Invoke
  • [LCIDConversionAttribute] is used on a P/Invoke
  • A type that is not a C#-unmanaged type is in a P/Invoke signature or the signature of a delegate with the [UnmanagedFunctionPointer] attribute.
  • A C#-unmanaged type that has any fields with [StructLayout(LayoutKind.Auto)] that is in a P/Invoke signature or the signature of a delegate with the [UnmanagedFunctionPointer] attribute.

The analyzer can also emit a "notice"-level diagnostic for the following scenarios that are likely user errors when [assembly:DisableRuntimeMarshalling] is applied. These scenarios use APIs that do not change behavior when [assembly:DisableRuntimeMarshalling] is applied and always respect the built-in marshalling system. I suggest a "notice" level diagnostic as it's possible that the user is calling APIs defined in other assemblies that don't have [assembly:DisableRuntimeMarshalling] applied or that the results of the APIs are correct even when marshalling is disabled.

  • When Marshal.SizeOf is used on an unmanaged type. (We could also include a code-fix here to recommend switching to Unsafe.SizeOf or the sizeof keyword)
  • When Marshal.OffsetOf is used.
  • When Marshal.StructureToPtr or Marshal.PtrToStructure is used. (We can include code-fixes here to use "unsafe" code or other unsafe APIs).

cc: @dotnet/interop-contrib

@jkoritzinsky jkoritzinsky added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer labels Jan 13, 2022
@jkoritzinsky jkoritzinsky added this to the 7.0.0 milestone Jan 13, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 13, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Jan 13, 2022
@tannergooding
Copy link
Member

tannergooding commented Jan 13, 2022

When Marshal.OffsetOf is used.

Just calling out that this is an "interesting" one where the "alternatives" are also not great.

Because the JIT requires ldflda to do a null check and this causes the JIT to throw a NRE using the "standard offsetof pattern" (which is (nuint)(&((T*)null)->Field)).

This means the next "best" thing is Unsafe.SkipInit(out T value); return (nuint)(&(&value)->Field) and the codegen here isn't "ideal".

It would be nice if one of these generated "optimal" codegen (noting that the "standard" pattern is actually generating optimal codegen, it generates a constants, but also emits a null check which is the problematic part).

Codegen for these two is here: https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABABgAJiBGAOgDkBXfGKASzFwG4BYAKDMq0ASgwB2GVsxoBhCPgAOrADYsAyiwBu7GFz59iAZkoAmctPIBvPuRuUjY3NgBmMcqIatx5ACpUAFACUlta2ocQA7OR+7p4YAX4AZH5+AGowYBjQKABUAe5KSgFwAHwA6gE8vKEAviE2dXbkDs6uMV7exoHBVaG2AKqiji40qgDWrPIAkqKsGH4QDBjkaRlZ5BrYSgwwFQ1hkdEe4vFJCRtbOyXllTV81UA===

@tannergooding
Copy link
Member

What about some of the other Marshal APIs?

We have things like AllocHGlobal that are likely better handled by NativeMemory.Alloc

There are things like StringToHGlobal and such which you can manually do using things like NativeMemory.* + Encoding.*, but which don't have "efficient helper alternatives".

SetLastPInvokeError and SetLastSystemError are new and expected to be used...

Read* and Write* are likely better handled via Span wrappers and/or Unsafe.Read/WriteUnaligned

We have things like GetDelegateForFunctionPointer which are likely better handled by function pointers or small managed wrappers that invoke the UnmanagedCallersOnly via a fnptr.

GetHINSTANCE is better handled by the new NativeLibrary API

Various COM functions are potentially better handled by the new low level CCW/RRW support, if that's something you're using

etc

@jkoritzinsky
Copy link
Member Author

I would rather add different analyzers for those cases. This issue is specifically for catching cases where behavior the user expects will work normally doesn't work when DisableRuntimeMarshallingAttribute is applied. The Marshal APIs mentioned above will have the same behavior no matter if the attribute is applied, but the primary use case of those methods I mentioned are to use them with P/Invokes, which will behave differently when the attribute is applied, creating a possible pit of failure.

@jkoritzinsky jkoritzinsky 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 Jan 25, 2022
@terrajobst
Copy link
Member

terrajobst commented Jan 25, 2022

Video

Category: Interoperability

Severity:

  • The first half should be Warning
    • SetLastError is set to true on a P/Invoke

    • [LCIDConversionAttribute] is used on a P/Invoke

    • A type that is not a C#-unmanaged type is in a P/Invoke signature or the signature of a delegate with the [UnmanagedFunctionPointer] attribute.

    • A C#-unmanaged type that has any fields with [StructLayout(LayoutKind.Auto)] that is in a P/Invoke signature or the signature of a delegate with the [UnmanagedFunctionPointer] attribute.

  • The second half should be IDE Suggestion:
    • When Marshal.SizeOf is used on an unmanaged type. (We could also include a code-fix here to recommend switching to Unsafe.SizeOf or the sizeof keyword)

    • When Marshal.OffsetOf is used.

    • When Marshal.StructureToPtr or Marshal.PtrToStructure is used. (We can include code-fixes here to use "unsafe" code or other unsafe APIs).

@terrajobst terrajobst 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 Jan 25, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 18, 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 code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants