1530552 - register JIT code with Windows runtime on ARM64/x64, so that RtlVirtualUnwind can unwind jit frames

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Luke Wagner [:luke]]

If we preserve %fp (bug 1426134), it's apparently not too hard to create the Windows 64-bit runtime unwind data that allows RtlVirtualUnwind() to unwind through JIT frames. Apparently V8 switched to doing this recently.

There are a lot of advantages for making RtlVirtualUnwind Just Work on JIT code:

• Breakpad and the Gecko profiler will be able to reliably unwind through JIT frames to the C++ on the other side, giving us better stacks.
• The Gecko profiler could stop using JS::ProfilingFrameIterator altogether on Win64.
• Other Windows tools, like Xperf, Windows Performance Analyzer and the MSVC debugger, also won't get lost in JIT code

This would also enable further simplifications in the next bug.

Comment 1

[Nicolas B. Pierron [:nbp]]

Setting P3 as bug 1426134 is a P3 and a large piece of work.

Comment 3

[Markus Stange [:mstange]]

From bug 1381585:

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)

I've started using ETW more for profiling so this would be really nice to have. V8 supports this now:
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/diagnostics/unwinding-info-win64.cc;l=560;drc=fb9e129964cf89d7de13cd8f2a7bcaf6573fb8da

Comment 4

[Markus Stange [:mstange]]

I don't think this bug strictly depends on bug 1426134. Sure, it would be easier to create these records if bug 1426134 were already fixed, because all functions could use the same dead-simple "use the frame pointer" unwinding information. But as long as we know the stack size of each JIT function, we can create accurate unwind information even if we don't use frame pointers.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

See also https://bugs.chromium.org/p/v8/issues/detail?id=11043 and https://docs.google.com/document/d/1xkXj94iExFgLWc_OszTNyNGi523ARaKMWPZTeomhI4U/edit

Comment 6

[Jeff Muizelaar [:jrmuizel]]

It looks like V8 uses a frame pointer on AMD64 which makes there unwind info pretty simple: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/diagnostics/unwinding-info-win64.cc;l=109;drc=fb9e129964cf89d7de13cd8f2a7bcaf6573fb8da

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Chakra, DotNet and Mono all support more complicated unwind info than just a frame pointer:

Chakra: https://github.com/chakra-core/ChakraCore/blob/1eae003b7a981b4b691928daef27b5254a49f5eb/lib/Backend/amd64/PrologEncoderMD.cpp#L21
DotNet: https://github.com/dotnet/runtime/blob/c8eeba753183d07679b7d2e65cc12d39c948eb4a/src/coreclr/vm/stublink.cpp#L1684
Mono: https://github.com/mono/mono/blob/5977f1ca54572edc708eb2052bc5e1b7fb5802e7/mono/mini/exceptions-amd64.c#L1763

Comment 8

[Jeff Muizelaar [:jrmuizel]]

My understanding is that the Windows AMD64 stack walking info requires using a frame pointer if the stack pointer changes outside of the prologue or epilogue. (e.g. instead of using push, clang will store arguments directly into their location on the stack and switches to using a frame pointer if the size of the stack is unknown)

That means that it's not possible to describe the JIT code that we currently generate because it uses push instructions when calling functions. https://searchfox.org/mozilla-central/rev/3e21f2bb69734804d311e16b914c4efcc94bb18d/js/src/jit/CodeGenerator.cpp#5452

Given this, it seems like it's more practical to preserve the stack pointer on AMD64 than it is to switch to a more static stack layout.

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1530552: Tell Windows about our frame pointers in JIT code if we're on Windows > 8.1. r=jandem

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/114885c100cc
Tell Windows about our frame pointers in JIT code if we're on Windows > 8.1. r=jandem

Comment 11

[Sandor Molnar[:smolnar]]

Backed out for causing data race failures in /application/firefox/libxul.so

Backout link: https://hg.mozilla.org/integration/autoland/rev/1bd5a8a63ddf7405c938a36b84b624b430ec6be9

Push with failures

Failure log: SUMMARY: ThreadSanitizer: data race (/builds/worker/workspace/build/application/firefox/libxul.so+0x4f96000) (BuildId: 110491d6f22f3be42c61e0cfdfea5be93f1f6dda)

L.E. Backed out by mistake and relanded the patch

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40c430582786
Tell Windows about our frame pointers in JIT code if we're on Windows > 8.1. r=jandem

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/40c430582786