[DllImportGenerator] Don't generate a full stub body when the body does nothing #60601

jkoritzinsky · 2021-10-19T01:01:39Z

Generate the partial function as a DllImport when a full-body generated stub would be a no-op wrapper around an inner DllImport.

Fixes #60596

…ed stub would be a no-op wrapper around an inner DllImport.

…c for blittable scenarios as well since we now turn a GeneratedDllImport into a DllImport when the stub would do nothing,.

src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/PInvokeStubCodeGenerator.cs

…rshaller for the basic forwarder scenario.

AaronRobinsonMSFT

This is great. I need this for the "unsupported framework" fallback. I have it all set up except the forwarder to DllImport logic.

am11 · 2021-10-19T03:30:12Z

Crossgen2 considers Boolean and Char as billitable types:

runtime/src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/MarshalUtilsTests.cs

Lines 25 to 56 in ba01c59

    
           [Theory] 
        
           [InlineData(WellKnownType.Void)] 
        
           [InlineData(WellKnownType.Boolean)] 
        
           [InlineData(WellKnownType.Char)] 
        
           [InlineData(WellKnownType.SByte)] 
        
           [InlineData(WellKnownType.Byte)] 
        
           [InlineData(WellKnownType.Int16)] 
        
           [InlineData(WellKnownType.UInt16)] 
        
           [InlineData(WellKnownType.Int32)] 
        
           [InlineData(WellKnownType.UInt32)] 
        
           [InlineData(WellKnownType.Int64)] 
        
           [InlineData(WellKnownType.UInt64)] 
        
           [InlineData(WellKnownType.IntPtr)] 
        
           [InlineData(WellKnownType.UIntPtr)] 
        
           [InlineData(WellKnownType.Single)] 
        
           [InlineData(WellKnownType.Double)] 
        
           [InlineData(WellKnownType.RuntimeFieldHandle)] 
        
           [InlineData(WellKnownType.RuntimeTypeHandle)] 
        
           [InlineData(WellKnownType.RuntimeMethodHandle)] 
        
           public void IsBlittableType_BilittableWellKnownTypes_ReturnsTrue(WellKnownType type) => 
        
               Assert.True(MarshalUtils.IsBlittableType(_context.GetWellKnownType(type))); 
        
           [Theory] 
        
           [InlineData(WellKnownType.String)] 
        
           [InlineData(WellKnownType.ValueType)] 
        
           [InlineData(WellKnownType.Enum)] 
        
           [InlineData(WellKnownType.Array)] 
        
           [InlineData(WellKnownType.MulticastDelegate)] 
        
           [InlineData(WellKnownType.Exception)] 
        
           [InlineData(WellKnownType.Object)] 
        
           public void IsBlittableType_NonBilittableWellKnownTypes_ReturnsFalse(WellKnownType type) => 
        
               Assert.False(MarshalUtils.IsBlittableType(_context.GetWellKnownType(type)));

but TypeSymbolExtensions doesn't:

runtime/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/TypeSymbolExtensions.cs

