Skip to content

Do not allow DynamicCodeSupport=true for NativeAOT #99590

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

Merged
merged 2 commits into from
Mar 13, 2024
Merged

Do not allow DynamicCodeSupport=true for NativeAOT #99590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bfe0048
Do not allow DynamicCodeSupport=true for NativeAOT
MichalStrehovsky Mar 12, 2024
35710e2
Update src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Nativ…
jkotas Mar 13, 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
2 changes: 2 additions & 0 deletions src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Publish.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@

<Error Condition="'$(PublishTrimmed)' == 'false'" Text="PublishTrimmed is implied by native compilation and cannot be disabled." />

<Error Condition="'$(DynamicCodeSupport)' == 'true'" Text="DynamicCodeSupport property cannot be set to true with native compilation." />

<!-- Fail with descriptive error message for common unsupported cases. -->
<Error Condition="'$(DisableUnsupportedError)' != 'true' and '$(OS)' == 'Windows_NT' and '$(_targetOS)' != 'win'"
Text="Cross-OS native compilation is not supported." />
Expand Down
18 changes: 7 additions & 11 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/SubstitutionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,18 +73,14 @@ private bool TryGetFeatureCheckValue(EcmaMethod method, out bool value)
if (featureGuardAttribute.FixedArguments is not [CustomAttributeTypedArgument<TypeDesc> { Value: EcmaType featureType }])
continue;

if (featureType.Namespace == "System.Diagnostics.CodeAnalysis") {
Copy link
Member Author

Choose a reason for hiding this comment

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

This has more diffs than needed because I also fixed the formatting.

Do we have a plan for what to do with the illinker formatting? I don't mind illinker's charming odd formatting but I do care when the formatting leaks out. This is not the first or second or third time it leaked out.

We also leak it into places like learn.microsoft.com and can't even be consistent about it there. It makes things look sloppy.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, we should just align the formatting to one standard repo-wide and fail at least one leg in CI matrix in PRs when this produces diffs:

runtime/eng/formatting/format.sh

Line 20 in 54ff0d2

echo "$MANAGED_FILES" | cat | xargs | sed -e 's/ /,/g' | dotnet format whitespace --include - --folder

There are no points in adhering to "historical reasons" at this point. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

We very rarely squabble over formatting in this repo. I'm confident 90% of all formatting nits I ever had in PRs were with illinker devs doing non-illinker work.

There are times when people violate the formatting rules because it's done to highlight structure that would be lost if formatted according to the book. I'm pretty sure a formatting police would demotivate people from doing that because nobody wants to research how to pragma suppress the formatter (I would certainly quit doing that because I hate pragma suppressions). The formatter would likely destroy more readability than what it would bring; I don't see formatting as a general problem in the PRs I review (or in reviews I get on my PRs).

Copy link
Member

Choose a reason for hiding this comment

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

fail at least one leg in CI matrix in PRs when this produces diffs:

There are already JIT formatting jobs with clang-format when you touch any files in src/coreclr/jit. Presumably the same thing can be done here if there's an agreement.

Copy link
Member

Choose a reason for hiding this comment

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

We plan to adopt runtime's formatting conventions for illink (tracking issue: #78050). The surest way to prevent current illink style from slipping in would be to add a formatting job. Much of it is muscle memory for me, and I'm likely to make mistakes even while trying to follow the runtime conventions. I'm also always happy to take style nit feedback on PRs.

Copy link
Member

@jkotas jkotas Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are already JIT formatting jobs with clang-format when you touch any files in src/coreclr/jit. Presumably the same thing can be done here if there's an agreement.

You can enforce formatting via Roslyn analyzer. It has been enabled for libraries for quite some time. No need to have an extra formatting job.

Copy link
Member Author

Choose a reason for hiding this comment

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

fail at least one leg in CI matrix in PRs when this produces diffs:

There are already JIT formatting jobs with clang-format when you touch any files in src/coreclr/jit. Presumably the same thing can be done here if there's an agreement.

Here's how they work (just the PRs I was pinged on in the past half a day or so):

image

image

They:

  1. Annoy people working in the codebase
  2. Produce an extra dose of CO2 (notice the ❌)
  3. Could be a fix to this manufactured problem. Using different coding styles in different part of the product is an entirely manufactured problem; I don't remember when I had to comment on formatting (or fix it) outside interactions with anyone exposed to illinker codebase; I think the only person who was able to maintain the separation was Marek because I don't recall issues with that but that would be an exception to a rule.

Copy link
Member

@am11 am11 Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't remember when I had to comment on formatting (or fix it) outside interactions with anyone exposed to illinker codebase

Could be because libs subset already enforces the formatting rules and fails the build (seen a single trailing space failing builds a few times).

Annoy people working in the codebase

Bit annoying it is, if we are in a rush. Which begs the question; why the rush merging PR which affects everyone? 🙂
I guess it is ok; better than annoying multiple people with unformatted code to save one developer's 5-10 seconds of time in active PR, who wasn't using VS or forgot to press Ctrl+K,D before Ctrl+S. They can use dotnet format <changed files> or the helper script in repo linked above (which only formats files in changeset, according to the .editorconfig and CodeAnalysis rules file; same as VS).

Copy link
Member Author

Choose a reason for hiding this comment

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

Could be because libs subset already enforces the formatting rules and fails the build (seen a single trailing space failing builds a few times).

99% of code I write or review doesn't have build-enforced formatting. I rarely review/work in the libs or JIT subsets. It's not that. It's just not a problem in general.

switch (featureType.Name) {
case "RequiresAssemblyFilesAttribute":
case "RequiresUnreferencedCodeAttribute":
return true;
case "RequiresDynamicCodeAttribute":
if (_hashtable._switchValues.TryGetValue(
"System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported",
out bool isDynamicCodeSupported)
&& !isDynamicCodeSupported)
if (featureType.Namespace == "System.Diagnostics.CodeAnalysis")
{
switch (featureType.Name)
{
case "RequiresAssemblyFilesAttribute":
case "RequiresUnreferencedCodeAttribute":
case "RequiresDynamicCodeAttribute":
return true;
break;
}
}
}
Expand Down