Change debug info tracking system default

[BruceForstall]

Due to a test failure (#32951), the original change to the debug info tracking system default (#2107) was reverted (#33021).

This was due to a bug where the recently-added coalescing algorithm attempts to look backwards in an instruction group (insGroup) to the previous instruction and attempt to coalesce adjacent identical ranges. This fails when there are "emitAdd" (overflow) instruction groups. More details are found in the issue #32951.

This issue tracks re-enabling the new debug info tracking system, and fixing the new coalescing algorithm.

category:implementation
theme:debug-info
skill-level:beginner
cost:small

[BruceForstall]

cc @echesakovMSFT @BrianBohe

[BrianBohe]

Hi @echesakovMSFT @BruceForstall

Is anyone following this?

[BruceForstall]

@BrianBohe Nobody is working on this right now.

[AndyAyersMS]

We think the change has demonstrable impact on the quality of debug info for optimized builds, right?

If so, we should get this fixed for .Net 5, and I'd recommend trying to fix it relatively soon as changes like this often have subtle interactions that can take a while to surface.

[AndyAyersMS]

@tommcdon @noahfalk @jkotas wanted to check with you about priority of addressing this for 5.0. It should improve debugging experience in optimized code, but hard to say exactly where or by how much.

My impression is that debugging of optimized code is something we don't regularly test, so quantifying regression vs improvement from something like this (and guarding against future regressions) are not well-addressed problems . I wonder if we should try to develop some better ways of measuring first...?

[jkotas]

Is the new debug info tracking system used just for optimized code; or is it used for both unoptimized and optimized code?

I have spot checked number of debug info deltas with the new debug info tracking system, and all of them were clear improvements. I would ok with taking this, even without having exact methodology to measure the improvement.

As to whether to take it for .NET 5 or later, I do not understand how hard it is to fix the problem that caused this to be reverted.

[AndyAyersMS]

My impression is the debug emission is dependent on the jit's instruction group boundaries which can vary when compiling vs cross compiling. The semantic impact of the debug info is the same, it's just that sometimes a live range is broken up into contiguous parts instead of a single range.

Not sure how hard this is to fix this either -- let me try and work up some sort of estimate.

[echesakov]

My impression is the debug emission is dependent on the jit's instruction group boundaries which can vary when compiling vs cross compiling. The semantic impact of the debug info is the same, it's just that sometimes a live range is broken up into contiguous parts instead of a single range.

Yes, this happens due to the different number of instrDesc per IG on cross-arch vs native compilers.

Another issue was that when computing the live range that crosses boundaries of 2 IGs in case when the second IG is an "overflow" IG.

[AndyAyersMS]

Is the new debug info tracking system used just for optimized code; or is it used for both unoptimized and optimized code?

It looks like it is used for both debug and optimized code (see eg siOpenScopesForNonTrackedVars). So there is some risk of changing/regressing debug info for debug codegen.

We can force the jit produce debug codegen and use PMI/SPMI to screen for possible diffs in the debug info produced.

Yes, this happens due to the different number of instrDesc per IG on cross-arch vs native compilers.
Another issue was that when computing the live range that crosses boundaries of 2 IGs in case when the second IG is an "overflow" IG.

If live range merging were updated to handle overflow IGs it would address both these issues, right?

Seems like a more general merging algorithm could ignore IGs all together and (likely) produce even more compact debug info independent of IGs; after all IGs are a jit artifact. I wonder if we could merge just before reporting rather than opportunistically merging when creating (or do both perhaps, and rely on the second merge to clean up the issues above).

[noahfalk]

Quantifying improvement ad-hoc can probably be done relatively quickly, either by looking at example diffs in the debug data or by debugging typical snippets of C# in the VS debugger. My understanding is that @davmason and @BrianBohe did that as part of the project. If there is concern that the current results wouldn't match the old ones the testing could be repeated.

In terms of regression prevention I don't think we have to gate re-enabling the feature on this, but two potential strategies:

1. If the JIT can guarantee that certain invariants about optimized debug data will be true then we could make debugger tests with release compiled debugees and verify that source mapping/variable home information adheres to those invariants.
2. The JIT could include debug data in jit diffs and JIT devs would validate that changes to it are appropriate, in the same manner that JIT devs validate changes to generated code are appropriate. In the past I think some concerns were raised that doing this comparison was challenging or JIT devs didn't understand the impact of changes. I'd be happy to brainstorm with you on solutions such as tooling improvements or growing JIT team knowledge.

[echesakov]

If live range merging were updated to handle overflow IGs it would address both these issues, right?

@AndyAyersMS I think so.

[AndyAyersMS]

If the JIT can guarantee that certain invariants about optimized debug data

I think we are a ways off from something like this, though for very simple methods we should have fairly stable debug info.

The JIT could include debug data in jit diffs

This is already an option, eg jit-diff diff --debugInfo. Not super happy with this overall (see #11045 and dotnet/jitutils#166) -- if there are also codegen changes then it is hard to assess the debug info diffs. But in this particular case we don't expect codegen diffs.

[AndyAyersMS]

Prospective fix... need to double-check if the count in the new clause should be 1 or 0.

 bool emitLocation::IsPreviousInsNum(const emitter* emit) const
 {
     assert(Valid());
-    bool isSameGroup  = (ig == emit->emitCurIG);
-    bool isSameInsNum = (emitGetInsNumFromCodePos(codePos) == emitGetInsNumFromCodePos(emit->emitCurOffset()) - 1);
-    return isSameGroup && isSameInsNum;
+
+    // Within the same IG?
+    if (ig == emit->emitCurIG)
+    {
+        return (emitGetInsNumFromCodePos(codePos) == emitGetInsNumFromCodePos(emit->emitCurOffset()) - 1);
+    }
+
+    // Spanning an "extended" IG boundary?
+    if ((ig->igNext == emit->emitCurIG) && ((emit->emitCurIG->igFlags & IGF_EXTEND) != 0))
+    {
+        return (emitGetInsNumFromCodePos(codePos) == ig->igInsCnt) && (emit->emitCurIGinsCnt == 0);
+    }
+
+    return false;
 }

[AndyAyersMS]

After debugging a bit -- I see no reason to limit merging to IGF_EXTEND; allowing any prev/next pair catches a few more cases.

Still see some oddities in debug output that I want to look at more closely.

[AndyAyersMS]

One source of oddities (see for example Kernel32:GetAndTrimString in SPC, crossgenned) is that we keep the prolog ranges separate from the body ranges, so if an arg is live into the body and remains in its arg register, it gets two ranges instead of one.

3 live ranges
  0(   UNKNOWN) : From 00000000h to 0000000Bh, in rcx
  0(   UNKNOWN) : From 0000000Bh to 0000000Eh, in rcx
  1(   UNKNOWN) : From 00000013h to 0000003Ah, in rdx

We generate the prolog out of order so allowing the prolog ranges to fuse with body ranges would take some tweaking; seems like we can leave this as a future enhancement.

[AndyAyersMS]

Another is that methods with zero-length prologs will end up with overlapping live range reports (for instance, in SYSTEMTIME:Equals(byref):bool:this):

4 live ranges
  0(   UNKNOWN) : From 00000000h to 00000001h, in rcx
  0(   UNKNOWN) : From 00000000h to 00000044h, in rcx
  1(   UNKNOWN) : From 00000000h to 00000001h, in rdx
  1(   UNKNOWN) : From 00000000h to 00000048h, in rdx

Presumably we could suppress these special "1 byte" prolog entries if we saw the same variable live in the non-prolog entry list at offset 0.

This same behavior shows up in the baseline jit, so it's not a regression/change from the new debug emission.

[AndyAyersMS]

In Globalization:InitICUFunctions we see

IN0019: 000015 mov      rsi, r9
IN0001: 000018 mov      rcx, r8
IN0002: 00001B call     [ReadOnlySpan`1:ToString():String:this]
IN0003: 000021 mov      r14, rax
IN0004: 000024 cmp      dword ptr [rsi+8], 0
IN0005: 000028 jg       SHORT G_M29980_IG04

G_M29980_IG03:

IN0006: 00002A xor      r9, r9
IN0007: 00002D jmp      SHORT G_M29980_IG05

G_M29980_IG04:

IN0008: 00002F mov      rcx, rsi
IN0009: 000032 call     [ReadOnlySpan`1:ToString():String:this]
IN000a: 000038 mov      r9, rax

with debug

  3(   UNKNOWN) : From 00000000h to 00000018h, in r9
  3(   UNKNOWN) : From 00000018h to 0000002Ah, in rsi
  3(   UNKNOWN) : From 0000002Fh to 00000030h, in rsi

Note we could arguably fuse these last two live ranges, if we adopted the same sort of lazy kill model for variable tracking that we use for GC refs. That is, in IG_03 var 3 is no longer live, but rsi still has the right value.

[AndyAyersMS]

Was aiming for getting this into P8 but now seems unlikely:

• Have hit at least one blocking issue trying to re-enable this (plus prospective fix), see Re-enable improvements to debug emission for optimized code #38894 (comment)
• Haven't yet verified that my prospective fix actually handles the cross-crossgen debug diff that lead this to be reverted.

Seems like this is at risk for not making it into .Net 5. Not sure how we'd feel about enabling it after P8.

[jkotas]

Agree - this should wait post .NET 5.

[AndyAyersMS]

Ok, I'm going to change the milestone.

[EgorBo]

Re-enabled in #50162