Closed Bug 1480549 Opened Last year Closed Last year

add xptcall routines for aarch64 Windows

Categories

(Core :: XPCOM, enhancement)

ARM64
Windows
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

We could get by with stub files for an initial build, but to run this at all, we're going to need something more sophisticated.  dmajor is supposedly doing this?
Since the Windows ABI on this platform just uses the ARM procedure call
standard, which is the same standard Unix uses, we can just
copy-and-paste everything, with some adjustments to the assembly code to
compile properly.

This code compiles, at least.  I make no claims as to it working, but it
*should* Just Work.
Attachment #9004992 - Flags: review?(dmajor)
Comment on attachment 9004992 [details] [diff] [review]
add aarch64 Windows support to xptcall

Review of attachment 9004992 [details] [diff] [review]:
-----------------------------------------------------------------

I diffed these against their unix/ counterparts, so this is mostly a review of the differences.

::: xpcom/reflect/xptcall/md/win32/xptcinvoke_aarch64.cpp
@@ +6,5 @@
> +/* Platform specific code to invoke XPCOM methods on native objects */
> +
> +#include "xptcprivate.h"
> +
> +#if !defined(_MSC_VER) && !defined(_M_ARM64)

This should be `||` right? Or if you think it's clearer, !(L && R)

::: xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_aarch64.asm
@@ +3,5 @@
> +; file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +	AREA	|.drectve|, DRECTVE
> +	DCB	"-defaultlib:LIBCMT "
> +	DCB	"-defaultlib:OLDNAMES "

Is this .drectve piece required? I'm not seeing why this particular file would care about libraries; this looks pretty self-contained.

::: xpcom/reflect/xptcall/md/win32/xptcstubs_aarch64.cpp
@@ +199,5 @@
> +
> +#include "xptcstubsdef.inc"
> +
> +void
> +xptc_dummy()

What's the purpose of xptc_dummy? I don't see it called anywhere: https://searchfox.org/mozilla-central/search?q=xptc_dummy
Attachment #9004992 - Flags: review?(dmajor) → review+
(In reply to David Major [:dmajor] from comment #2)
> Comment on attachment 9004992 [details] [diff] [review]
> add aarch64 Windows support to xptcall
> 
> Review of attachment 9004992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I diffed these against their unix/ counterparts, so this is mostly a review
> of the differences.
> 
> ::: xpcom/reflect/xptcall/md/win32/xptcinvoke_aarch64.cpp
> @@ +6,5 @@
> > +/* Platform specific code to invoke XPCOM methods on native objects */
> > +
> > +#include "xptcprivate.h"
> > +
> > +#if !defined(_MSC_VER) && !defined(_M_ARM64)
> 
> This should be `||` right? Or if you think it's clearer, !(L && R)

`||` would skip the #error for clang compiling this file, for instance.  I think `&&` is correct.

> ::: xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_aarch64.asm
> @@ +3,5 @@
> > +; file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +	AREA	|.drectve|, DRECTVE
> > +	DCB	"-defaultlib:LIBCMT "
> > +	DCB	"-defaultlib:OLDNAMES "
> 
> Is this .drectve piece required? I'm not seeing why this particular file
> would care about libraries; this looks pretty self-contained.

I don't know.  I was cargo-culting from what cl.exe spits out, but I think the assembly file for the ICU data file doesn't have the .drective stuff.  I can take it out.

> ::: xpcom/reflect/xptcall/md/win32/xptcstubs_aarch64.cpp
> @@ +199,5 @@
> > +
> > +#include "xptcstubsdef.inc"
> > +
> > +void
> > +xptc_dummy()
> 
> What's the purpose of xptc_dummy? I don't see it called anywhere:
> https://searchfox.org/mozilla-central/search?q=xptc_dummy

I'm not sure!  I was cargo-culting it from the x86-64 file, but I guess we could just lose it?
Assignee: dmajor → nfroyd
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/694735f79cb7
add aarch64 windows support to xptcall; r=dmajor
> > > +#if !defined(_MSC_VER) && !defined(_M_ARM64)
> > 
> > This should be `||` right? Or if you think it's clearer, !(L && R)
> 
> `||` would skip the #error for clang compiling this file, for instance.  I
> think `&&` is correct.

clang proper, yes, but clang-cl would be fine. As written, I could also run this through x86 MSVC or x86 clang-cl, and it wouldn't hit the #error.
https://hg.mozilla.org/mozilla-central/rev/694735f79cb7
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.