Nathan Froyd [:froydnj] Reporter
	Description • 6 years ago

We disabled the jit unwind handling for AArch64 Windows so we could get things working.  This bug is about re-enabling it, which will likely require a good deal more cleverness than "blanket this region with a single exception handler" strategy we employ on x86-64.

	Luke Wagner [:luke]
	Comment 1 • 6 years ago

Does the "blanket this region with a single exception handler" scheme not work on AArch64 Windows?  Because if we actually have to for-real unwind, then yes, it will take significantly more effort.  Talking to Chakra, it sounds like the easiest way is to preserve fp (bug 1426134) and then you can write somewhat-simple unwind logic with their mostly-undocumented JIT API.

	Nathan Froyd [:froydnj] Reporter
	Comment 2 • 6 years ago

(In reply to Luke Wagner [:luke] from comment #1)
> Does the "blanket this region with a single exception handler" scheme not
> work on AArch64 Windows?  Because if we actually have to for-real unwind,
> then yes, it will take significantly more effort.  Talking to Chakra, it
> sounds like the easiest way is to preserve fp (bug 1426134) and then you can
> write somewhat-simple unwind logic with their mostly-undocumented JIT API.

For inexplicable reasons (memory usage?), RUNTIME_FUNCTION, which we use here:

https://searchfox.org/mozilla-central/source/js/src/jit/ProcessExecutableMemory.cpp#105-110

and fill in here:

https://searchfox.org/mozilla-central/source/js/src/jit/ProcessExecutableMemory.cpp#135-145

is significantly different on AArch64 Windows vs. x86-ish.  The AArch64 RUNTIME_FUNCTION looks like it actually expects to cover a bona fide function:

typedef struct _IMAGE_ARM64_RUNTIME_FUNCTION_ENTRY {
    DWORD BeginAddress;
    union {
        DWORD UnwindData;
        struct {
            DWORD Flag : 2;
            DWORD FunctionLength : 11;
            DWORD RegF : 3;
            DWORD RegI : 4;
            DWORD H : 1;
            DWORD CR : 2;
            DWORD FrameSize : 9;
        } DUMMYSTRUCTNAME;
    } DUMMYUNIONNAME;
} IMAGE_ARM64_RUNTIME_FUNCTION_ENTRY, * PIMAGE_ARM64_RUNTIME_FUNCTION_ENTRY;

Always preserving FP on AArch64 is a lot more reasonable (so many registers!) than x86-ish processors, so that's probably the right way forward.  What's the mostly-undocumented JIT API?  RtlAddFunctionTable and friends?  Or something else?

	Luke Wagner [:luke]
	Comment 3 • 6 years ago

Ah, weird.  I can see why it would be nice to nail down enough of the ABI that one could unwind declaratively rather than calling some arbitrary callback.  But what determines whether the union's UnwindData vs. DUMMYSTRUCTNAME members are accessed?  I wonder if UnwindData is the escape hatch and we can use that.  (There's still the question of where EndAddress goes if UnwindData is used.)

Yeah, RtlAddFunctionTable and friends were, last I looked, rather undocumented.

	(Away) Assignee
	Comment 4 • 6 years ago

There's some documentation at https://docs.microsoft.com/en-us/cpp/build/arm64-exception-handling?view=vs-2017#arm64-exception-handling-information .

It looks like if DUMMYSTRUCTNAME.Flag == 0, then UnwindData is interpreted as an RVA to an exception information record.

Although instead of a direct EndAddress, it has an 18-bit field for FunctionLength/4, so we'd need a separate entry for each megabyte of code.

	(Away) Assignee
	Comment 5 • 6 years ago

(Or perhaps RtlInstallFunctionTableCallback could save us a bit of trouble by only dealing with these as needed.)

	Sean Stangl [:sstangl]
	Comment 6 • 6 years ago

Luke -- I don't know how helpful it will be, but ARM's implementation of their ABI logic in C++ is here: https://git.linaro.org/arm/vixl.git/tree/src/aarch64/abi-aarch64.h.

	(Away) Assignee
	Comment 7 • 6 years ago

I'm (slowly and intermittently) poking at this. Three options I'm exploring:

* 1024 separate RtlAddFunctionTable calls, one for each megabyte of the region. (We're not supposed to let region boundaries split an epilogue, but given that we're not doing proper unwinding, maybe we can sneak by.) I know the repetition sucks, but at least it keeps us on similar codepaths to existing tested code.

* An RtlInstallFunctionTableCallback that, when called, returns a RUNTIME_FUNCTION that does an analogous thing to the x86_64 code.

* An RtlInstallFunctionTableCallback that calls breakpad right then and there. This seems like the easiest option, but I'd want to convince myself that it's safe, e.g. NT isn't holding locks while using the callback. And also that the callback is only called when an exception happens, and not e.g. someone is RtlVirtualUnwind'ing us. ARM64 doesn't seem to have a concept of UNW_FLAG_EHANDLER etc.

	Luke Wagner [:luke]
	Comment 8 • 6 years ago

Agreed that the third bullet seems attractive.  We might even be able to switch the x64 path to use it, to get the full testing weight of x64 and reduce platform-specific differences.  I worry that we might miss (in normal automated testing) rare cases that call the callback when not handling an exception.

Is the idea for the second bullet that the PGET_RUNTIME_FUNCTION_CALLBACK would simply return a RUNTIME_FUNCTION that narrowly contained whatever the given pc was and so this could be considered a laziness optimization of the first bullet?

For the first bullet: agreed that it seems fine to arbitrarily slice functions overlapping the 1mb boundary, given that our RUNTIME_FUNCTION is simply calling breakpad.  We could also create these lazily, in jit::AllocateExecutableMemory() (which is a coarse-grained page allocator).  But this seems like more work than the second bullet.

	(Away) Assignee
	Comment 9 • 6 years ago

> * An RtlInstallFunctionTableCallback that calls breakpad right then and there.

Well, I just realized that function table callbacks aren't passed enough context to be able to call Breadkpad meaningfully -- no exception record, etc. So that option seems to be out.

	Mike Hommey [:glandium]
	Comment 10 • 6 years ago

Note: I changed the conditionals related to the JIT exception handler in bug 1514209, disabling the code on aarch64 for now.

	Eric Rahm [:erahm]
	Comment 11 • 6 years ago

Sounds like dmajor is looking at this.

	(Away) Assignee
	Comment 15 • 6 years ago

Because the .xdata format on ARM64 can only encode sizes up to 1M (much too small for our JIT code regions), we register a function table callback to provide RUNTIME_FUNCTIONs at runtime. Windows doesn't seem to care about the size fields on RUNTIME_FUNCTIONs that are created in this way, so the same RUNTIME_FUNCTION can work for any address in the region. We'll set up a generic one in RegisterExecutableMemory and the callback can just return a pointer to it.

	Pulsebot
	Comment 18 • 6 years ago

Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d932b82695c
Extend the Windows JIT unwind handler to ARM64 r=luke