Repository navigation
JIT: Add DebugInfo to ret expr statement when doing GDV - #135209
Open
anderspedersen wants to merge 6 commits into
Open
anderspedersen wants to merge 6 commits into
anderspedersen wants to merge 6 commits into
Conversation
dotnet-policy-service
Bot
added
the
community-contribution
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
3 tasks
Contributor
|
Thanks for chasing this down. I measured before/after against the same tree with the line reverted, on Checked builds of the PR head for linux-x64, linux-arm64, win-x64, win-arm64 and osx-arm64, plus a linux-x64 Release build (without native PGO/LTO).
The other arm of that Happy to push it here or separately. MeasurementsCaller frame after Tier1 GDV (
Same rows on all five platforms (Checked), on linux-x64 Release, and, on linux-x64 Checked, with the probe precompiled by crossgen2 (R2R; there the reference frame is the R2R code's, not Tier0's). Tier1 IP map for the guarded block of the first row (the test's Notes:
Patch: the extra line + regression test (against 5abd6a0)diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp
index 635c0f7ea6f..2b2805c4dfa 100644
--- a/src/coreclr/jit/indirectcalltransformer.cpp
+++ b/src/coreclr/jit/indirectcalltransformer.cpp
@@ -1033,7 +1033,7 @@ private:
if (m_returnTemp != BAD_VAR_NUM)
{
GenTree* const store = m_compiler->gtNewTempStore(m_returnTemp, call);
- m_compiler->fgNewStmtAtEnd(block, store);
+ m_compiler->fgNewStmtAtEnd(block, store, m_stmt->GetDebugInfo());
}
else
{
diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_134523/Runtime_134523.cs b/src/tests/JIT/Regression/JitBlue/Runtime_134523/Runtime_134523.cs
new file mode 100644
index 00000000000..b6636232f97
--- /dev/null
+++ b/src/tests/JIT/Regression/JitBlue/Runtime_134523/Runtime_134523.cs
@@ -0,0 +1,248 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Diagnostics;
+using System.Reflection;
+using System.Runtime.CompilerServices;
+using System.Threading;
+using Xunit;
+
+// Guarded devirtualization must not drop the IL offset of the call it guards.
+// Each caller makes a single interface call whose result is used, so the
+// devirtualized call needs a return temp. Once Tier1 has guarded-devirtualized
+// the call site (the profile only ever sees Impl), the stack frame for the
+// caller must still report an IL offset inside the call's statement, as it
+// does at Tier0.
+public class Runtime_134523
+{
+ public interface IValue
+ {
+ int Inlined(int x);
+ int NotInlined(int x);
+ }
+
+ public sealed class Impl : IValue
+ {
+ // Inlineable: after inlining, the call to Check ends up in the
+ // statement created for the GDV return expression.
+ public int Inlined(int x) => Check(x);
+
+ // Not inlineable: GDV still devirtualizes the call and keeps it as a
+ // direct call stored to the return temp.
+ [MethodImpl(MethodImplOptions.NoInlining)]
+ public int NotInlined(int x) => Check(x);
+ }
+
+ // The shape reported in the issue: a delegate returns a holder, and the
+ // result of an inlined getter is dereferenced, so a null Data faults in
+ // this frame (hardware NullReferenceException).
+ public interface IHolder
+ {
+ object Data { get; }
+ }
+
+ public sealed class HolderImpl : IHolder
+ {
+ public object Data { get; set; }
+ }
+
+ [MethodImpl(MethodImplOptions.NoInlining)]
+ private static bool CallIssue(Func<IHolder> holderFactory, bool flag)
+ {
+ var holder = holderFactory();
+ return flag && holder.Data.ToString() != null;
+ }
+
+ // IL range of CallIssue's 'return flag && ...' statement: everything after the
+ // first statement (ldarg.0; callvirt Invoke; stloc.0).
+ private static (int Lo, int Hi) IssueStatementRange()
+ {
+ MethodInfo method = typeof(Runtime_134523).GetMethod(nameof(CallIssue), BindingFlags.NonPublic | BindingFlags.Static);
+ byte[] il = method.GetMethodBody().GetILAsByteArray();
+ Assert.True(il.Length > 7 && il[0] == 0x02 && il[1] == 0x6F && il[6] == 0x0A, "unexpected IL shape in CallIssue");
+ return (7, il.Length - 1);
+ }
+
+ private static int IssueFrameILOffset(Func<IHolder> holderFactory)
+ {
+ try
+ {
+ CallIssue(holderFactory, true);
+ }
+ catch (NullReferenceException ex)
+ {
+ foreach (StackFrame frame in new StackTrace(ex, false).GetFrames())
+ {
+ if (frame.GetMethod()?.Name == nameof(CallIssue))
+ {
+ return frame.GetILOffset();
+ }
+ }
+ }
+
+ Assert.Fail("expected a NullReferenceException from CallIssue");
+ return -1;
+ }
+
+ [MethodImpl(MethodImplOptions.NoInlining)]
+ private static int Check(int x)
+ {
+ if (x < 0)
+ {
+ throw new ArgumentOutOfRangeException(nameof(x));
+ }
+
+ return x;
+ }
+
+ [MethodImpl(MethodImplOptions.NoInlining)]
+ private static void Consume(int x) { }
+
+ [MethodImpl(MethodImplOptions.NoInlining)]
+ private static int CallInlined(IValue v, int x)
+ {
+ Consume(x);
+ return v.Inlined(x) + 1;
+ }
+
+ [MethodImpl(MethodImplOptions.NoInlining)]
+ private static int CallNotInlined(IValue v, int x)
+ {
+ Consume(x);
+ return v.NotInlined(x) + 1;
+ }
+
+ // IL range of the statement 'return v.X(x) + 1;': from just after the
+ // 'call Consume' instruction up to and including the 'callvirt'.
+ private static (int Lo, int Hi) CallStatementRange(string methodName)
+ {
+ MethodInfo method = typeof(Runtime_134523).GetMethod(methodName, BindingFlags.NonPublic | BindingFlags.Static);
+ byte[] il = method.GetMethodBody().GetILAsByteArray();
+ int lo = -1;
+ int hi = -1;
+ int i = 0;
+ while (i < il.Length)
+ {
+ switch (il[i])
+ {
+ case 0x28: // call <token>
+ if (lo < 0)
+ {
+ lo = i + 5;
+ }
+ i += 5;
+ break;
+ case 0x6F: // callvirt <token>
+ hi = i;
+ i += 5;
+ break;
+ default:
+ // Only one-byte opcodes without operands (ldarg.N, ldc.i4.1,
+ // add, ret, nop) occur otherwise in these two methods.
+ i += 1;
+ break;
+ }
+ }
+
+ Assert.True(lo > 0 && hi >= lo, $"unexpected IL shape in {methodName}");
+ return (lo, hi);
+ }
+
+ private static int ThrowingFrameILOffset(Func<IValue, int, int> caller, IValue v, string callerName)
+ {
+ try
+ {
+ caller(v, -1);
+ }
+ catch (ArgumentOutOfRangeException ex)
+ {
+ StackTrace trace = new StackTrace(ex, false);
+ foreach (StackFrame frame in trace.GetFrames())
+ {
+ if (frame.GetMethod()?.Name == callerName)
+ {
+ return frame.GetILOffset();
+ }
+ }
+
+ Assert.Fail($"no frame for {callerName} in:{Environment.NewLine}{trace}");
+ }
+
+ Assert.Fail("expected ArgumentOutOfRangeException");
+ return -1;
+ }
+
+ [Fact]
+ public static void TestEntryPoint()
+ {
+ IValue v = new Impl();
+ (int Lo, int Hi) inlinedRange = CallStatementRange(nameof(CallInlined));
+ (int Lo, int Hi) notInlinedRange = CallStatementRange(nameof(CallNotInlined));
+
+ // Tier0 reference: the frame lands inside the call statement.
+ Assert.InRange(ThrowingFrameILOffset(CallInlined, v, nameof(CallInlined)), inlinedRange.Lo, inlinedRange.Hi);
+ Assert.InRange(ThrowingFrameILOffset(CallNotInlined, v, nameof(CallNotInlined)), notInlinedRange.Lo, notInlinedRange.Hi);
+
+ // The issue's shape faults in CallIssue itself, in its second statement.
+ Func<IHolder> holderFactory = () => new HolderImpl();
+ (int Lo, int Hi) issueRange = IssueStatementRange();
+ Assert.InRange(IssueFrameILOffset(holderFactory), issueRange.Lo, issueRange.Hi);
+
+ // Drive the callers through instrumented Tier0 to Tier1 with a
+ // monomorphic class profile, so Tier1 guarded-devirtualizes the calls.
+ // If tier-up has not finished by the checks below, they run against
+ // Tier0 code and pass, so a slow machine can only make this vacuous.
+ for (int i = 0; i < 100; i++)
+ {
+ for (int j = 0; j < 1000; j++)
+ {
+ CallInlined(v, j);
+ CallNotInlined(v, j);
+ }
+
+ // As in the issue, warm CallIssue through the throwing path.
+ for (int j = 0; j < 100; j++)
+ {
+ try
+ {
+ CallIssue(holderFactory, true);
+ }
+ catch (NullReferenceException)
+ {
+ }
+ }
+
+ Thread.Sleep(20);
+ }
+
+ Thread.Sleep(200);
+
+ // Check every caller before asserting, so one failure does not hide another.
+ string failures = "";
+ for (int i = 0; i < 20; i++)
+ {
+ failures += CheckCaller(CallInlined, v, nameof(CallInlined), inlinedRange);
+ failures += CheckCaller(CallNotInlined, v, nameof(CallNotInlined), notInlinedRange);
+
+ int issueOffset = IssueFrameILOffset(holderFactory);
+ if ((issueOffset < issueRange.Lo) || (issueOffset > issueRange.Hi))
+ {
+ failures += $"{nameof(CallIssue)}: frame IL offset 0x{issueOffset:X} outside statement [0x{issueRange.Lo:X}, 0x{issueRange.Hi:X}]{Environment.NewLine}";
+ }
+ }
+
+ Assert.True(failures.Length == 0, failures);
+ }
+
+ private static string CheckCaller(Func<IValue, int, int> caller, IValue v, string callerName, (int Lo, int Hi) range)
+ {
+ int offset = ThrowingFrameILOffset(caller, v, callerName);
+ if ((offset >= range.Lo) && (offset <= range.Hi))
+ {
+ return "";
+ }
+
+ return $"{callerName}: frame IL offset 0x{offset:X} outside call statement [0x{range.Lo:X}, 0x{range.Hi:X}]{Environment.NewLine}";
+ }
+}
diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_134523/Runtime_134523.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_134523/Runtime_134523.csproj
new file mode 100644
index 00000000000..99b6b9b01e6
--- /dev/null
+++ b/src/tests/JIT/Regression/JitBlue/Runtime_134523/Runtime_134523.csproj
@@ -0,0 +1,22 @@
+<Project Sdk="Microsoft.NET.Sdk">
+ <PropertyGroup>
+ <Optimize>True</Optimize>
+ <!-- Needed for CLRTestEnvironmentVariable -->
+ <RequiresProcessIsolation>true</RequiresProcessIsolation>
+ <!-- Tiered compilation is disabled in interpreter mode. -->
+ <InterpreterIncompatible>true</InterpreterIncompatible>
+ <!-- Relies on Tiered PGO and on reading IL via MethodBody. -->
+ <NativeAotIncompatible>true</NativeAotIncompatible>
+ </PropertyGroup>
+ <ItemGroup>
+ <Compile Include="$(MSBuildProjectName).cs" />
+
+ <!-- GDV needs a class profile, so let Tiered PGO collect one quickly. -->
+ <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="1" />
+ <CLRTestEnvironmentVariable Include="DOTNET_TieredPGO" Value="1" />
+ <CLRTestEnvironmentVariable Include="DOTNET_TC_CallCountThreshold" Value="1" />
+ <CLRTestEnvironmentVariable Include="DOTNET_TC_CallCountingDelayMs" Value="0" />
+ <!-- A MinOpts stress leg would make Tier1 MinOpts (no GDV) and the test vacuous. -->
+ <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
+ </ItemGroup>
+</Project> |
Author
|
@pcshrosbree
Either option works for me. Let me know if there's anything you need me to do. |
Contributor
|
@anderspedersen Thanks. One PR is simplest, since the test only passes with both lines in place and I can't push to your branch. If you apply the patch from my comment above on top of |
Author
|
@dotnet-policy-service agree |
Author
|
@pcshrosbree I have added your changes to my branch |
Contributor
|
Thanks — confirmed a93e567 matches the patch exactly. Nothing further from me; over to the reviewers. |
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #134523
When doing GDV of a method with an return value, the compiler inserts a statement for the call with DebugInfo and a statement for the return expression without DebugInfo. Later, more logic from the devirtualized call is merged into the return expression statement, so we have a statement with logic from the devirtualized call, but with no IP mapping, so stack traces will point to first line of method, rather than the line containing the devirtualized call.
This PR fixes this by adding DebugInfor to the return expression also.