Fix unwind info for cold EH funclets on ARM64 #1930

amanasifkhalid · 2022-07-08T18:00:38Z

Follow-up to #1923. When reserving and allocating unwind info on ARM64, we now consider the possibility that EH funclets are in the cold section. Below are some examples of reserving/allocating unwind info with this new behavior:

Main body split, no EH funclets:

reserveUnwindInfo(isFunclet=false, isColdCode=false, unwindSize=0xc)
reserveUnwindInfo(isFunclet=false, isColdCode=true, unwindSize=0x10)

allocUnwindInfo(pHotCode=0x00000231387F5530, pColdCode=0x0000000000000000, startOffset=0x0, endOffset=0x1c, unwindSize=0xc, pUnwindBlock=0x000002313AC97674, funKind=0 (main function))
allocUnwindInfo(pHotCode=0x00000231387F5530, pColdCode=0x00000231387F2A40, startOffset=0x0, endOffset=0x4c, unwindSize=0x10, pUnwindBlock=0x000002313AA80A57, funKind=0 (main function))

Main body split, with cold EH funclets:

reserveUnwindInfo(isFunclet=false, isColdCode=false, unwindSize=0x10)
reserveUnwindInfo(isFunclet=false, isColdCode=true, unwindSize=0x1c)
reserveUnwindInfo(isFunclet=true, isColdCode=true, unwindSize=0xc)

allocUnwindInfo(pHotCode=0x0000028CBABC5420, pColdCode=0x0000000000000000, startOffset=0x0, endOffset=0x58, unwindSize=0x10, pUnwindBlock=0x0000028CBD150D70, funKind=0 (main function))
allocUnwindInfo(pHotCode=0x0000028CBABC5420, pColdCode=0x0000028CBC82BC70, startOffset=0x0, endOffset=0x4d0, unwindSize=0x1c, pUnwindBlock=0x0000028CBD3DEC83, funKind=0 (main function))
allocUnwindInfo(pHotCode=0x0000028CBABC5420, pColdCode=0x0000028CBC82BC70, startOffset=0x4d0, endOffset=0x560, unwindSize=0xc, pUnwindBlock=0x0000028CBD150ED1, funKind=1 (handler))

Main body NOT split, with cold EH funclets:

reserveUnwindInfo(isFunclet=false, isColdCode=false, unwindSize=0x10)
reserveUnwindInfo(isFunclet=true, isColdCode=true, unwindSize=0xc)
reserveUnwindInfo(isFunclet=true, isColdCode=true, unwindSize=0xc)
reserveUnwindInfo(isFunclet=true, isColdCode=true, unwindSize=0xc)
reserveUnwindInfo(isFunclet=true, isColdCode=true, unwindSize=0xc)

allocUnwindInfo(pHotCode=0x0000024CCC700300, pColdCode=0x0000000000000000, startOffset=0x0, endOffset=0x588, unwindSize=0x10, pUnwindBlock=0x0000024CCE21BC27, funKind=0 (main function))
allocUnwindInfo(pHotCode=0x0000024CCC700300, pColdCode=0x0000024CCC7008F0, startOffset=0x0, endOffset=0x38, unwindSize=0xc, pUnwindBlock=0x0000024CCE21BD8C, funKind=1 (handler))
allocUnwindInfo(pHotCode=0x0000024CCC700300, pColdCode=0x0000024CCC7008F0, startOffset=0x38, endOffset=0x60, unwindSize=0xc, pUnwindBlock=0x0000024CCE21BEEC, funKind=1 (handler))
allocUnwindInfo(pHotCode=0x0000024CCC700300, pColdCode=0x0000024CCC7008F0, startOffset=0x60, endOffset=0x128, unwindSize=0xc, pUnwindBlock=0x0000024CCE21C04C, funKind=1 (handler))
allocUnwindInfo(pHotCode=0x0000024CCC700300, pColdCode=0x0000024CCC7008F0, startOffset=0x128, endOffset=0x160, unwindSize=0xc, pUnwindBlock=0x0000024CCE21C1AC, funKind=1 (handler))

I apologize if I'm stating the obvious here, but note that when the main body is split, unwindSize > 0 for the cold part's unwind info; it looks like we don't use chained unwind info on ARM64. If this is true, then we will likely have to modify the Scratch table's lookup implementation to correctly find cold functions/funclets on ARM64.

