Fix debugger thread context validation after recent change

[kouvel]

Followup fix to #55185

[kouvel]

@noahfalk, I seem to have broken this path in a recent PR that made it into preview 7, for the currently typical case where CET is disabled. As a result, the check for Debugger::IsThreadContextInvalid would always say that the thread context is invalid when a debugger is attached, preventing the suspension from happening while the thread is in managed code. Seems like it has a potential to be problematic, do you feel that a fix should be ported back to preview 7?

[janvorli]

LGTM

[noahfalk]

LGTM

[noahfalk]

preventing the suspension from happening while the thread is in managed code

This sounds like the debugger would frequently (never?) be able to suspend, such as when hitting breakpoints or stepping. Is that the right way to think about it or it is less severe?

do you feel that a fix should be ported back to preview 7?

If debugging is pretty badly broken (it sounds like it could be?) and this fix seems very low risk, then yeah, I think we should port it back if possible. Do you know what the process is for that or we could Manish/Jeff.

[noahfalk]

fyi @dotnet/dotnet-diag

[kouvel]

This sounds like the debugger would frequently (never?) be able to suspend, such as when hitting breakpoints or stepping. Is that the right way to think about it or it is less severe?

I think it's a bit less severe but it could be bad at times. To be more specific when a thread is trying to suspend another thread that is constantly (on every attempt to suspend) in interruptible managed code, it would fail to suspend (if it's not in an interruptible region we would use return address hijacking, which I think would succeed). Here's an example, somehow for this test it seems to hang on attach even before breaking the process:

static class Program
{
    private static void Main()
    {
        var t = new Thread(Foo);
        t.IsBackground = true;
        t.Start();
        Foo();
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Foo()
    {
        while (true)
        {
        }
    }
}

I'll go ahead and port it back. I believe the process is the same as for servicing.

[kouvel]

Also I'm not sure where or how frequently GC polls would occur but they would also allow a thread to suspend

[noahfalk]

Ah, if this is only for interruptible code I am much less concerned (and it aligns with not having seen our debugger tests blow up). If it is easy port back great, if it laborious or tactics wants to hold a higher bar I wouldn't push too hard. Thanks Kount!

[kouvel]

It seems I was mistaken. The check for a valid thread context is done before return address hijacking also, so suspension would fail whenever the thread remains in managed code for an extended time, interruptible or not. If the thread happens to switch GC modes at some point or hits a GC poll, it would suspend at that point.

[kouvel]

There seems to be one new failure in the diagnostics tests from the issue, I've confirmed that it's fixed after this change.

[noahfalk]

Thanks @kouvel! Appreciate it : )