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

Update linker warning numbers #40865

Merged
merged 2 commits into from
Aug 17, 2020
Merged

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Aug 14, 2020

In response to dotnet/linker#1385.
This updates the linker and replaces the global suppressions for IL2006 with the warnings that it was split into.

In response to dotnet/linker#1385.
This replaces the global suppressions for IL2006 with the warnings that it was split into.
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

I assume you checked that we don't get any new warnings compared to the state before the linker update - right?
The change looks good.

@sbomer
Copy link
Member Author

sbomer commented Aug 14, 2020

Yup, I checked the modified UnconditionalSuppressMessage attributes by linking without global suppressions (@eerhardt showed me how to run on the Extensions libraries which don't get linked in the build).

@@ -26,7 +26,7 @@ public static partial class Marshal
[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern int SizeOfHelper(Type t, bool throwIfNotMarshalable);

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2006:UnrecognizedReflectionPattern",
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern",
Copy link
Member

Choose a reason for hiding this comment

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

probably not a huge deal, but do we have a better string than UnrecognizedReflectionPattern now? It sort of made sense when IL2006 was used, but now we are using 30 differnet codes, all with "UnrecognizedReflectionPattern"

Copy link
Member Author

Choose a reason for hiding this comment

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

@mateoatr does this string have any particular meaning? It looks to me like you could put anything here and the linker would treat it the same. I agree it'd be nice to come up with more specific names for these.

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that linker doesn't actually look at that string, so it should be possible to use really anything. @mateoatr would know for sure.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to not address this in this PR to unblock you for 5.0. But it would be nice to get something like this early in 6.0.

It is totally a convenience for the readers - but it is a useful convenience IMO, so I don't need to look up the error code from https://github.com/mono/linker/blob/master/docs/error-codes.md each time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed dotnet/linker#1432 for this

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, any string can be used here and it shouldn't affect the linker.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

The changes look good to me - once the trimming tests are updated with the new warning numbers as well.

@sbomer sbomer merged commit b61effa into dotnet:master Aug 17, 2020
@sbomer
Copy link
Member Author

sbomer commented Aug 17, 2020

Merging since the failures are known (#40885 and #40844).

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@sbomer sbomer deleted the renumberILLinkWarnings branch November 3, 2023 18:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants