1527471 - add unwind information for xptcall assembly stubs

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Makoto Kato [:m_kato]]

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.

Comment 2

[Nathan Froyd [:froydnj]]

(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?

Comment 3

[(Away)]

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

Comment 4

[Makoto Kato [:m_kato]]

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)

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: Bug 1527471 - part 1 - preprocess aarch64 windows assembly xptcall files; r=dmajor

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: Bug 1527471 - part 1 - preprocess aarch64 windows assembly xptcall files; r=dmajor

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: Bug 1527471 - part 1 - preprocess aarch64 windows assembly xptcall files; r=dmajor

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

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: Bug 1527471 - part 2 - generate unwind data for xptcall aarch64 windows routines; r=dmajor

Depends on D19819

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/a64ba70e327a
https://hg.mozilla.org/mozilla-central/rev/cd3c6c72d196

Comment 11

[Mike Hommey [:glandium]]

(In reply to Pulsebot from comment #9)

Pushed by [e-mail]:
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...

Comment 12

[Mike Hommey [:glandium]]

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

Comment 13

[Nathan Froyd [:froydnj]]

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

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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 15

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/20db0e0d2373

Comment 18

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/37dca6044dcb
https://hg.mozilla.org/releases/mozilla-beta/rev/8d7fe5194e93