Make FailFast blocks cold (GS cookies)

[EgorBo]

Make blocks with CORINFO_HELP_FAIL_FAST cold. Currently, we emit them in the codegen phase so it's too late to put them into a cold section, instead, I was using the same mechanism we use for Overflow/DividedByZero exceptions by creating blocks earlier and then wiring them up.

int Test()
{
    byte* p = stackalloc byte[16];
    Consume(p);
    return 42;
}

; Assembly listing for method Prog:Test():int:this (FullOpts)
G_M3272_IG01:  ;; offset=0x0000
       sub      rsp, 56
       xor      eax, eax
       mov      qword ptr [rsp+0x20], rax
       mov      qword ptr [rsp+0x28], rax
       mov      rax, 0x9ABCDEF012345678
       mov      qword ptr [rsp+0x30], rax
G_M3272_IG02:
       lea      rcx, [rsp+0x20]
       call     [Prog:Consume(ulong)]
       mov      eax, 42
       mov      rcx, 0x9ABCDEF012345678
       cmp      qword ptr [rsp+0x30], rcx
-      je       SHORT G_M3272_IG03
+      jne      SHORT G_M3272_IG04
-      call     CORINFO_HELP_FAIL_FAST
G_M3272_IG03:
-      nop      
-G_M3272_IG04:
       add      rsp, 56
       ret      
+G_M3272_IG04:
+      call     CORINFO_HELP_FAIL_FAST
+      int3   
; Total bytes of code 75

As a side-bonus: multiple gs checks can use the same BasicBlock for fail-fast

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Make blocks with CORINFO_HELP_FAIL_FAST cold. Currently, we emit them in the codegen phase so it's too late to put them into a cold section, instead, I was using the same mechamism we use for Overflow/DividedByZero exceptions by creating blocks earlier and then wiring them up.

int Test()
{
    byte* p = stackalloc byte[16];
    Consume(p);
    return 42;
}

; Assembly listing for method Prog:Test():int:this (FullOpts)
G_M3272_IG01:  ;; offset=0x0000
       sub      rsp, 56
       xor      eax, eax
       mov      qword ptr [rsp+0x20], rax
       mov      qword ptr [rsp+0x28], rax
       mov      rax, 0x9ABCDEF012345678
       mov      qword ptr [rsp+0x30], rax
G_M3272_IG02:
       lea      rcx, [rsp+0x20]
       call     [Prog:Consume(ulong)]
       mov      eax, 42
       mov      rcx, 0x9ABCDEF012345678
       cmp      qword ptr [rsp+0x30], rcx
-      je       SHORT G_M3272_IG03
+      jne      SHORT G_M3272_IG04
-      call     CORINFO_HELP_FAIL_FAST
G_M3272_IG03:
-      nop      
-G_M3272_IG04:
       add      rsp, 56
       ret      
+G_M3272_IG04:
+      call     CORINFO_HELP_FAIL_FAST
+      int3   
; Total bytes of code 75

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

@AndyAyersMS @dotnet/jit-contrib PTAL, Diffs

I expected size regressions (as usually happens when we move blocks to cold sections) but there were many size improvements, mostly, from re-using a single FAIL_FAST block from multiple GS checks. But the main goal is to improve hot code's density.

[AndyAyersMS]

Glad to see this, but I have one concern.

By using 0 for the SCK cache index we may have funclets branching back to the main function to fail fast. I suspect this will mess up unwinding. I also suspect we may not have tests for this case; can you find one or try and create one?

Perhaps fail fast is special and the runtime won't try and unwind.? If so it would be good to confirm this.

There's a bit of overlap here with #93371 but I'm still working on that one.

[EgorBo]

There's a bit of overlap here with #93371 but I'm still working on that one.

Oops, I didn't see your PR, what a coincidence 😄

[SingleAccretion]

By using 0 for the SCK cache index we may have funclets branching back to the main function to fail fast.

Cookie checks are only emitted for GT_RETURNs, which can only be part of the main function.

[AndyAyersMS]

Ah, ok. Good.

If all the vulnerable state is on the base method frame then there's no need to GS protect the funclet frame. And I suspect the runtime ensures that the base method cookie is intact before invoking the funclet, so we can't enter a funclet if the base frame is compromised.

[EgorBo]

I'll still check whether we have any tests for all (I am not even sure it's possible to test).
@AndyAyersMS if you want I can merge this after your PR is merged to avoid conflicts, it'd easier for me to handle them here

[AndyAyersMS]

I am not quite ready to push my PR forward, so don't wait for it...