Add DebuggerHidden to managed JitHelpers

[AaronRobinsonMSFT]

Looking for thoughts on this before I move it out of draft.

/cc @dotnet/dotnet-diag @jkotas

[stephentoub]

What's the intended user experience in the debugger this is aiming to achieve?

[jkotas]

I do not think that this makes to apply wholesale to all throw helpers.

If this is necessary to make the recently introduce managed JIT throw helpers to behave more like the unmanaged JIT throw helpers, we should move the JIT throw helpers into a different type and only apply DebuggerHidden to methods in that type. There will be a bit of code duplication and that's fine.

[noahfalk]

Same thought as @stephentoub, its helpful to understand where these helpers are going to be used and what aspect of the debugging experience we are trying to change. My default principle is that what users see in the stack should match what they'd see reading the source code. So if the BCL C# code calls the helpers I'd prefer not to have those helpers hidden. If the JIT is inserting the helper where no such call exists in the source that would be a good place to hide it. I think @jkotas suggestion alludes to the same principle.

[AaronRobinsonMSFT]

This was suggested after trying to fix a diagnostic issue. We do seem slightly inconsistent when we apply this or not, so I just applied to all JitHelpers. The arguments presented by both @jkotas and @noahfalk make sense to me though. I figured it was appropriate to hide them in all cases. Seems that is not the case.

@tommcdon Did I misunderstand the suggestion?

[jkotas]

I just applied to all JitHelpers

ThrowHelpers are not the same as JitHelpers.

[tommcdon]

@tommcdon Did I misunderstand the suggestion?

#105671 added an extra managed frame on to the stack for certain exceptions, regressing the developer experience. For example with Just My Code off, we are now seeing a managed frame on the stack for IndexOutOfRangeException on array access:

System.ThrowHelper.ThrowIndexOutOfRangeException (source line information unavailable)
ExceptionSpace.MyException._Generate_Exception0 (Exceptions.cs:59)
ExceptionSpace.MyException.Main (Exceptions.cs:29)

We are seeing the same behavior with DivideByZeroException on ARM.
I think it makes sense to hide these frames from the debugger as they are implementation detail to the runtime. The debugger already hides native throw helpers in the runtime, and so I think it makes sense to hide throw helpers that were previously implemented in native code prior to #105671.

[jkotas]

Right, I think the fix for this issue should be:

• Introduce a new type for the jit throw helpers. The throw helpers in this type should be only used as jit throw helpers, they should not be called directly from other CoreLib code as general purpose throw helpers
• Mark this new type and the methods in the type the same way as other managed jit helpers: https://github.com/dotnet/runtime/blob/main/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs
• Bonus points for deduplicating this new type with the native AOT JIT throw helpers (https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs). It does not need to be done in the same PR.

[AaronRobinsonMSFT]

I just applied to all JitHelpers

ThrowHelpers are not the same as JitHelpers.

Sure, that is fair. Several of the functions in this file are also JitHelpers. I naively applied it to all as if they were. I'll more narrowly apply DebuggerHidden.

Introduce a new type for the jit throw helpers.

Will do.

[AaronRobinsonMSFT]

@jkotas I took your suggestion. I'm sure I missed something so this is likely to fail on native AOT or something, but can you take a look at the general direction?

[AaronRobinsonMSFT]

All green. @jkotas and @dotnet/dotnet-diag Please take another look.

[tommcdon]

LGTM, thanks Aaron!

[jkotas]

LGTM otherwise

[noahfalk]

Oops, sorry I reviewed yesterday but apparently forgot to hit submit :(

[AaronRobinsonMSFT]

Ugh. This is all sorts of messed up. I'll need to sit down and change more of this. There are API compat and protection levels issues when I change native AOT to internal.

For example:

.../runtime/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/ThrowHelper.cs(4,55): error CS0122: 'ThrowHelpers' is inaccessible due to its protection level [.../runtime/src/coreclr/nativeaot/System.Private.TypeLoader/src/System.Private.TypeLoader.csproj]

[jkotas]

runtime/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/TypeSystem/ThrowHelper.cs(4,55): error CS0122: 'ThrowHelpers' is inaccessible due to its protection level

You can fix this by changing the NAOT TypeLoader to call RuntimeAugments, and move the throw helpers that are needed by the type loader there.

I think Internal\Runtime\CompilerHelpers\ThrowHelpers.cs should be only used for ThrowHelpers that are called by the JITed code directly. It should not be used for cases where some random code needs to throw an exception.

[AaronRobinsonMSFT]

Updated.

[jkotas]

LGTM