Remove uses of DECODE_RETURN_KIND part of GCInfo

[VSadov]

A follow-up to the GC info changes in 9.0.

Since the call sites now have complete liveness info, the part about return value kind[s] recorded in GC info is redundant.
Evidently, with CET enabled, hijacking already has no need for that.

This change removes all the uses of DECODE_RETURN_KIND in hijacking scenarios.

NOTE: The encoder still populates this part of GC info for backward-compat reasons. Decoder skips it.
Once we move forward the minimum R2R version, we can remove the return kind bits from the GC info format. This will save a few bits of GC info for each method and will allow more methods to use "slim" format.

X86: stays on the old plan.

[dotnet-policy-service]

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

[dotnet-policy-service]

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

[VSadov]

The key change.
On non-x86 we can treat a thread self-interrupted synchronously by a hijack the same as a thread interrupted asynchronously by a signal. The difference is the size of the captured context and who captured it.

In theory the hijack could capture the entire context as the OS interrupt mechanisms do, but that would be fairly redundant, so hijack captures only what we could possibly need.

There is a small catch here. If in the future we need to report more registers (say some ABI is extended to return up to 3 GC pointers), we would need to adjust OnHijackTripThread and HijackFrame::UpdateRegDisplay accordingly. If hijack captured the entire context as OS interrupts do, we would not have to adjust. Things would "just work".

I thought about this and, considering that a good portion of context cannot contain GC pointers at call site, not capturing the entire context seems the right approach.

[VSadov]

The PTFF_THREAD_HIJACK is the NativeAOT couterpart to the HijackFrame.
This is basically a way to mark hijacked frames as leaf/active/top frames.
(as opposed to regular transition frames, which are not leaf frames)

[jkotas]

Should these flags stay around for compat with SOS (see the comment at the top of the file)?

[VSadov]

Once we change the format it will be a breaking change. Diagnostics repo seems to take a copy of the decoder once that happens.
Otherwise, the flag is just for decoder API, it should not affect anything that does not use the new decoder.

I think this is ok to change, but we can use the old value to be sure. This is an int32, so no shortage of bits.

[jkotas]

Thanks!

[VSadov]

Thanks!!

[VSadov]

@jakobbotsch - once this gets merged with runtime async code, we will no longer need to do anything special for the continuation return in HijackFrame::GcScanRoots. However HijackFrame::UpdateRegDisplay will need to be adjusted to include context pointer to AsyncRet register in cases where it is not already included in the set of possible GC returns.

[jakobbotsch]

@VSadov Nice! Am I also correct in assuming that we'll need to adjust the GC info encoder to also encode the fact that there is a new register that can contain GC refs after a call returns?

[VSadov]

Am I also correct in assuming that we'll need to adjust the GC info encoder to also encode the fact that there is a new register that can contain GC refs after a call returns?

The encoder records everything that is live and contains GC refs, so we should not need to do anything special.
From GC info point of view hijacking is no different from asynchronously stopping a thread at call site (or, more precisely, at return IP), which should already work and already needs to know about all live GC refs at that location.

[jakobbotsch]

Am I also correct in assuming that we'll need to adjust the GC info encoder to also encode the fact that there is a new register that can contain GC refs after a call returns?

The encoder records everything that is live and contains GC refs, so we should not need to do anything special. From GC info point of view hijacking is no different from asynchronously stopping a thread at call site (or, more precisely, at return IP), which should already work and already needs to know about all live GC refs at that location.

So we will not need to make changes in e.g. GcInfoEncoder::DoNotTrackInPartiallyInterruptible?

[am11]

Should PTFF_THREAD_HIJACK be PTFF_THREAD_HIJACK_HI? It should be giving a compiler error because PTFF_THREAD_HIJACK is undefined.
Noticed in #110688 when I was resolving the merge conflicts.

[VSadov]

Right, this should be PTFF_THREAD_HIJACK_HI or PTFF_THREAD_HIJACK needs to be defined. Either way should work. It would be a trivial fix.

I'd be more worried whether this runs correctly. it should, but I had no way to verify.

[am11]

@shushanhf please check if the main branch build is fixed after changing it to PTFF_THREAD_HIJACK_HI. In riscv64, I have just used PTFF_THREAD_HIJACK_HI, but haven't tested the nativeaot implementation due to other issues we have in that port at the moment.

[VSadov]

So we will not need to make changes in e.g. GcInfoEncoder::DoNotTrackInPartiallyInterruptible?

@jakobbotsch Good point - DoNotTrackInPartiallyInterruptible is another place where we filter out what cannot contain object refs at return site. We do not have to do that filtering. If something does not contain a GC ref it will not end up in GC info. The filtering is an early-out optimization, and I am not sure it saves a lot, but there is filtering.

Yes, we need to be careful to be sure it does not filter out AsyncRet.

This is the same concern as with HijackFrame::UpdateRegDisplay. Need to be sure that we do not optimize/skip something that actually may contain a GC ref.