Dllimport generator build and test fixes #59658

jkoritzinsky · 2021-09-27T17:14:09Z

Build and test fixes for DllImportGenerator.

Marked as draft and targeting main to validate CI. Once CI is validated, will re-target against feature/use-dllimport-generator.

…ns (marked in source with analyzer suppressions).

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.CloseHandle.cs

jkotas · 2021-09-27T18:14:29Z

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetFullPathNameW.cs

@@ -8,16 +8,13 @@ internal static partial class Interop
 {
    internal static partial class Kernel32
    {
+#pragma warning disable DLLIMPORTGENANALYZER015 // Use 'GeneratedDllImportAttribute' instead of 'DllImportAttribute' to generate P/Invoke marshalling code at compile time
+                                                // Disabled since GetFullPathNameW takes 'ref's into buffers (the ref parameters don't represent single characters).


What is going to be the recommended way to make these work with GeneratedDllImports?

This particular P/Invoke took a dependency on the fact that chars are conditionally blittable, so no copying was done. So far, the Interop team has decided to not consider chars as conditionally blittable.

To make this particular P/Invoke work, there are a few different options:

Update the generator to consider char parameters as blittable when CharSet=CharSet.Unicode.

Update this P/Invoke to take char* parameters instead of ref char and pass in pinned refs.

Update the first parameter to take a string (which will be pinned) or a ReadOnlySpan<char> (using the new Span marshalers) and the second to take a char*, which will be passed without change. As the usage of this method calculates the refs to pass in, from a ReadOnlySpan<char> and a ValueStringBuilder, this would be possible without too much trouble.

We did (1) in #59700, so we should be able to switch this and GetLongPathNameW back.

Fine with getting this change in to the feature branch and updating in the main merge PR.

With the merge conflicts I had after the property order normalization, I just fixed this as part of resolving those.

…matically excludes these test projects from the .NET Framework test build.

…e/browser platforms.

…tFullPathNameW.

…t against an issue with debug/checked CoreCLR.

…untime into dllimport-generator-build-fixes

…s.csproj to enable DNNE's architecture detection on Windows.

jkotas · 2021-09-29T00:17:35Z

public static bool IsReleaseRuntime => typeof(object).Assembly.GetCustomAttribute().Configuration == "Release";

We do not want to have tests that only work on release flavors of the runtime. We should either fix the non-release runtime; or delete the test and leave the behavior unspecified.

stephentoub · 2021-09-29T13:50:48Z

docs/design/libraries/DllImportGenerator/Compatibility.md

+
+### Semantic changes compared to `DllImportAttribute`
+
+The default value of [`CharSet`](https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.dllimportattribute.charset) is runtime/language-defined. In the built-in system, the default value of the `CharSet` property is `CharSet.Ansi`. The P/Invoke source generator makes no assumptions about the `CharSet` if it is not explicitly set on `GeneratedDllImportAttribute`. Marshalling of `char` or `string` requires explicitly specifying marshalling information.


The default value of CharSet is runtime/language-defined

What does it mean for the default value to be "runtime" defined when the code for it is being emitted by the generator?

I assume the generator emits a diagnostic and fails when CharSet would be used and it hasn't been explicitly specified to something other than None?

What does it mean for the default value to be "runtime" defined when the code for it is being emitted by the generator?

This is for the DllImportAttribute not for the GeneratedDllImportAttribute. All this is saying is the new system doesn't attempt to make the choice for you and instead expects explicit statement. ~~The CharSet.Auto is basically not supported.~~

CharSet.Auto is basically not supported

CharSet.Auto is supported (for strings). Not specifying CharSet at all (or specifying CharSet.None) is not supported when marshalling char or string.

Bah. Right. I am confusing the "default" with the concept of specifically setting CharSet.Auto. Apologies.

docs/design/libraries/DllImportGenerator/Compatibility.md

stephentoub · 2021-09-29T13:55:01Z

docs/design/libraries/DllImportGenerator/Compatibility.md

+
+When converting from native to managed, the built-in system would throw a [`MarshalDirectiveException`](https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.marshaldirectiveexception) if the string's length is over 0x7ffffff0. The generated marshalling code will no longer perform this check.
+
+In the built-in system, marshalling a `string` contains an optimization for parameters passed by value to allocate on the stack (instead of through [`AllocCoTaskMem`](https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.marshal.alloccotaskmem)) if the string is below a certain length (MAX_PATH). For UTF-16, this optimization was also applied for parameters passed by read-only reference. The generated marshalling code will include this optimization for read-only reference parameters for non-UTF-16 as well.


The generated marshalling code will include this optimization for read-only reference parameters for non-UTF-16 as well.

Why only "readonly reference parameters"? What's an example of something where we could do so but we're choosing not to?

Inadvertent stack corruption. We were leery of supporting the stackalloced solution for all cases so deferred support. This is one of those point in time decisions that can be updated in V2.

Inadvertent stack corruption.

By whom? The generator is generating all of the code in the implementation.

By the callee. Readonly parameters assume and are contracted to be read only on the native side so we assume that here. If the callee does incorrectly write to the buffer it could negatively impact the runtime state and so was deferred until we see a strong use case.

stephentoub · 2021-09-29T15:59:20Z

src/libraries/Microsoft.Extensions.Options/Microsoft.Extensions.Options.sln

+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Generators", "..\Microsoft.Extensions.Logging.Abstractions\gen\Microsoft.Extensions.Logging.Generators.Roslyn3.11.csproj", "{E00A72D2-A6C2-4334-92AC-7CF7ECC87F4D}"
+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Generators", "..\Microsoft.Extensions.Logging.Abstractions\gen\Microsoft.Extensions.Logging.Generators.Roslyn4.0.csproj", "{EBEF8790-8095-4FD9-8436-1354B30E1529}"
+EndProject


Why are all these .slns being updated with additional projects?

I built the slngen.proj project in 27034bc to regenerate all of the solutions since I added new project references for the DllImportGenerator. Looks like someone didn't run it beforehand and this update also updated the solutions with the logging source generator as well.

stephentoub · 2021-09-29T16:04:36Z

src/libraries/System.Net.Ping/tests/FunctionalTests/System.Net.Ping.Functional.Tests.csproj

@@ -4,6 +4,7 @@
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <IncludeRemoteExecutor>true</IncludeRemoteExecutor>
    <IgnoreForCI Condition="'$(TargetOS)' == 'Browser'">true</IgnoreForCI>
+    <EnableDllImportGenerator>true</EnableDllImportGenerator>


Why is this needed? We can't just enable it everywhere?

stephentoub · 2021-09-29T16:07:36Z

...ibraries/System.Runtime.InteropServices/gen/DllImportGenerator/AnalyzerReleases.Unshipped.md

+DLLIMPORTGENANALYZER014 | Usage            | Error    | RefValuePropertyUnsupported
+DLLIMPORTGENANALYZER015 | Interoperability | Disabled | ConvertToGeneratedDllImportAnalyzer
+DLLIMPORTGENANALYZER016 | Usage            | Error    | GenericTypeMustBeClosed
+DLLIMPORTGENANALYZER017 | Usage            | Warning  | GeneratedDllImportContainingTypeMissingModifiers


Will we shift to using the SYSLIB prefix we use in our other generators?
https://github.com/dotnet/runtime/blob/main/docs/project/list-of-diagnostics.md

Also, IIRC the docs earlier stated that everything is an error, but that doesn't map to the severity column here.

stephentoub · 2021-09-29T16:09:39Z

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

+namespace Microsoft.Interop.Diagnostics
+{
+    [EventSource(Name = "Microsoft-Interop-SourceGeneration-Events")]
+    internal sealed class Events : EventSource


Are these still needed? Weren't similar events added to the Roslyn infrastructure for all generators?

stephentoub · 2021-09-29T16:11:46Z

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

+        private const string GeneratedDllImport = nameof(GeneratedDllImport);
+        private const string GeneratedDllImportAttribute = nameof(GeneratedDllImportAttribute);
+
+        private static readonly Version MinimumSupportedFrameworkVersion = new Version(5, 0);


5 or 6 (or 7)?

jkoritzinsky · 2021-09-29T16:32:15Z

@stephentoub can you please move your feedback to #59579? Once I get a green build, this PR will retarget to the feature/use-dllimport-generator branch (as mentioned in the PR description) and all of the files you have commented on will no longer be in the diff for the PR. Whereas #59579 is the PR that will merge the generator into main, so your feedback will be more useful there.

stephentoub · 2021-09-29T16:41:06Z

can you please move your feedback to #59579?

I had ~40 comments. I'm not going to recreate them all on the other PR.

There was no indication in the PR description that feedback shouldn't be provided here, this wasn't marked as no review, etc.

If you'd like to address or respond to my feedback elsewhere, that's fine.

jkoritzinsky · 2021-09-29T16:54:00Z

I explicitly marked it as a draft PR and as No-Merge, which means I didn't want review on it. I honestly thought that we had gotten rid of the NO REVIEW tag with the move to use draft PRs. Also, I explicitly stated in the PR description that I'm going to re-target the PR to a different branch, so most of the commits will not show up.

I only suggest moving the feedback to #59579 to make sure that we don't lose the feedback when I retarget this PR. I don't know what GitHub does when you retarget a PR and the files that have comments are no longer part of the diff.

I've since marked this PR as "NO REVIEW" to make sure that this doesn't happen again. Sorry for the confusion.

…w Mono loads (or more acurately doesn't) the native library component of these tests.

AaronRobinsonMSFT · 2021-09-29T18:16:53Z

@stephentoub Lots of good comments here. We will work on addressing them in the integration PR – #59579. A few of the comments are about productization points which we plan to address after the integrating into the product as making it publicly consumable is step 2. There are multiple inconsistencies that we need to address as we make the prototype follow the dotnet/runtime style guidelines. The many design queries we will attempt to answer above and continue that conversation.

…covering the native toolchain for DNNE.

… next DNNE version.

…Mono.

…ke and MSBuild.

…oolchain correctly.

…oid changing anything in eng/common.

…MacCatalyst only now for simplicity since DNNE doesn't support mobile platforms anyway)

…dotnet/runtime into dllimport-generator-build-fixes

…pported and DNNE doesn't support it.

…builds.

jkoritzinsky · 2021-10-05T20:15:07Z

All of the CI failures are known and tracked with runfo issues, so I'm going to re-target this and mark it ready for review.

CI build of 4d55f3a against main: https://dev.azure.com/dnceng/public/_build/results?buildId=1403599&view=results

elinor-fung · 2021-10-06T01:27:43Z

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetFullPathNameW.cs

@@ -8,16 +8,13 @@ internal static partial class Interop
 {
    internal static partial class Kernel32
    {
+#pragma warning disable DLLIMPORTGENANALYZER015 // Use 'GeneratedDllImportAttribute' instead of 'DllImportAttribute' to generate P/Invoke marshalling code at compile time
+                                                // Disabled since GetFullPathNameW takes 'ref's into buffers (the ref parameters don't represent single characters).


We did (1) in #59700, so we should be able to switch this and GetLongPathNameW back.

Fine with getting this change in to the feature branch and updating in the main merge PR.

src/samples/DllImportGeneratorSample/DllImportGeneratorSample.csproj

elinor-fung · 2021-10-06T01:38:02Z

src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.Tests/AssemblyInfo.cs

+// As a result, we need to exclude the Mono run here since we build the tests once for CoreCLR and Mono for desktop test runs.
+// We should switch this to another mechanism in the future so we don't submit a work item of this assembly that skips every test
+// for Mono-on-Desktop-Platforms test runs.
+[assembly:ActiveIssue("https://github.com/dotnet/runtime/issues/59815", TestRuntimes.Mono)]


Can we do this via a ProjectExclusions based on RuntimeFlavor in tests.proj? The assembly attribute results in us still sending the test to Helix, running through test discovery for each test to determine that it should not be run, and uploading logs - which seems rather wasteful.

Doh. I completely forgot about that. +1.

We can't do that because we don't build the libraries tests for each runtime flavor, we use one build for both CoreCLR and Mono on desktop platforms (like Linux and Mac). I tried that originally and it didn't work.

...libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/NativeExports.csproj

elinor-fung · 2021-10-06T01:57:05Z

...libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/NativeExports.csproj

+    </XmlPeek>
+    <XmlPeek XmlContent="$(CrossTargetXml)" Query="toolchain-info/compiler-args/text()">
+      <Output TaskParameter="Result" PropertyName="DnneCompilerUserFlags" />
+    </XmlPeek>


XmlPeek

Learned about a new task today - not sure I'm happy about it...

…dotnet/runtime into dllimport-generator-build-fixes

jkoritzinsky added 2 commits September 27, 2021 09:46

Enable DllImportGenerator in more tests.

ea54b1f

Keep some of the P/Invokes using GeneratedDllImport for various reaso…

6f40531

…ns (marked in source with analyzer suppressions).

jkoritzinsky added area-System.Runtime.InteropServices NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Sep 27, 2021

jkotas reviewed Sep 27, 2021

View reviewed changes

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.CloseHandle.cs Show resolved Hide resolved

jkotas reviewed Sep 27, 2021

View reviewed changes

Use TargetFrameworks so the TargetFramework.Sdk sdk kicks in and auto…

dbc141d

…matically excludes these test projects from the .NET Framework test build.

runfoapp bot mentioned this pull request Sep 27, 2021

System.Text.RegularExpressions.Tests.RegexMatchTests.Match_ExcessPrefix Failing in CI #59541

Closed

jkoritzinsky added 2 commits September 27, 2021 17:21

Disable DllImportGenerator runtime tests that depend on DNNE on mobil…

2a65756

…e/browser platforms.

Undo GeneratedDllImport on GetLongPathNameW for the same reason as Ge…

a24525e

…tFullPathNameW.

jkoritzinsky force-pushed the dllimport-generator-build-fixes branch from 9e21237 to a24525e Compare September 28, 2021 00:21

runfoapp bot mentioned this pull request Sep 28, 2021

Test Failure SetOut.SetOutReadToEnd #59008

Closed

jkoritzinsky added 4 commits September 28, 2021 13:58

Make sure that the DllImportGeneratorSample can build.

7a3148f

Fix function pointer usage bug in tests. Disable one SetLastError tes…

fe101e4

…t against an issue with debug/checked CoreCLR.

Merge branch 'feature/use-dllimport-generator' of github.com:dotnet/r…

ed44881

…untime into dllimport-generator-build-fixes

Pass the OutputRid property as the RuntimeIdentifier for NativeExport…

e05c631

…s.csproj to enable DNNE's architecture detection on Windows.

stephentoub reviewed Sep 29, 2021

View reviewed changes

jkoritzinsky added the NO-REVIEW Experimental/testing PR, do NOT review it label Sep 29, 2021

Disable DllImportGenerator.Tests on Mono wholesale due to a bug in ho…

e5652ae

…w Mono loads (or more acurately doesn't) the native library component of these tests.

jkoritzinsky added 2 commits September 29, 2021 16:35

Add MSBuild and CMake in NativeExports.csproj to enable correctly dis…

986e9a5

…covering the native toolchain for DNNE.

Resolve nethost for NativeExports in a manner that will work with the…

102a95d

… next DNNE version.

jkoritzinsky mentioned this pull request Sep 30, 2021

[loader] Look for P/Invoke libs next to the managed assembly #59814

Merged

jkoritzinsky added 2 commits September 30, 2021 09:36

Update issue reference for why DllImportGenerator.Tests don't run on …

4644434

…Mono.

Fix handling custom user compilation/link flags correctly between CMa…

ac37572

…ke and MSBuild.

elinor-fung mentioned this pull request Sep 30, 2021

Make QCalls respect EntryPoint #59823

Closed

jkoritzinsky added 12 commits October 3, 2021 14:38

Windows cross builds don't need to use the CMake info to set up the t…

a3efde9

…oolchain correctly.

Move changes for toolchain.cmake to output-toolchain-info.cmake to av…

c8fcd84

…oid changing anything in eng/common.

Refactor NativeExports custom targets.

e77b7f0

Add resolution step for target triplets for Apple platforms (OSX and …

e320cbd

…MacCatalyst only now for simplicity since DNNE doesn't support mobile platforms anyway)

Exclude tests for FreeBSD and (in a way that works) Mono on Desktop.

d4aff51

Update DNNE

392ee65

Merge branch 'feature/use-dllimport-generator' of https://github.com/…

d633dc8

…dotnet/runtime into dllimport-generator-build-fixes

Use TargetOS instead of TargetsX properties.

6107024

Fix macOS target triple calculations.

ff049ab

Disable DllImportGenerator tests on Win-arm as it's not officially su…

50c189e

…pported and DNNE doesn't support it.

Set the nethost dir for all builds, not just for non-Apple Unix-like …

6646f12

…builds.

Use xcrun to find the compiler and sysroot for Apple builds.

4d55f3a

This was referenced Oct 5, 2021

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

Closed

System.Runtime.Serialization.Xml.Tests failed #59926

Closed

profiler.elt work item test failures in slowpatheltenter #60018

Closed

jkoritzinsky changed the base branch from main to feature/use-dllimport-generator October 5, 2021 20:15

jkoritzinsky marked this pull request as ready for review October 5, 2021 20:15

jkoritzinsky requested review from AaronRobinsonMSFT and elinor-fung October 5, 2021 23:25

AaronRobinsonMSFT approved these changes Oct 5, 2021

View reviewed changes

elinor-fung reviewed Oct 6, 2021

View reviewed changes

jkoritzinsky added 2 commits October 6, 2021 09:36

Fix elinor's feedback for merging this PR.

2c94662

Merge branch 'feature/use-dllimport-generator' of https://github.com/…

00726aa

…dotnet/runtime into dllimport-generator-build-fixes

jkoritzinsky force-pushed the dllimport-generator-build-fixes branch from cab205b to 00726aa Compare October 6, 2021 16:39

jkoritzinsky merged commit 2dff7cd into dotnet:feature/use-dllimport-generator Oct 6, 2021

jkoritzinsky deleted the dllimport-generator-build-fixes branch October 6, 2021 16:40

runfoapp bot mentioned this pull request Oct 8, 2021

SetOut.SetOutReadToEnd (System.Console.Tests) failing #57935

Closed

elinor-fung mentioned this pull request Nov 1, 2021

Fix inconsistency in p/invoke arguments introduced when switching some to be blittable #61071

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dllimport generator build and test fixes #59658

Dllimport generator build and test fixes #59658

jkoritzinsky commented Sep 27, 2021

jkotas Sep 27, 2021

jkoritzinsky Sep 27, 2021

elinor-fung Oct 6, 2021 •

edited

Loading

jkoritzinsky Oct 6, 2021

jkotas commented Sep 29, 2021

stephentoub Sep 29, 2021

AaronRobinsonMSFT Sep 29, 2021 •

edited

Loading

elinor-fung Sep 29, 2021

AaronRobinsonMSFT Sep 29, 2021

stephentoub Sep 29, 2021

AaronRobinsonMSFT Sep 29, 2021

stephentoub Sep 29, 2021

AaronRobinsonMSFT Sep 29, 2021

stephentoub Sep 29, 2021

jkoritzinsky Sep 29, 2021

stephentoub Sep 29, 2021

stephentoub Sep 29, 2021

stephentoub Sep 29, 2021

stephentoub Sep 29, 2021

stephentoub Sep 29, 2021

jkoritzinsky commented Sep 29, 2021 •

edited

Loading

stephentoub commented Sep 29, 2021 •

edited

Loading

jkoritzinsky commented Sep 29, 2021

AaronRobinsonMSFT commented Sep 29, 2021

jkoritzinsky commented Oct 5, 2021

elinor-fung Oct 6, 2021 •

edited

Loading

elinor-fung Oct 6, 2021

AaronRobinsonMSFT Oct 6, 2021

jkoritzinsky Oct 6, 2021

elinor-fung Oct 6, 2021


		### Semantic changes compared to `DllImportAttribute`

		The default value of [`CharSet`](https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.dllimportattribute.charset) is runtime/language-defined. In the built-in system, the default value of the `CharSet` property is `CharSet.Ansi`. The P/Invoke source generator makes no assumptions about the `CharSet` if it is not explicitly set on `GeneratedDllImportAttribute`. Marshalling of `char` or `string` requires explicitly specifying marshalling information.


		When converting from native to managed, the built-in system would throw a [`MarshalDirectiveException`](https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.marshaldirectiveexception) if the string's length is over 0x7ffffff0. The generated marshalling code will no longer perform this check.

		In the built-in system, marshalling a `string` contains an optimization for parameters passed by value to allocate on the stack (instead of through [`AllocCoTaskMem`](https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.marshal.alloccotaskmem)) if the string is below a certain length (MAX_PATH). For UTF-16, this optimization was also applied for parameters passed by read-only reference. The generated marshalling code will include this optimization for read-only reference parameters for non-UTF-16 as well.

Dllimport generator build and test fixes #59658

Dllimport generator build and test fixes #59658

Conversation

jkoritzinsky commented Sep 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elinor-fung Oct 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Sep 29, 2021

Choose a reason for hiding this comment

AaronRobinsonMSFT Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkoritzinsky commented Sep 29, 2021 • edited Loading

stephentoub commented Sep 29, 2021 • edited Loading

jkoritzinsky commented Sep 29, 2021

AaronRobinsonMSFT commented Sep 29, 2021

jkoritzinsky commented Oct 5, 2021

elinor-fung Oct 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elinor-fung Oct 6, 2021 •

edited

Loading

AaronRobinsonMSFT Sep 29, 2021 •

edited

Loading

jkoritzinsky commented Sep 29, 2021 •

edited

Loading

stephentoub commented Sep 29, 2021 •

edited

Loading

elinor-fung Oct 6, 2021 •

edited

Loading