add unwind information for xptcall assembly stubs

RESOLVED FIXED in Firefox 66

Status

()

defect
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks 2 bugs)

unspecified
mozilla67
ARM64
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed, firefox67 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

3 months ago

This is necessary so we can unwind through them for debugging and for crash reports. The ARM assembler supports some nice directives for doing this, but MSVC manually emits the necessary data for:

https://docs.microsoft.com/en-us/cpp/build/arm64-exception-handling?view=vs-2017

I think our code is simple enough that we can use packed unwind data, so a single dword of bits to describe what's going on, but it will require some manual counting of things.

Microsoft uses ksarm64.h to generate unwind information. ex. https://github.com/dotnet/coreclr/blob/master/src/debug/ee/arm64/dbghelpers.asm. NESTED_ENTRY, PROLOG_SAVE_REG_PAIR and etc emit unwind information to .xdata.

Assignee

Comment 2

3 months ago

(In reply to Makoto Kato [:m_kato] from comment #1)

Microsoft uses ksarm64.h to generate unwind information. ex. https://github.com/dotnet/coreclr/blob/master/src/debug/ee/arm64/dbghelpers.asm. NESTED_ENTRY, PROLOG_SAVE_REG_PAIR and etc emit unwind information to .xdata.

Thanks for pointing that out. I don't see where PROLOG_SAVE_REG_PAIR are defined, though. ksarm64.h just has a bunch of constant definitions, and asmmacros.h (from coreclr) just seems to have things that build on top of PROLOG_SAVE_REG_PAIR and such. Unless armasm64 has some macros built in, or something like that?

ksarm64.h (indirectly) includes kxarm64unw.h, where the macros are defined.

Also, armasm64 cannot read C headers, so you have to use C pre-processor before using armasm64. (MS's code adds custom rule to compile unwind macro for MSBuild.exe)

Attachment #9043917 - Attachment is obsolete: true
Attachment #9043925 - Attachment is obsolete: true
Assignee

Comment 7

3 months ago

We need to preprocess these files so we can eventually add unwind
information, for which we need to include C headers.

Comment 9

3 months ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a64ba70e327a
part 1 - preprocess aarch64 windows assembly xptcall files; r=dmajor
https://hg.mozilla.org/integration/autoland/rev/cd3c6c72d196
part 2 - generate unwind data for xptcall aarch64 windows routines; r=dmajor

Comment 10

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

(In reply to Pulsebot from comment #9)

Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a64ba70e327a
part 1 - preprocess aarch64 windows assembly xptcall files; r=dmajor

Sadly, this broke doing aarch64 artifact builds...

The problem is the build system doesn't know not to build generated_file targets that are compilation sources.

Depends on: 1529139
Assignee

Comment 13

3 months ago

Comment on attachment 9043945 [details]
Bug 1527471 - part 1 - preprocess aarch64 windows assembly xptcall files; r=dmajor

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

No user impact. Less information in crash reports, though.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Low risk, just some additional metadata in the binary. No functional changes.

Bug 1529139 is only necessary if we want to keep artifact builds working on beta...but I think aarch64 windows artifact builds are 67-only, so maybe we don't need that bug!

String changes made/needed

None

Attachment #9043945 - Flags: approval-mozilla-beta?

From discussion on #sheriffs sounds like they are being built but not shipped or published to archive.mozilla.org. So, maybe best to uplift the other patch as well so that the builds will keep on keeping on.

Comment on attachment 9043945 [details]
Bug 1527471 - part 1 - preprocess aarch64 windows assembly xptcall files; r=dmajor

Better crash information sounds good. Let's uplift for beta 11.

Attachment #9043945 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Note for sheriffs best not to uplift until we also have approval on bug 1529139.

You need to log in before you can comment on or make changes to this bug.