search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

Runtime's illink runs don't catch certain set of warnings #84297

Open vitek-karas opened 1 year ago

vitek-karas commented 1 year ago

If a framework library contains code which:

Then runtime build will not report the trim warning anywhere. But dotnet/sdk will fail a test when such framework makes it into the SDK repo.

We have 3 runs of illink on a framework assembly:

Found in https://github.com/dotnet/sdk/pull/31571. Related discussion in https://github.com/dotnet/runtime/pull/84272.

ghost commented 1 year ago

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

Issue Details
If a framework library contains code which: * Produces a trim warning * Is not referenced from public API * Is rooted via a LibraryBuild descriptor for testing purposes Then runtime build will not report the trim warning anywhere. But dotnet/sdk will fail a test when such framework makes it into the SDK repo. We have 3 runs of illink on a framework assembly: * First is in runtime when we trim the assembly for shipping (the per assembly run). This runs with the "library" mode, but it also passes the `ILLink.Descriptors.LibraryBuild.xml` which is a descriptor, which roots more code (typically for testing purposes - I assume the tests create these via reflection for now). This run actually sees the problem in the code, but we disable all trim related warnings in this run, so it's not reported. * Second is in runtime, the "sfx" run - where we trim the entire framework in "library" mode, but in this case, we don't apply the `ILLink.Descriptors.LibraryBuild.xml`, so the affected code is not rooted. And thus the trimmer doesn't even see the affected code. * Third run is in SDK, where we trim the entire framework in "copy" mode, which sees all code, and thus reports the problem. Found in https://github.com/dotnet/sdk/pull/31571. Related discussion in https://github.com/dotnet/runtime/pull/84272.
Author: vitek-karas
Assignees: -
Labels: `linkable-framework`, `area-Tools-ILLink`
Milestone: -
vitek-karas commented 1 year ago

@sbomer for ideas.

I think the cleanest solution would be to fix the "sfx" run to also merge all LibraryBuild.xml files and pass them to the linker. That would more closely mimic the per-assembly behavior.

jkotas commented 1 year ago

What the "sfx" run trying to test? If it is trying to test what happens on end-user machine, it should not use LibraryBuild.xml - LibraryBuild.xml is not going to be used on the end-user machine either.

vitek-karas commented 1 year ago

What the "sfx" run trying to test? If it is trying to test what happens on end-user machine, it should not use LibraryBuild.xml - LibraryBuild.xml is not going to be used on the end-user machine either.

It's there to report all of the trim warnings in the framework. It IS the run which should have reported this problem. (There's a matching "oob" run which does the same for all OOB assemblies).

While it's true that LibaryBuild.xml will not be there on user's machine, the code which it roots will be on the user machine. So if the user ends up marking the affected framework assembly as "copy", the trimmer would still see the affected code and report the problem to the user.

It's not very common that users will mark framework assemblies as copy, but it does happen (typically when they play the whack-a-mole game if trimming breaks their app and they don't care to fix the warnings).

That's basically what the test in SDK is trying to validate, that we're not shipping framework code which will produce trim warnings. The test originally targets down-level TFMs - meaning that latest SDK will not produce warnings when trimming downlevel apps. But we can't (shouldn't) disable the test for "current" TFM, since around RC that test needs to be passing for the current TFM, and we're bound to forget to enable it on-time.

jkotas commented 1 year ago

So if the user ends up marking the affected framework assembly as "copy", the trimmer would still see the affected code and report the problem to the user.

Should the "sfx" run use the "copy" mode then?

vitek-karas commented 1 year ago

Should the "sfx" run use the "copy" mode then?

Maybe it should. But it's not perfect either - I think it runs on the built output, not the trim output, but I could be wrong (in which case it would see all code, even that which is not rooted by anything ever). If it runs on the trim output, then "copy" would be correct behavior I think.

eerhardt commented 1 year ago

That's basically what the test in SDK is trying to validate, that we're not shipping framework code which will produce trim warnings.

It's not testing that "we're not shipping framework code which will produce trim warnings". It is testing that the warnings the framework code produces are expected.

https://github.com/dotnet/sdk/blob/b488cef31aeecda692158df7a28c352cba0fb51f/src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs#L713-L730

There are a ton of expected warnings in that test.

In my mind, the only issue here is that this test is in the dotnet/sdk and not in dotnet/runtime. If we want to catch these issues earlier (which are pretty rare, AFAICT), we can move this test to dotnet/runtime. But I'm not sure it is strictly necessary. The test did its job - it caught the problem. It just didn't do it before the code was merged into dotnet/runtime.