Use ReferencePathWithRefAssemblies for libs ILLink

[ViktorHofer]

Fixes#79513

The libraries build invokes ILLink to perform "library" mode trimming. The libraries build step is expected to be runtime agnostic and shouldn't bind against a specific CoreLib runtime implementation.

During compilation, even though we have a ProjectReference pointing to the CoreLib/src project, we build against CoreLib/ref. But that ILLink invocation doesn't honor that as it uses the ReferencePath msbuild item instead of ReferencePathWithRefAssemblies.

[ghost]

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

Issue Details

---

Fixes #79513

The libraries build invokes ILLink to perform "library" mode trimming. The libraries build step is expected to be runtime agnostic and shouldn't bind against a specific CoreLib runtime implementation.

During compilation, even though we have a ProjectReference pointing to the CoreLib/src project, we build against CoreLib/ref. But that ILLink invocation doesn't honor that as it uses the ReferencePath msbuild item instead of ReferencePathWithRefAssemblies.

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

[jkotas]

I need to understand the implications of passing in ref/CoreLib instead of src/CoreLib

Yes, it is going to fix the problem. We are going to lose some trimming that is happening right now. It should not be in the noise range for CoreCLR (about 3kB across whole netcoreapp). It can be more for Mono mobile targets, but I do not think it is a problem there since mobile targets should always run trimming step during publish and so it is not going to impact size of the final app.

[jkotas]

You can also revert #79523 as part of this change. It should not be necessary anymore.

[jkotas]

@marek-safar Any concerns about impact of this change on mobile targets?

[marek-safar]

@jkotas no concerns, mobile runtime packs are massive for other reasons so any sdk like trimming has no visible impact.

[ViktorHofer]

You can also revert #79523 as part of this change. It should not be necessary anymore.

Will do that in a follow-up PR.

[akoeplinger]

Should we backport this?

[ViktorHofer]

I defer to @jkotas and @MichalStrehovsky regarding the likeliness of this affecting customers. As this also impacts inbox binaries which ship inside a package, we would need to also re-ship those (< 10 libs). I.e. System.Text.Json.

[MichalStrehovsky]

I defer to @jkotas and @MichalStrehovsky regarding the likeliness of this affecting customers. As this also impacts inbox binaries which ship inside a package, we would need to also re-ship those (< 10 libs). I.e. System.Text.Json.

We have #78532 lined up for servicing that is fixing DiagnosticSource to work with NativeAOT. The fix is to call RuntimeFeature.IsDynamicCodeSupported, and this call is outside corelib. Those are the same conditions that triggered #79513 that this PR is fixing. I didn't check, but I think this will trigger the same issue and we're going to run into this during the servicing validation. Might as well preempt that and backport this.

[jkotas]

As this also impacts inbox binaries which ship inside a package

This should only affect binaries that reference CoreLib project directly. None of binaries that reference CoreLib project directly ship in a package.

S.D.DiagnosticSource does not reference CoreLib project directly so it should not be affected.

This is the list of assemblies affected by this change (diffed IL before/after):

• System.Collections.Concurrent - IntPtr.Size, Array.MaxLength
• System.Collections - Array.MaxLength
• System.Data.Common - RuntimeHelpers.IsDynamicCodeSupported
• System.Diagnostics.TraceSource - Environment.NewLine
• System.Memory - Array.MaxLength
• System.Private.Uri - OperatingSystem.IsWindows()

System.Data.Common is the only assembly where the problem is observable.

I think it would be hard to justify pushing the fix to servicing given the limited impact. We do not have any reports of customers hitting the problem in System.Data.Common, and it is likely that anybody trying to use System.Data.Common at this point with native AOT is going to hit number of issues and warnings (the managed database providers are not Aot clean) so having one more caused by a runtime bug is not a big deal.

[MichalStrehovsky]

Ah, makes sense. Thank you for checking! It is a great testament to the libs test code coverage that the only issue we had was actually found in testing!