amanasifkhalid · 2022-07-08T18:00:57Z

@cshung @BruceForstall PTAL, thank you!

AndyAyersMS · 2022-07-11T22:22:01Z

it looks like we don't use chained unwind info on ARM64.

Is there some explanation for this?

amanasifkhalid · 2022-07-12T01:12:44Z

Is there some explanation for this?

I do not know the answer to this, but perhaps chained unwind info was never considered for ARM64 since hot/cold splitting wasn't implemented previously. It may be more complicated to implement here since we split each function/funclet into 512KB fragments, but the current crossgen/VM implementation relies on the use of chained unwind info, so this would improve portability.

BruceForstall · 2022-07-12T01:24:33Z

There's no concept of chained unwind for arm64.

Every function fragment (including "whole" functions) gets its own unique and separate RUNTIME_FUNCTION (.pdata) and .xdata entry (we never use the compact form; we always have .xdata). Funclets have both a prolog and epilog, so have both kinds of unwind codes. Fragments without a prolog have a "phantom" prolog. There is no reference from one to another. See https://docs.microsoft.com/en-us/cpp/build/arm64-exception-handling for details.

Chained unwind info for x64 is described here: https://docs.microsoft.com/en-us/cpp/build/exception-handling-x64

BruceForstall · 2022-07-12T01:25:06Z

nit: btw, fragments on arm64 are 1MB; on arm32 they are 512KB.

BruceForstall · 2022-07-12T17:33:05Z

src/coreclr/jit/unwind.cpp

+    if (isFunclet)
+    {
+        eeAllocUnwindInfo((BYTE*)pHotCode, (BYTE*)pColdCode, startOffset, endOffset, unwindCodeBytes, pUnwindBlock,
+                          (CorJitFuncKind)func->funKind);
+    }
+    else
+    {
+        // If the function is split, EH funclets are always cold, so pass in pColdCode to indicate so.
+        eeAllocUnwindInfo((BYTE*)pHotCode, nullptr /* pColdCode */, startOffset, endOffset, unwindCodeBytes,
+                          pUnwindBlock, (CorJitFuncKind)func->funKind);
+    }


I'm confused by this:

unwindEmitFuncCFI() should never get called, so why update it?

Do we have no incoming (function argument) indication if the funclet has a hot part? I guess we're currently assuming a funclet is always all cold. In that case, shouldn't we just skip the entire "hot" part here, and let the cold code below execute instead?

It seems confusing to me that the "else" branch comment isn't in the "if" part instead.

The cold code path below seems to be specifically for chained unwind info (see unwindCodeBytes = 0, pUnwindBlock = nullptr). I thought I would lay the groundwork here for if/when we enable hot/cold splitting on NativeAOT, though it's probably better we leave it unimplemented until we start testing it. I'll remove it.

BruceForstall · 2022-07-12T17:40:12Z

src/coreclr/jit/unwindarm.cpp

