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

Default DynamicCodeSupport to true for CoreCLR #103594

Merged
merged 3 commits into from
Jun 21, 2024
Merged

Default DynamicCodeSupport to true for CoreCLR #103594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d582d5f
Default DynamicCodeSupported to true for CoreCLR
sbomer Jun 17, 2024
2edd3a7
Merge branch 'main' into dynamicCodeDefault
sbomer Jun 18, 2024
146a42d
Fix crossgen2_publish
sbomer Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@
<!-- Use .dwarf files instead of .dsym files since our symbol exporting may not safely handle folders. -->
<NativeSymbolExt Condition="'$(_IsApplePlatform)' == 'true'">.dwarf</NativeSymbolExt>
<DsymUtilOptions Condition="'$(_IsApplePlatform)' == 'true'">--flat</DsymUtilOptions>
<!-- Prevent getting DynamicCodeSupport=true default from ILLink targets that are imported
with the Sdk.targets import above, before Native AOT defaults get a chance to set it -->
<DynamicCodeSupport>false</DynamicCodeSupport>
Comment on lines +56 to +58
Copy link
Member

Choose a reason for hiding this comment

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

Anyone have opinions about switching crossgen2 to use the package version of ILC, same as cdacreader and ILC itself? I don't know if it's useful to use the live version and it's probably more trouble than it's worth - looks like we stabilized on using the package version instead.

(This comment is not proposing work for this PR and we'd only do this after #103375 merges.)

@dotnet/ilc-contrib @am11 for thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

Pros: live version gives (an additional) validation from CG2 (a real-world app) at the PR stage.
Cons: it slows down the development a bit; since R2R has separate code paths which are normally stabilized when next preview release is nearing. e.g. #103537 (comment)

I think with ongoing code unification and converging implementations, it appears that we are leaning towards fixing issues earlier during the development phase; so using live builds is a general goodness. Also, some downstream repos like aspnetcore run into issues when a bug makes its way into the package undetected, since they have direct runtime dependency without waiting for the next SDK preview release.

Copy link
Member

Choose a reason for hiding this comment

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

My typical opinion is to use "live versions" when possible.

Copy link
Member

Choose a reason for hiding this comment

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

Anyone have opinions about switching crossgen2 to use the package version of ILC

I would be fine with that. I do not think that maintaining two different schemes is worth it, and switching everything to live build is way too complicated.

it appears that we are leaning towards fixing issues earlier during the development phase; so using live builds is a general goodness. Also, some downstream repos like aspnetcore run into issues when a bug makes its way into the package undetected, since they have direct runtime dependency without waiting for the next SDK preview release.

This will get better with unified build codeflow in not-so-distant future. The unified build codeflow is going to be dotnet/runtime -> dotnet/dotnet and the validation of that flow is going to do the multistage build that exercises live-built toolset. If AOT compiler bug sneaks in, we will see a break in that flow.

</PropertyGroup>

<ItemGroup Condition="'$(NativeAotSupported)' == 'true'">
Expand Down
1 change: 1 addition & 0 deletions src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<_ComObjectDescriptorSupport Condition="'$(_ComObjectDescriptorSupport)' == ''">false</_ComObjectDescriptorSupport>
<_DesignerHostSupport Condition="'$(_DesignerHostSupport)' == ''">false</_DesignerHostSupport>
<_DefaultValueAttributeSupport Condition="'$(_DefaultValueAttributeSupport)' == ''">false</_DefaultValueAttributeSupport>
<DynamicCodeSupport Condition="'$(DynamicCodeSupport)' == ''">true</DynamicCodeSupport>
sbomer marked this conversation as resolved.
Show resolved Hide resolved
<UseSystemResourceKeys Condition="'$(UseSystemResourceKeys)' == ''">false</UseSystemResourceKeys>
</PropertyGroup>

Expand Down
Loading