Remove redundant trimmer warning suppressions

[jkurdek]

Removed suppressions identified as redundant by the linker (dotnet/linker#2922).

\cc @vitek-karas

[ghost]

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Removed suppressions identified as redundant by the linker (dotnet/linker#2922).

\cc @vitek-karas

Author:	jkurdek
Assignees:	-
Labels:	area-Meta
Milestone:	-

[tlakollo]

Yikes, I just added annotations with RDC and therefore more redundant suppressions x.x

[ghost]

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Removed suppressions identified as redundant by the linker (dotnet/linker#2922).

\cc @vitek-karas

Author:	jkurdek
Assignees:	jkurdek
Labels:	area-Meta, linkable-framework, community-contribution
Milestone:	-

[vitek-karas]

Yikes, I just added annotations with RDC and therefore more redundant suppressions x.x

The current linker changes will not detect RDC related suppressions - for that we would have to port it over to NativeAOT and run the libraries through NativeAOT (somehow).

[vitek-karas]

@eerhardt: still a draft, but could you please take a look?

[ghost]

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Removed suppressions identified as redundant by the linker (dotnet/linker#2922).

\cc @vitek-karas

Author:	jkurdek
Assignees:	jkurdek
Labels:	area-Infrastructure-libraries, linkable-framework
Milestone:	-

[vitek-karas]

Probably same thing here: https://github.com/dotnet/runtime/pull/52461/files#diff-ebbc6bf7ee73960d1921153ebc67451c142e0455a82e08a2b7c61c45cf7c2217R201

[vitek-karas]

@AaronRobinsonMSFT is this a leftover from your code review ;-) (it sounds like you)
In any case @jkurdek could you please change the Justification to something more professional - like "The PInvoke has object/interface typed parameters which are potentially trim incompatible, but in this case they're OK".

@AaronRobinsonMSFT - I don't know the details of the implicit COM interop , but I'm not actually sure this IS OK - There are two parameters which are potentially problematic:
pvConfigParams (typed as object) - but we're passing in null, so there's no trimming problem.
ppvObject which is declared as [MarshalAs(UnmanagedType.Interface)] out ITransactionDispenser ppvObject - my understanding is that the interop will take the managed definition of ITransactionDespenser and assume the vtable coming from native matches the managed definition. With trimming, that means we have to guarantee that we preserve the entire ITransactionDespenser as-is. Additionally the code will try to cast this to couple of other interface types (ITmNodeName, ITransactionImportWhereabouts and IResourceManagerFactory2) all of which will need the same treatment I guess.

Isn't this something which should use ComWrappers to be trim-compatible? I guess that is covered in #75031 ?

[AaronRobinsonMSFT]

is this a leftover from your code review ;-) (it sounds like you)

100 % me, apologies. Please update as appropriate.

I don't know the details of the implicit COM interop

I think there was discussion that the Windows version of Transactions wasn't trim-compatible at all. However, how we express that isn't clear to me and at the time I was simply trying to get it to "leave me alone". Your analysis makes sense to me as far as why it may not be okay.

Isn't this something which should use ComWrappers to be trim-compatible? I guess that is covered in #75031 ?

From my conversations this particular part is a legacy or at the very least non-forward looking feature set that will not have a lot of investment in the future. It is unclear if ComWrappers will be worth the long term effort but perhaps #75031 is meant to track that conversation. @roji is the product owner here and can help guide us toward the correct solution.

[roji]

Ah sorry, the unprofessional comment was my bad - @AaronRobinsonMSFT just proposed a draft PR with this, and I was supposed to change the text. Definitely go ahead and update as appropriate.

As @AaronRobinsonMSFT wrote, System.Transactions generally isn't a forward-looking components; the reason we ported the code containing the COM interop is to unblock .NET Framework users depending on distributed transactions and allow them to port to .NET. I very much doubt we'll invest more in this in the future; if we see a real need for a cross-platform API, we'll be implementing new support for that (#71769), but that likely wouldn't involve direct COM interop in any way.

I'll post a comment in #75031 clarifying the situation.

[MichalStrehovsky]

We have references to this bug under src/coreclr/nativeaot as well - how was the redundant suppression analysis for NativeAOT done? I wonder why it didn't find those (they should be redundant because they were just a bug workaround).

[jkurdek]

The redundant warning suppression detection has not been ported to nativeaot yet (#75201). The suppression you mentioned should be identified after the functionality is ported and run witihin nativeaot.

[MichalStrehovsky]

This PR has a change in src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/Assignability.cs - what mechanism was used to detect it?

(NativeAOT doesn't run on code that is unreferenced and doesn't have a "library mode" like illink does - so the ability to detect redundant suppressions is somewhat limited even if we were to port the feature - we would have to come up with an app that includes all the code in NativeAOT's libraries. It will only flag things that are used, making it easy for us to ship with an actual warning that only some customers will see - customers who happen to use that part of the library.)

[vitek-karas]

It was detected because linker was used to "trim libraries for warnings" (The sfx project run of linker) and it was given the AOT corelib as input. Jeremi found out that this happens when you run build Clr.Aot+Libs -rc Debug -lc Release, in which case linker gets artifacts\bin\coreclr\windows.x64.Debug\aotsdk\System.Private.CoreLib.dll as one of the input assemblies.

As for the comment about NativeAOT not having libraries mode - that should not matter. The detection works in the "trim app" mode as well - it only reports redundant warnings on methods which were "marked".

[MichalStrehovsky]

It was detected because linker was used to "trim libraries for warnings" (The sfx project run of linker) and it was given the AOT corelib as input

I see, yes, there can only be one corelib project for the libs partition but we have 3 in this repo.

The corelib is selected here:

runtime/Directory.Build.props

Lines 261 to 263 in e8b1491

	<CoreLibProject Condition="'$(RuntimeFlavor)' == 'CoreCLR'">$([MSBuild]::NormalizePath('$(CoreClrProjectRoot)', 'System.Private.CoreLib', 'System.Private.CoreLib.csproj'))</CoreLibProject>
	<CoreLibProject Condition="'$(RuntimeFlavor)' == 'Mono'">$([MSBuild]::NormalizePath('$(MonoProjectRoot)', 'System.Private.CoreLib', 'System.Private.CoreLib.csproj'))</CoreLibProject>
	<CoreLibProject Condition="'$(UseNativeAotCoreLib)' == 'true'">$([MSBuild]::NormalizePath('$(CoreClrProjectRoot)', 'nativeaot', 'System.Private.CoreLib', 'src', 'System.Private.CoreLib.csproj'))</CoreLibProject>

[vitek-karas]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[vitek-karas]

Parts of the failures in WASM are #74413. (socket and http related failures)
The rest of the failures is #75223