Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[main] Update dependencies from dotnet/linker #59257

Merged
merged 8 commits into from
Sep 30, 2021

Conversation

dotnet-maestro[bot]
Copy link
Contributor

@dotnet-maestro dotnet-maestro bot commented Sep 17, 2021

This pull request updates the following dependencies

From https://github.com/dotnet/linker

  • Subscription: 0d6d6ae4-f71f-4395-53e6-08d8e409d87d
  • Build: 20210928.1
  • Date Produced: September 29, 2021 12:25:58 AM UTC
  • Commit: d31e26ef3e552f1795c86a88c1451d36922e08f1
  • Branch: refs/heads/main

…916.2

Microsoft.NET.ILLink.Tasks
 From Version 7.0.100-1.21460.1 -> To Version 7.0.100-1.21466.2
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-codeflow for labeling automated codeflow label Sep 17, 2021
…920.2

Microsoft.NET.ILLink.Tasks
 From Version 7.0.100-1.21460.1 -> To Version 7.0.100-1.21470.2
…921.2

Microsoft.NET.ILLink.Tasks
 From Version 7.0.100-1.21460.1 -> To Version 7.0.100-1.21471.2
…923.2

Microsoft.NET.ILLink.Tasks
 From Version 7.0.100-1.21460.1 -> To Version 7.0.100-1.21473.2
…924.1

Microsoft.NET.ILLink.Tasks
 From Version 7.0.100-1.21460.1 -> To Version 7.0.100-1.21474.1
@marek-safar
Copy link
Contributor

The failures look like possible regression from dotnet/linker#2288

@vitek-karas @MichalStrehovsky PTAL

@vitek-karas
Copy link
Member

I'm looking into this.
It's definitely caused by the binder flags changes in the linker. I'll gather all of the different cases this hits and then try to determine the best solution. Since this it integration into "main" (7.0), I think it's OK to keep this PR opened for a bit until we resolve those issues.

@vitek-karas
Copy link
Member

There are 3 types of issues here, in all cases linker warns in places it didn't before:

  1. Using BindingFlags.GetField in a call to Type.GetField - using this binding flag has no effect at runtime, but the new linker treats this as "unknown binding flags" which leads to marking all fields (and the caller doesn't have annotation matching that request).
  2. Activator.CreateInstance used with the binding flag BindingFlags.CreateInstance - this actually might have a runtime effect (I'm not 100% sure). It should probably not be warning in the linker for this case?
  3. Linker failed to warn in cases where calls to reflection methods with binding flags coming from method parameters. This exposes actually new places in the runtime which need annotations. Linker SHOULD warn in these cases. This only affects OOB assemblies which we didn't make trim-ready, so it's just about updating suppressions.

@MichalStrehovsky
Copy link
Member

Thanks for investigating @vitek-karas!

Looks like the 1 and 2 would be fixed if we went with what was in the first commit of dotnet/linker#2288 - only consider null and don't care about extra flags we don't understand.

But it looks like that would just silently stomp over the pre-existing issue that is: the BindingFlagsAreUnsupported logic is too conservative. E.g. for typeof(SomeType).GetField("FooBar", .Public | .Instance | .GetField) we would go and mark all fields on FooBar. That behavior existed before #2288; now we just aligned with it.

I'm looking into making BindingFlagsAreUnsupported more precise. This should fix 1 and 2.

dotnet-maestro bot and others added 3 commits September 28, 2021 12:10
…927.1

Microsoft.NET.ILLink.Tasks
 From Version 7.0.100-1.21460.1 -> To Version 7.0.100-1.21477.1
…928.1

Microsoft.NET.ILLink.Tasks
 From Version 7.0.100-1.21460.1 -> To Version 7.0.100-1.21478.1
These warnings are legit, but the libraries this applies to are not yet annotated, so the suppressions are added to the library build only.
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suppressions Vitek!

@marek-safar marek-safar merged commit 424aea9 into main Sep 30, 2021
@marek-safar marek-safar deleted the darc-main-5a6c414e-4969-41e4-953a-b96412fcb725 branch September 30, 2021 06:48
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-codeflow for labeling automated codeflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants