Reuse x86 GC and unwinding code in NativeAOT

[filipnavara]

Extracts x86 unwinding code and EnumGCRefs from eetwain.h/cpp into gc_unwind_x86.h/inl files. The bulk of the code remains unchanged with the methods UnwindStackFrameX86 and EnumGcRefsX86 extracted from the original EECodeManager methods.

CoffNativeCodeManager includes all the x86 GC decoders and includes gc_unwind_x86.inl for TARGET_X86. Missing methods are implemented in terms of UnwindStackFrameX86, EnumGcRefsX86, and DecodeGCHdrInfo.

Funclet calling assembly helpers are fixed up to follow the same pattern as on other platform.

For the purpose of the PR review the first commit moves the code from eetwain.cpp into gc_unwind_x86.inl verbatim. It doesn't compile. The second commit is the real viewable diff where the changes are actually visible without the move.

[filipnavara]

This likely should be changed back and bitvector.cpp needs to be added to NativeAOT/win-x86 build.

[jkotas]

BitVector does not seem to be used anywhere else. What is the advantage of using the BITVECTOR wrapper? Can we delete bitvector.h/bitvector.cpp and turn the handful of methods into internal helpers used by x86 GC info decoding only?

[filipnavara]

It is used for ptrArgTP which is in turn used in the x86 GC info decoder. I suppose it may be needed for methods with > 64 arguments. For the sake of simplicity I opted to go with !USE_BITVECTOR for the bring up but I suppose the BitVector is necessary for correctness.

[filipnavara]

Oh, if you mean to merge it with the rest of the extracted code, I guess that would make sense. I'd prefer to do it separately to keep it reviewable.

[jkotas]

Yes, I would move it together (and maybe renamed it too). It is not a general purpose bitvector. Also, delete the non-BitVector path.

It is fine to do it in a follow up PR.

[filipnavara]

....and, just as expected, I must have messed up something while cleaning this up and I broke win-x86 CoreCLR. Switching back to draft until I figure out what was it.

[janvorli]

@filipnavara I am OOF, I will review it in detail on Monday.

[janvorli]

Does pushing the ebp that the macro does in the middle of a function work well with the debugger stack unwinding from the actual funclet / the code inside of this macro after the ebp is pushed?

[filipnavara]

That's good question and I will try.

That said, the hack with custom object file that ProjectN used doesn't really transfer well and requires custom code all over the place. Since the thread hijacking works differently from ProjectN I don't expect this to hit the same issues. The NativeAOT unwinder used for exception handling, GC refs, and hijacking, can work with the ebp on the stack.

[filipnavara]

I suppose I can reserve one more stack slot through FUNCLET_CALL_PROLOGUE and use that instead of push / pop. Would that make sense?

[janvorli]

Maybe it is ok as is, so if you verify that stepping through that code keeps debugger showing correct stack trace, we can keep it as is.

[filipnavara]

Stack trace before the push ebp:

[0x0]   Exceptions!RhpCallCatchFunclet+0x44   0xd7f888   0x7b1b4b   
[0x1]   Exceptions!_ehinfo_System.Activator__CreateInstance<System___Canon>   0xd7f88c   0x56e5f4   
[0x2]   Exceptions!System.Runtime.EH__DispatchEx+0x5e4   0xd7f8a4   0x56def9   
[0x3]   Exceptions!System.Runtime.EH__RhThrowHwEx+0x149   0xd7f9b0   0x34428a   
[0x4]   Exceptions!RhpThrowHwEx2   0xd7f9e8   0x4ba181   
[0x5]   Exceptions!Exceptions_BringUpTest__TestGenericExceptions+0x21   0xd7faf0   0x56b87a   
[0x6]   Exceptions!System.Runtime.GCSettings__get_IsServerGC+0xa   0xd7fafc   0x0   

Stack trace after the mov ebp, eax:

[0x0]   Exceptions!RhpCallCatchFunclet+0x47   0xd7f884   0x4ba181   
[0x1]   Exceptions!Exceptions_BringUpTest__TestGenericExceptions+0x21   0xd7faf0   0x4b98fb   
[0x2]   Exceptions!Exceptions_BringUpTest__Main+0xbb   0xd7fb0c   0x6256c8   
[0x3]   Exceptions!Exceptions__Module___MainMethodWrapper+0x8   0xd7fc40   0x62572e   
[0x4]   Exceptions!Exceptions__Module___StartupCodeMain+0x5e   0xd7fc48   0x343cbb   
[0x5]   Exceptions!wmain+0x2b   0xd7fc68   0x414bd3   
[0x6]   Exceptions!invoke_main+0x33   0xd7fc7c   0x414aa7   
[0x7]   Exceptions!__scrt_common_main_seh+0x157   0xd7fc9c   0x41494d   
[0x8]   Exceptions!__scrt_common_main+0xd   0xd7fcf8   0x414c38   
[0x9]   Exceptions!wmainCRTStartup+0x8   0xd7fd00   0x75775d49   
[0xa]   KERNEL32!BaseThreadInitThunk+0x19   0xd7fd08   0x76f0bb1b   
[0xb]   ntdll!__RtlUserThreadStart+0x2b   0xd7fd18   0x76f0baa1   
[0xc]   ntdll!_RtlUserThreadStart+0x1b   0xd7fd70   0x0   

So, it is broken, but it is actually broken in the first part of the code.

[janvorli]

It would be interesting to investigate why the stack trace is broken before the push, but that's clearly unrelated to this PR.

[filipnavara]

I think I know how to fix it. I'll look at it once I flush the current patch queue.

[janvorli]

LGTM, thank you!