[release/7.0] Fix DiagnosticSource to work with NativeAOT #78532
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There were 2 problems: 1. The use of MakeGenericType doesn't work when a property is a ValueType. An app will crash when a listener is enabled and DiagnosticSourceEventSource tries writing values. 2. The properties on KeyValuePair were not being preserved correctly, so the Arguments of the DiagnosticSourceEventSource methods were not being serialized correctly. Add test (and infrastructure) to ensure DiagnosticSource works in a NativeAOT app Fix #75945
- Only run them in Release configuration - Suppress IL2026 warning
Set EventSourceSupport only on the projects that need it.
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti Issue DetailsBackport of #76109 to release/7.0 /cc @MichalStrehovsky @eerhardt Customer ImpactTestingRiskIMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.
|
|
@eerhardt @noahfalk any opinion on backporting this to 7.0? The AOT unsafety warning is not a great user experience. Making But I don't know if this is going to meet the bar - I didn't ask for approvals yet, just to see how the CI fares and whether this can be backported cleanly. Or we could just wait for more customer feedback on DiagnosticSource like I wrote in #76109 (comment). People wanting to use DiagnosticSource is more likely to meet the bar. |
|
(The issue in question with the warning is #78367) |
I'm supportive of backporting it. We already have 2 instances of customers bumping into this (one was trying to profile their NativeAOT application and the app crashing, and now the warning that customers can't do anything about). Notes on backporting:
|
These days with source analyzers being part of the targeting pack, it ships automatically with every release, i.e. here you can see the
|
|
@MichalStrehovsky @eerhardt I added the required package servicing changes as described here for you. |
src/libraries/Microsoft.Extensions.Hosting/src/Microsoft.Extensions.Hosting.csproj
Outdated
Show resolved
Hide resolved
|
@eerhardt Could you fill in the Risk section? This looks low risk to me since it keeps the old code path when not using AOT, but I'd like your take. |
|
@MichalStrehovsky @eerhardt Besides the request to fill out the risk section and verifying the CI failures, is there anything else to address? Otherwise, can you please add the I ask because for the January release, we will only have a one day window to merge servicing PRs, so I want to make sure all the PRs are 100% ready on that day for me to just click the merge button. That window is from Nov. 29th to Nov. 30th. |
|
I've filled out the Risk section. It appears the |
MichalStrehovsky
added
the
Servicing-consider
On it. I will send a PR today that will target main. Let's backport the fix after it got some validation in main. |
jeffschwMSFT
removed
the
Servicing-consider
Manual backport of c8503d3 Fixes dotnet#77988 Unblocks dotnet#78532 Introduce the AnnotateTargetPathWithContract switch to make it configure-able when a source project should return the reference project's assembly instead of the source assembly, when other projects compile against it. Set it so that reference assemblies are only returned for NetCoreAppCurrent tfms or when the project isn't packable. - Fix System.DirectoryServices.AccountManagement build System.DirectoryServices.AccountManagement now builds against src/System.DirectoryServices instead of ref/System.DirectoryServices (because the package doesn't contain the ref assembly). Because of that, the compiler now gets confused because of the System.DirectoryServices.Interop namespace and the global Interop class. This happens even though the DirectoryServices.Interop namespace doesn't include any public types. That results in the following errors: src\libraries\System.DirectoryServices.AccountManagement\src\System\DirectoryServices\AccountManagement\AD\SidList.cs(50,26): error CS0246: The type or namespace name 'SID_AND_ATTRIBUTES' could not be found (are you missing a using directive or an assembly reference?) src\libraries\System.DirectoryServices.AccountManagement\src\System\DirectoryServices\AccountManagement\interopt.cs(439,20): error CS0246: The type or namespace name 'UNICODE_INTPTR_STRING' could not be found (are you missing a using directive or an assembly reference?) This commit fixes that by removing the System.DirectoryServices.Interop namespace and moving the types into the parent namespace. - Suppress nullable warnings in Serialization.Schema Now that Schema compiles against the source assembly of System.CodeDom, it receives nullability errors. I'm suppressing them manually for now but am filing an issue to correctly fix those. Related: dotnet#78036
ViktorHofer
added
Servicing-consider
|
CI failure is #78778. |
|
@MichalStrehovsky can you please sign-off? |
rbhanda
added
Servicing-approved
|
A note for servicing validation of this - we should make sure we get the updates to the reference assembly in the 7.0 ref pack, and that the same fixes are present in the nuget package. |
|
Signed-off by area owners. |
ghost
locked as resolved and limited conversation to collaborators
Backport of #76109 to release/7.0
/cc @MichalStrehovsky @eerhardt
Customer Impact
Right now some extremely common code paths (i.e. HttpClient) report AOT warnings. Reported issues include:
This warning is confusing to customers because it is in our internal library code, and it is not possible for the customer to address the warning.
In late 7.0 we pushed out a change that disables EventSource on NativeAOT by default. We did this because DiagnosticSource (built on top of EventSource) was not AOT safe and if DiagnosticSource codepaths got activated (due to e.g. a machine-wide PerfView session), DiagnosticSource would cause the app to crash.
The current situation is that:
We got feedback about both of these aspects
With this change that DiagnosticSource no longer requires dynamic code (allowing it to work in a NativeAOT'd app). The change has been in main for 6 weeks, merges cleanly to 7.0, and addresses both of those aspects. Trimming problems may still occur (and trimming warnings will appear as appropriate), but are less common since the most common uses of DiagnosticSource have been manually preserved.
Testing
All NativeAOT testing. This change has been in main for 6 weeks. There are new AOT tests being added that ensure this scenario works.
Risk
Fairly low. The behavior change is limited to when "Dynamic Code" isn't support (i.e. NativeAOT), and when the Type being reflected upon is a ValueType. For "normal" .NET Apps there is no behavior change.