Closed Bug 1527471 Opened 6 years ago Closed 6 years ago

add unwind information for xptcall assembly stubs

Categories

(Core :: XPCOM, defect)

ARM64
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

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.

(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

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

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
Status: NEW → RESOLVED
Closed: 6 years 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

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.

Attachment

General

Created:
Updated:
Size: