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

Disable STJ reflection by default on AOT publish #88609

Conversation

eiriktsarpalis
Copy link
Member

Following up on dotnet/sdk#33757 (comment) this updates the AOT targets so that the JsonSerializerIsReflectionEnabledByDefault feature switch is defaulted to false.

@ghost
Copy link

ghost commented Jul 10, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Following up on dotnet/sdk#33757 (comment) this updates the AOT targets so that the JsonSerializerIsReflectionEnabledByDefault feature switch is defaulted to false.

Author: eiriktsarpalis
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

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.

This LGTM. It would be good to get a review from @MichalStrehovsky before merging.

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.

This LGTM. It would be good to get a review from @MichalStrehovsky before merging.

I don't think this is necessary. Native AOT without trimming doesn't make sense. Neither does a potential future Mono AOT using Native AOT-specific targets (that this PR is changing). It's likely the RequiresDynamicCode-annotated code doesn't even pose a problem for Mono-based AOT in its current form.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 10, 2023
@eerhardt
Copy link
Member

This LGTM. It would be good to get a review from @MichalStrehovsky before merging.

I don't think this is necessary. Native AOT without trimming doesn't make sense. Neither does a potential future Mono AOT using Native AOT-specific targets (that this PR is changing). It's likely the RequiresDynamicCode-annotated code doesn't even pose a problem for Mono-based AOT in its current form.

See feedback from @vitek-karas here.

@vitek-karas - thoughts?

@MichalStrehovsky
Copy link
Member

See feedback from @vitek-karas dotnet/sdk#33757 (comment).

I saw that - my comment is that I don't see how the ILCompiler targets (that are edited here) could be relevant for a possible PublishAot-use in Mono and that we haven't even decided how RDC annotations could apply to Mono (Mono doesn't have a matching mode that would be able to take advantage of RDC restrictions so as things stand now, enabling the RDC analyzer on Mono would annoy people with warnings, most of which will not actually be a problem because Mono has both an interpreter, and universal shared code).

@vitek-karas
Copy link
Member

I will happily follow Michal's guidance on this.

I was thinking about it on the abstract level - feature which is incompatible with AOT (due to RequiresDynamicCode) should be disabled for AOT. On the other hand, NativeAOT currently doesn't support the configuration without trimming and mono might have different needs (and thus different feature switch defaults).

@MichalStrehovsky
Copy link
Member

I will happily follow Michal's guidance on this.

I was thinking about it on the abstract level - feature which is incompatible with AOT (due to RequiresDynamicCode) should be disabled for AOT. On the other hand, NativeAOT currently doesn't support the configuration without trimming and mono might have different needs (and thus different feature switch defaults).

It is good to think about PublishAot/PublishTrimmed as two separate things, but since we don't have plans to support PublishAot without PublishTrimmed in ILCompiler (we error out if someone unsets PublishTrimmed), it doesn't feel necessary to put it into ILCompiler .targets file. It might be useful in some Mono AOT .targets file at some point in the future. Such .targets file might not even exist right now thought. I would close this - there's no spot where we could proactively add this right now.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants