Repository navigation
fix(#890): Tiered PGO off in every compiling host — the .NET 10 JIT drops the isinst in Roslyn's inlined ContainingType #6254
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
56 changes: 56 additions & 0 deletions
56
test/Memex.Portal.Shared.Test/TieredPgoIsOffInEveryCompilingHostTest.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| using System; | ||
| using System.IO; | ||
| using System.Text.Json; | ||
| using Xunit; | ||
|
|
||
| namespace Memex.Portal.Shared.Test; | ||
|
|
||
| /// <summary> | ||
| /// #890: a process that compiles in-mesh C# must run with Tiered PGO OFF. | ||
| /// | ||
| /// <para>On .NET 10 linux-x64 the Tier-1 JIT, in roughly one test host in three of this suite's | ||
| /// workload, inlines Roslyn's <c>SourceMemberContainerTypeSymbol.ContainingType</c> | ||
| /// (<c>_containingSymbol as NamedTypeSymbol</c>) into <c>NamedTypeSymbol.AsNestedTypeDefinitionImpl</c> | ||
| /// without the <c>isinst</c>. A top-level type's namespace container then reads as its containing type, | ||
| /// and every later <c>Emit</c> in that process throws in <c>get_ContainingTypeDefinition</c> — which is | ||
| /// how <c>InstallNeverInheritsRepoCompileVerdictTest</c>, <c>ModulesUpdateIndependentlyOfThePlatformTest</c> | ||
| /// and <c>ModuleLinkVersionTest</c> went red on unrelated pull requests. Measured in CI's own shape: the | ||
| /// listing has no <c>isinst</c> in every poisoned host and has it in every clean one, and with Tiered PGO | ||
| /// off no host poisoned (Doc/Architecture/NodeTypeCompilation, "The defect, in one listing").</para> | ||
| /// | ||
| /// <para>The defect itself is per-process and architecture-specific, so it cannot be reproduced on | ||
| /// demand in one test. What CAN be pinned deterministically is that the switch that removes it reaches | ||
| /// the host: the runtime reads <c>System.Runtime.TieredPGO</c> from this host's runtimeconfig, and the | ||
| /// build writes it there from <c><TieredPGO>false</TieredPGO></c> in the Directory.Build.props | ||
| /// files. Negative control: building this project with <c>-p:TieredPGO=true</c> makes both assertions | ||
| /// fail.</para> | ||
| /// </summary> | ||
| public class TieredPgoIsOffInEveryCompilingHostTest | ||
| { | ||
| private const string Switch = "System.Runtime.TieredPGO"; | ||
|
|
||
| [Fact] | ||
| public void TheRuntimeIsHandedTieredPgoOff() | ||
| { | ||
| var value = AppContext.GetData(Switch); | ||
| Assert.True(value is not null, | ||
| $"{Switch} is not set for this host, so the runtime's default (Tiered PGO ON) applies and the " | ||
| + "#890 miscompile can poison every Emit in the process. Set <TieredPGO>false</TieredPGO>."); | ||
| Assert.Equal("false", value!.ToString(), ignoreCase: true); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void TheRuntimeConfigOnDiskCarriesIt() | ||
| { | ||
| var name = typeof(TieredPgoIsOffInEveryCompilingHostTest).Assembly.GetName().Name; | ||
| var path = Path.Combine(AppContext.BaseDirectory, $"{name}.runtimeconfig.json"); | ||
| Assert.True(File.Exists(path), $"no runtimeconfig at {path}"); | ||
| using var doc = JsonDocument.Parse(File.ReadAllText(path)); | ||
| Assert.True( | ||
| doc.RootElement.GetProperty("runtimeOptions").TryGetProperty("configProperties", out var props) | ||
| && props.TryGetProperty(Switch, out var pgo) | ||
| && pgo.ValueKind == JsonValueKind.False, | ||
| $"{path} does not carry \"{Switch}\": false — the switch never reaches a host launched as " | ||
| + "`dotnet <name>.dll`, which is how CI's shards launch it."); | ||
| } | ||
| } | ||
38 changes: 38 additions & 0 deletions
38
test/MeshWeaver.PluginTester.Test/BakeHostRunsWithTieredPgoOffTest.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| using System; | ||
| using System.IO; | ||
| using System.Text.Json; | ||
| using Xunit; | ||
|
|
||
| namespace MeshWeaver.PluginTester.Test; | ||
|
|
||
| /// <summary> | ||
| /// #890, the production half: the executables built under the ROOT <c>Directory.Build.props</c> — | ||
| /// represented here by the bake host <c>mw-plugin-test</c>, which compiles every NodeType a CI bake | ||
| /// seals — must run with Tiered PGO OFF. Under Tiered PGO the .NET 10 linux-x64 Tier-1 JIT can drop the | ||
| /// <c>isinst</c> in Roslyn's inlined <c>SourceMemberContainerTypeSymbol.ContainingType</c>, after which | ||
| /// no <c>Emit</c> in that process succeeds (Doc/Architecture/NodeTypeCompilation, "The defect, in one | ||
| /// listing"). | ||
| /// | ||
| /// <para>This test host inherits <c>test/Directory.Build.props</c>, not the root, so it reads the | ||
| /// runtimeconfig the SDK copies beside it from the referenced executables — the files a | ||
| /// <c>dotnet mw-plugin-test.dll</c> launch actually reads. Deleting <c><TieredPGO>false</TieredPGO></c> | ||
| /// from the root props turns it red; the test tree's own copy of the property cannot keep it green.</para> | ||
| /// </summary> | ||
| public class BakeHostRunsWithTieredPgoOffTest | ||
| { | ||
| [Theory] | ||
| [InlineData("mw-plugin-test")] | ||
| [InlineData("mw-combo-verify")] | ||
| public void TheRootInheritingExecutable_CarriesTieredPgoOff(string host) | ||
| { | ||
| var path = Path.Combine(AppContext.BaseDirectory, $"{host}.runtimeconfig.json"); | ||
| Assert.True(File.Exists(path), $"no runtimeconfig for {host} at {path} — the referenced executable was not copied"); | ||
| using var doc = JsonDocument.Parse(File.ReadAllText(path)); | ||
| Assert.True( | ||
| doc.RootElement.GetProperty("runtimeOptions").TryGetProperty("configProperties", out var props) | ||
| && props.TryGetProperty("System.Runtime.TieredPGO", out var pgo) | ||
| && pgo.ValueKind == JsonValueKind.False, | ||
| $"{path} does not carry \"System.Runtime.TieredPGO\": false — {host} inherits the root " | ||
| + "Directory.Build.props, and without the property it compiles under the #890 miscompile."); | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, and fixed in 4182810.
BakeHostRunsWithTieredPgoOffTestinMeshWeaver.PluginTester.Testparses the runtimeconfigs the SDK copies beside it from the referenced root-inheriting executables,mw-plugin-test.runtimeconfig.jsonandmw-combo-verify.runtimeconfig.json, and asserts"System.Runtime.TieredPGO": falsein each.Negative control, run locally: I deleted
<TieredPGO>false</TieredPGO>from the rootDirectory.Build.propsonly and left the test tree's copy in place. Both cases went red (Total: 2, Failed: 2). With the property restored, both pass. The doc section and the root-props comment now name both guards.