Lines 54 to 69 in b201a16

    
           private static bool IsSpecialTypeBlittable(SpecialType specialType) 
        
               => specialType switch 
        
               { 
        
                   SpecialType.System_Void 
        
                   or SpecialType.System_SByte 
        
                   or SpecialType.System_Byte 
        
                   or SpecialType.System_Int16 
        
                   or SpecialType.System_UInt16 
        
                   or SpecialType.System_Int32 
        
                   or SpecialType.System_UInt32 
        
                   or SpecialType.System_Int64 
        
                   or SpecialType.System_UInt64 
        
                   or SpecialType.System_Single 
        
                   or SpecialType.System_Double 
        
                   or SpecialType.System_IntPtr 
        
                   or SpecialType.System_UIntPtr => true,

should they match?

jkoritzinsky · 2021-10-19T05:02:54Z

In this case, crossgen2 is incorrect. Bool and Char are not blittable in the current system (which is the whole reason blittable != unmanaged)

am11 · 2021-10-19T05:21:03Z

Ah right. The test was added as part of addressing #46653 (comment), which suggests crossgen2 case is different (crossgen2 is not involved in Marshal.SizeOf).

am11 · 2021-10-19T05:24:08Z

Perhaps we can split IsBlittable and RequiresSpecialMarshalling methods in crossgen2 to avoid confusion. cc @MichalStrehovsky.

MichalStrehovsky · 2021-10-19T05:57:38Z

Crossgen2 considers Boolean and Char as billitable types:

It doesn't. The unit test just uses a weird base class library where Bool and Char have no fields:

runtime/src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/CoreTestAssembly/Platform.cs

Lines 34 to 35 in ba01c59

    
           public struct Boolean { } 
        
           public struct Char { }

If you zoom into IsBlittableType implementation:

runtime/src/coreclr/tools/Common/TypeSystem/Interop/IL/MarshalUtils.cs

Lines 14 to 65 in 15be630

    
           public static bool IsBlittableType(TypeDesc type) 
        
           { 
        
               if (!type.IsDefType) 
        
               { 
        
                   return false; 
        
               } 
        
               TypeDesc baseType = type.BaseType; 
        
               bool hasNonTrivialParent = baseType != null 
        
                   && !baseType.IsWellKnownType(WellKnownType.Object) 
        
                   && !baseType.IsWellKnownType(WellKnownType.ValueType); 
        
               if (hasNonTrivialParent && !IsBlittableType(baseType)) 
        
               { 
        
                   return false; 
        
               } 
        
               var mdType = (MetadataType)type; 
        
               if (!mdType.IsSequentialLayout && !mdType.IsExplicitLayout) 
        
               { 
        
                   return false; 
        
               } 
        
               foreach (FieldDesc field in type.GetFields()) 
        
               { 
        
                   if (field.IsStatic) 
        
                   { 
        
                       continue; 
        
                   } 
        
                   MarshallerKind marshallerKind = MarshalHelpers.GetMarshallerKind( 
        
                       field.FieldType, 
        
                       parameterIndex : null, 
        
                       customModifierData: null, 
        
                       field.GetMarshalAsDescriptor(), 
        
                       isReturn: false, 
        
                       isAnsi: mdType.PInvokeStringFormat == PInvokeStringFormat.AnsiClass, 
        
                       MarshallerType.Field, 
        
                       elementMarshallerKind: out var _); 
        
                   if (marshallerKind != MarshallerKind.Enum 
        
                       && marshallerKind != MarshallerKind.BlittableValue 
        
                       && marshallerKind != MarshallerKind.BlittableStruct 
        
                       && marshallerKind != MarshallerKind.UnicodeChar) 
        
                   { 
        
                       return false; 
        
                   } 
        
               } 
        
               return true; 
        
           }

You can see why we would end up saying the struct is blittable if there's no fields.

Maybe the code in MarshalUtils should check for primitive types first, but as things are now I think it kind of works for real core libraries. I think we would see bugs if it didn't work.

jkoritzinsky · 2021-10-19T06:12:31Z

We definitely would definitely see bugs if this was broken in real scenarios. I was very confused about how it was broken.

src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/Compiles.cs

…nerator.UnitTests/Compiles.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/DllImportGenerator.cs

src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/Compiles.cs

src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/DllImportGenerator.cs

src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/Compiles.cs

...teropServices/tests/DllImportGenerator.UnitTests/ConvertToGeneratedDllImportAnalyzerTests.cs

… a stub to be generated.

…nsky/runtime into dllimportgen-blittable-in-place

jkoritzinsky added 2 commits October 18, 2021 17:55

Generate the partial function as a DllImport when a full-body generat…

cac8936

…ed stub would be a no-op wrapper around an inner DllImport.

Update the ConvertToGeneratedDllImportAnalyzer to provide a diagnosti…

00a0c8a

…c for blittable scenarios as well since we now turn a GeneratedDllImport into a DllImport when the stub would do nothing,.

jkoritzinsky added area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature labels Oct 19, 2021

jkoritzinsky added this to the 7.0.0 milestone Oct 19, 2021

jkoritzinsky requested review from AaronRobinsonMSFT and elinor-fung October 19, 2021 01:01

elinor-fung reviewed Oct 19, 2021

View reviewed changes

src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/PInvokeStubCodeGenerator.cs Show resolved Hide resolved

Require the managed return marshaller to also be the native return ma…

4ce28f4

…rshaller for the basic forwarder scenario.

AaronRobinsonMSFT approved these changes Oct 19, 2021

View reviewed changes

jkotas reviewed Oct 19, 2021

View reviewed changes

src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/Compiles.cs Outdated Show resolved Hide resolved

Update src/libraries/System.Runtime.InteropServices/tests/DllImportGe…

4c81816

…nerator.UnitTests/Compiles.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

elinor-fung reviewed Oct 19, 2021

View reviewed changes

runfoapp bot mentioned this pull request Oct 19, 2021

System.Net.Sockets.Tests.SendPacketsAsync.SendPacketsElement_FileZeroCount_Success sometimes fails #60017

Closed

jkoritzinsky added 3 commits October 19, 2021 10:42

Use an explicitly non-blittable type in the method signature to force…

b50ae7d

… a stub to be generated.

PR feedback.

429a32a

Merge branch 'dllimportgen-blittable-in-place' of github.com:jkoritzi…

ce236e3

…nsky/runtime into dllimportgen-blittable-in-place

jkoritzinsky requested a review from elinor-fung October 19, 2021 21:11

Fix various DllImportGenerator.UnitTests failures that I missed earlier.

dac2ce8

elinor-fung approved these changes Oct 19, 2021

View reviewed changes

runfoapp bot mentioned this pull request Oct 19, 2021

1ES Pools often run out of space #60610

Closed

jkoritzinsky merged commit ee4a76d into dotnet:main Oct 20, 2021

jkoritzinsky deleted the dllimportgen-blittable-in-place branch October 20, 2021 15:51

ghost locked as resolved and limited conversation to collaborators Nov 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DllImportGenerator] Don't generate a full stub body when the body does nothing #60601

[DllImportGenerator] Don't generate a full stub body when the body does nothing #60601

Uh oh!

jkoritzinsky commented Oct 19, 2021

Uh oh!

Uh oh!

AaronRobinsonMSFT left a comment

Uh oh!

am11 commented Oct 19, 2021

Uh oh!

jkoritzinsky commented Oct 19, 2021

Uh oh!

am11 commented Oct 19, 2021

Uh oh!

am11 commented Oct 19, 2021

Uh oh!

MichalStrehovsky commented Oct 19, 2021

Uh oh!

jkoritzinsky commented Oct 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[DllImportGenerator] Don't generate a full stub body when the body does nothing #60601

[DllImportGenerator] Don't generate a full stub body when the body does nothing #60601

Uh oh!

Conversation

jkoritzinsky commented Oct 19, 2021

Uh oh!

Uh oh!

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

am11 commented Oct 19, 2021

Uh oh!

jkoritzinsky commented Oct 19, 2021

Uh oh!

am11 commented Oct 19, 2021

Uh oh!

am11 commented Oct 19, 2021

Uh oh!

MichalStrehovsky commented Oct 19, 2021

Uh oh!

jkoritzinsky commented Oct 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants