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.

Sign up for GitHub

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

Jump to bottom

Suppress linker warnings properly #84272

Merged
merged 7 commits into from
Apr 4, 2023
Merged

Suppress linker warnings properly #84272

merged 7 commits into from
Apr 4, 2023

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Apr 3, 2023

The Linker warnings were incorrectly suppressed in #83554 and causing a build failure in dotnet/sdk#31571 (comment)

The method will be referenced for loading all .NET primitive types from user provided Core assembly. I assumed core primitive types are trimmer safe, but it seems unreferenced primitive type could be trimmed, therefore propagating the linker warning for the method. And suppressing the warning for the usages as I think it is safe to suppress these warnings for System.Object and System.Void types.

In order to unblock build failure caused by this raising this PR with the quick fix. Will reiterate the handling of other primitive types separately

@ghost ghost assigned buyaa-n Apr 3, 2023
@ghost
Copy link

ghost commented Apr 3, 2023

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

Issue Details

The Linker warnings were incorrectly suppressed in #83554 and causing a build failure in dotnet/sdk#31571 (comment)

The method will be referenced for loading all .NET primitive types from user provided Core assembly. I assumed core primitive types are trimmer safe, but it seems unreferenced primitive type could be trimmed, therefore propagating the linker warning for the method. And suppressing the warning for the usages as I think it is safe to suppress these warnings for System.Object and System.Void types.

In order to unblock build failure caused by this raising this PR with the quick fix. Will reiterate the handling of other primitive types separately

Author: buyaa-n
Assignees: -
Labels:

area-System.Reflection.Emit
Milestone: -

@jkotas
Copy link
Member

jkotas commented Apr 3, 2023

causing a build failure in dotnet/sdk#31571 (comment)

What is the build configuration where this is causing a build failure? I would expect that the new Reflection.Emit implementation is always going to be trimmed for now, and thus it should not produce any warnings. The warning can be a symptom of some more fundamental problem.

@build-analysis build-analysis bot mentioned this pull request Apr 4, 2023
Closed
marek-safar
marek-safar reviewed Apr 4, 2023
View reviewed changes
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs Outdated

default: throw new NotSupportedException(SR.Format(SR.NotSupported_Signature, type.FullName));
// We need to translate from Reflection.Type to SignatureTypeEncoder.
if (id > -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the extra if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

module.GetTypeIdFromCoreTypes(type) returns -1 in case it is not core type or the type not found

Copy link
Contributor

Choose a reason for hiding this comment

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

Right but why do you optimizing for exception code path?

Copy link
Contributor Author

@buyaa-n buyaa-n Apr 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For now we only support core types, eventually that path will be updated to support other types

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep but even in that case they would better be handled via default: than this if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how, seems there is no default signature option in SignatureTypeEncoder https://github.com/dotnet/runtime/blob/main/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/Encoding/BlobEncoders.cs#L888-L1145

Copy link
Member

Choose a reason for hiding this comment

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

What would a default signature option do?

The existing PrimitiveTypeCode defined by System.Reflection.Metadata looks very similar for the CoreTypeId. Maybe we should just use PrimitiveTypeCode.

Copy link
Member

Choose a reason for hiding this comment

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

we should just use PrimitiveTypeCode.

Thoughts about this one?

Copy link
Contributor Author

@buyaa-n buyaa-n Apr 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thoughts about this one?

Tried, PrimitiveTypeCode uses SignatureTypeCode values that are not matching by the index, specifically the value of IntPtr is 24, UIntPtr is 25, Object value is 28, leaving a gap in the array indexes

runtime/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Signatures/PrimitiveTypeCode.cs

Line 11 in bc887b3

Boolean = SignatureTypeCode.Boolean,

@vitek-karas vitek-karas mentioned this pull request Apr 4, 2023
Open
@vitek-karas
Copy link
Member

I checked why the warnings were not caught by the runtime build originally.
The reason is that there's a difference in what the runtime does and SDK.
Runtime runs the linker on all framework assemblies in the "library" mode - so it trims things down to only code accessible through public APIs of each assembly.
But SDK runs the linker on all framework in the "copy" mode - so all code is kept.

In this case, the offending code is not connected to any public API (yet I assume), so in the "library" mode it gets trimmed away. But SDK will still see it.

@sbomer for ideas - the SDK is effectively testing "more" than runtime.

I create #84297 to track the "infra" issue.

jkotas
jkotas reviewed Apr 4, 2023
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Hope the suggested edits make sense

src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs Show resolved Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs Outdated Show resolved Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs Show resolved Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs Show resolved Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs Show resolved Hide resolved
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Apr 4, 2023

Hope the suggested edits make sense

Yes, it makes sense, thank you!

@build-analysis build-analysis bot mentioned this pull request Apr 4, 2023
Closed
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Apr 4, 2023

The failure unrelated and known, merging

@buyaa-n buyaa-n merged commit 3e6ad47 into dotnet:main Apr 4, 2023
@buyaa-n buyaa-n deleted the suppress-warning branch April 4, 2023 23:22
@ghost ghost locked as resolved and limited conversation to collaborators May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MichalStrehovsky MichalStrehovsky MichalStrehovsky left review comments

@vitek-karas vitek-karas vitek-karas left review comments

@marek-safar marek-safar marek-safar left review comments

@jkotas jkotas jkotas approved these changes

@steveharter steveharter Awaiting requested review from steveharter

@eerhardt eerhardt Awaiting requested review from eerhardt

@AaronRobinsonMSFT AaronRobinsonMSFT Awaiting requested review from AaronRobinsonMSFT

Assignees

@buyaa-n buyaa-n

Labels
area-System.Reflection.Emit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@buyaa-n @jkotas @vitek-karas @marek-safar @MichalStrehovsky