+    else
+    {
+        // If the function is split, EH funclets are always cold.
+        const bool isHotCode = (fgFirstColdBlock == nullptr);


Suggested change

const bool isHotCode = (fgFirstColdBlock == nullptr);

const bool isHotCode = (func->funKind == FUNC_ROOT) || (fgFirstColdBlock == nullptr);

then get rid of the if condition.

Thanks for catching that. This logic will probably be removed altogether in my next refactor, though.

BruceForstall · 2022-07-12T17:50:05Z

src/coreclr/jit/unwindarm.cpp

+    // func->uwiCold is used only for the cold part of the main function,
+    // and is always NULL for funclets.


There's something confusing about the model here. In particular, it looks like cold funclets have their unwind info in the "hot" uwi structure instead of having an empty uwi and putting their unwind info in the uwiCold structure.

Wouldn't it be better to introduce a notion of the "hot" unwind info (in uwi) being empty, if the entire funclet is cold?

One of the things I'm worried about here is distributing the logic about "if a function with EH is split then all funclets are cold" instead of just setting up the FuncInfoDsc with the right information and simply iterating over it.

Good point. I was thinking it would make more sense to use uwiCold for cold EH funclets, but the current implementation always utilizes uwi (and thus makes the assumption uwi has unwind info), so this may require some non-trivial refactoring. If we're okay with the higher churn, I can try this out.

Edit: I kept uwi initialized for cold funclets and just initialized uwiCold for them, and this allowed for a simpler implementation.

amanasifkhalid · 2022-07-12T21:49:16Z

Pushed changes addressing @BruceForstall's feedback. Most notably, cold EH funclets now use func->uwiCold instead of func->uwi; this separation helps preserve the existing logic. Below are some sample reserve/alloc unwind info calls from various JIT dumps:

Main body split, no funclets:

reserveUnwindInfo(isFunclet=false, isColdCode=false, unwindSize=0x18)
reserveUnwindInfo(isFunclet=false, isColdCode=true, unwindSize=0x10)

allocUnwindInfo(pHotCode=0x0000019474090540, pColdCode=0x0000000000000000, startOffset=0x0, endOffset=0x600, unwindSize=0x18, pUnwindBlock=0x000001947269E4E0, funKind=0 (main function))
allocUnwindInfo(pHotCode=0x0000019474090540, pColdCode=0x000001947261D120, startOffset=0x0, endOffset=0x60, unwindSize=0x10, pUnwindBlock=0x0000019474A04027, funKind=0 (main function))

Main body split, cold funclets:

reserveUnwindInfo(isFunclet=false, isColdCode=false, unwindSize=0x1c)
reserveUnwindInfo(isFunclet=false, isColdCode=true, unwindSize=0x10)
reserveUnwindInfo(isFunclet=true, isColdCode=true, unwindSize=0x10)

allocUnwindInfo(pHotCode=0x00000194725FA580, pColdCode=0x0000000000000000, startOffset=0x0, endOffset=0x42c, unwindSize=0x1c, pUnwindBlock=0x0000019474D1E8B4, funKind=0 (main function))
allocUnwindInfo(pHotCode=0x00000194725FA580, pColdCode=0x000001947435C4C0, startOffset=0x0, endOffset=0x100, unwindSize=0x10, pUnwindBlock=0x0000019474D1E7AF, funKind=0 (main function))
allocUnwindInfo(pHotCode=0x00000194725FA580, pColdCode=0x000001947435C4C0, startOffset=0x100, endOffset=0x190, unwindSize=0x10, pUnwindBlock=0x0000019474D1E928, funKind=1 (handler))

Main body NOT split, hot funclets:

reserveUnwindInfo(isFunclet=false, isColdCode=false, unwindSize=0x18)
reserveUnwindInfo(isFunclet=true, isColdCode=false, unwindSize=0xc)

allocUnwindInfo(pHotCode=0x000001947260F340, pColdCode=0x0000000000000000, startOffset=0x0, endOffset=0x2ec, unwindSize=0x18, pUnwindBlock=0x0000019474CC17E7, funKind=0 (main function))
allocUnwindInfo(pHotCode=0x000001947260F340, pColdCode=0x0000000000000000, startOffset=0x2ec, endOffset=0x324, unwindSize=0xc, pUnwindBlock=0x0000019472630AB4, funKind=1 (handler))

Main body NOT split, cold funclets:

reserveUnwindInfo(isFunclet=false, isColdCode=false, unwindSize=0x10)
reserveUnwindInfo(isFunclet=true, isColdCode=true, unwindSize=0x10)
reserveUnwindInfo(isFunclet=true, isColdCode=true, unwindSize=0x10)
reserveUnwindInfo(isFunclet=true, isColdCode=true, unwindSize=0x10)
reserveUnwindInfo(isFunclet=true, isColdCode=true, unwindSize=0x10)

allocUnwindInfo(pHotCode=0x00000194743580C0, pColdCode=0x0000000000000000, startOffset=0x0, endOffset=0x588, unwindSize=0x10, pUnwindBlock=0x00000194726344B7, funKind=0 (main function))
allocUnwindInfo(pHotCode=0x00000194743580C0, pColdCode=0x0000019474F4A490, startOffset=0x0, endOffset=0x38, unwindSize=0x10, pUnwindBlock=0x00000194726D08BF, funKind=1 (handler))
allocUnwindInfo(pHotCode=0x00000194743580C0, pColdCode=0x0000019474F4A490, startOffset=0x38, endOffset=0x60, unwindSize=0x10, pUnwindBlock=0x00000194726D0A1F, funKind=1 (handler))
allocUnwindInfo(pHotCode=0x00000194743580C0, pColdCode=0x0000019474F4A490, startOffset=0x60, endOffset=0x128, unwindSize=0x10, pUnwindBlock=0x00000194726D0B7F, funKind=1 (handler))
allocUnwindInfo(pHotCode=0x00000194743580C0, pColdCode=0x0000019474F4A490, startOffset=0x128, endOffset=0x160, unwindSize=0x10, pUnwindBlock=0x00000194726D0CCF, funKind=1 (handler))

BruceForstall · 2022-07-13T21:07:25Z

src/coreclr/jit/unwind.cpp

@@ -311,7 +311,6 @@ void Compiler::unwindEmitFuncCFI(FuncInfoDsc* func, void* pHotCode, void* pColdC
 #endif // DEBUG

    assert(endOffset <= info.compTotalHotCodeSize);
-


Looks like the only change in unwind.cpp is an arbitrary whitespace change. Maybe remove unwind.cpp from the PR.

BruceForstall requested changes Jul 12, 2022

View reviewed changes

amanasifkhalid force-pushed the arm64-unwind-info branch from b29e63a to c3c6ada Compare July 12, 2022 21:45

BruceForstall approved these changes Jul 13, 2022

View reviewed changes

Fix unwind info for cold EH funclets on ARM64

793abf5

amanasifkhalid force-pushed the arm64-unwind-info branch from c3c6ada to 793abf5 Compare July 13, 2022 21:29

amanasifkhalid merged commit 37939e3 into dotnet:feature/hot-cold-splitting Jul 13, 2022

amanasifkhalid deleted the arm64-unwind-info branch July 13, 2022 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unwind info for cold EH funclets on ARM64 #1930

Fix unwind info for cold EH funclets on ARM64 #1930

amanasifkhalid commented Jul 8, 2022

amanasifkhalid commented Jul 8, 2022

AndyAyersMS commented Jul 11, 2022

amanasifkhalid commented Jul 12, 2022

BruceForstall commented Jul 12, 2022

BruceForstall commented Jul 12, 2022

BruceForstall Jul 12, 2022

amanasifkhalid Jul 12, 2022

BruceForstall Jul 12, 2022

amanasifkhalid Jul 12, 2022

BruceForstall Jul 12, 2022

amanasifkhalid Jul 12, 2022 •

edited

Loading

amanasifkhalid commented Jul 12, 2022

BruceForstall Jul 13, 2022

	const bool isHotCode = (fgFirstColdBlock == nullptr);
	const bool isHotCode = (func->funKind == FUNC_ROOT) \|\| (fgFirstColdBlock == nullptr);

		// func->uwiCold is used only for the cold part of the main function,
		// and is always NULL for funclets.

		@@ -311,7 +311,6 @@ void Compiler::unwindEmitFuncCFI(FuncInfoDsc* func, void* pHotCode, void* pColdC
		#endif // DEBUG

		assert(endOffset <= info.compTotalHotCodeSize);

Fix unwind info for cold EH funclets on ARM64 #1930

Fix unwind info for cold EH funclets on ARM64 #1930

Conversation

amanasifkhalid commented Jul 8, 2022

amanasifkhalid commented Jul 8, 2022

AndyAyersMS commented Jul 11, 2022

amanasifkhalid commented Jul 12, 2022

BruceForstall commented Jul 12, 2022

BruceForstall commented Jul 12, 2022

BruceForstall Jul 12, 2022

Choose a reason for hiding this comment

amanasifkhalid Jul 12, 2022

Choose a reason for hiding this comment

BruceForstall Jul 12, 2022

Choose a reason for hiding this comment

amanasifkhalid Jul 12, 2022

Choose a reason for hiding this comment

BruceForstall Jul 12, 2022

Choose a reason for hiding this comment

amanasifkhalid Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

amanasifkhalid commented Jul 12, 2022

BruceForstall Jul 13, 2022

Choose a reason for hiding this comment

amanasifkhalid Jul 12, 2022 •

edited

Loading