1318905 - Changes to make SpiderMonkey build on Linux/RISC-V

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Stefan O'Rear]

Attachment: 0001-update-config-guess.patch

These changes allow SpiderMonkey to build in isolation under riscv64-unknown-linux-gnu using the Fedora toolchain and the qemu emulator.  They are based on hg rev 57a8cde3f08c.

Patch #5 is functionally equivalent to https://bugzilla.mozilla.org/show_bug.cgi?id=1317033#c16 and probably should not be used; I am including it to document what I used.

Comment 1

[Stefan O'Rear]

Attachment: 0002-update-double-conversion.patch

Comment 2

[Stefan O'Rear]

Attachment: 0003-riscv-build-changes.patch

Comment 3

[Stefan O'Rear]

Attachment: 0004-riscv-TestPoisonArea.patch

Comment 4

[Stefan O'Rear]

Attachment: 0005-fix-no-ion.patch

Comment 5

[Makoto Kato [:m_kato]]

sorear2, could you create patch via mercurial, not git?  repository is http://hg.mozilla.org/mozilla-central.

Comment 6

[Stefan O'Rear]

Attachment: 0001-update-config-guess.patch

Comment 7

[Stefan O'Rear]

Attachment: 0002-update-double-conversion.patch

Comment 8

[Stefan O'Rear]

Attachment: 0003-riscv-build-changes.patch

Comment 9

[Stefan O'Rear]

Attachment: 0004-riscv-TestPoisonArea.patch

Comment 10

[Stefan O'Rear]

Attachment: 0005-fix-no-ion.patch

Comment 11

[Stefan O'Rear]

(In reply to Makoto Kato [:m_kato] from comment #5)
> sorear2, could you create patch via mercurial, not git?  repository is
> http://hg.mozilla.org/mozilla-central.

I have transferred the changes into a mercurial repository (using "patch -p1") and recreated the patches using "hg diff -c".

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8812600 [details] [diff] [review]
0001-update-config-guess.patch

This patch is from contributor.  update config.guess for RISC-V support

Comment 13

[Makoto Kato [:m_kato]]

Comment on attachment 8812602 [details] [diff] [review]
0003-riscv-build-changes.patch

This patch is from contributor.

Comment 14

[Makoto Kato [:m_kato]]

Comment on attachment 8812603 [details] [diff] [review]
0004-riscv-TestPoisonArea.patch

This patch is from contributor to support RISC-V.

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8812604 [details] [diff] [review]
0005-fix-no-ion.patch

This is from contributor...

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8812604 [details] [diff] [review]
0005-fix-no-ion.patch

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

I've got a r+ patch for this in bug 1317033, I'm going to land it right now; thanks for the fix though!

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8812601 [details] [diff] [review]
0002-update-double-conversion.patch

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

This is fine, thanks.

For avoidance of doubt, as this is used not only by SpiderMonkey, but also by the wider Firefox build, this patch will need to be tested with all of Firefox's tests as well prior to landing.

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 8812603 [details] [diff] [review]
0004-riscv-TestPoisonArea.patch

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

::: mfbt/tests/TestPoisonArea.cpp
@@ +157,5 @@
>  #elif defined __aarch64__
>  #define RETURN_INSTR 0xd65f03c0 /* ret */
>  
> +#elif defined __riscv
> +#define RETURN_INSTR 0x8067 /* ret aka jalr zero, 0(ra) */

I see RETURN_INSTR is assigned into a RETURN_INSTR_TYPE*, and that macro appears not to be defined here -- thus defaulting to uint32_t.  But this constant is a 16-bit constant, no?  So isn't this going to fill the page not with this return instruction, but with this return instruction alternated with 16-bit zeroes?

Seems to me you want to #define RETURN_INSTR_TYPE uint16_t.  Or am I missing something here?

(It would be nice if you could point to a reference on RISC-V instruction formats, while you're at it -- right now I'm only assuming your comment/number are correct because of Linux/RISC-V not being a tier-1 platform.)

Comment 19

[Stefan O'Rear]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18)
> Comment on attachment 8812603 [details] [diff] [review]
> 0004-riscv-TestPoisonArea.patch
> 
> Review of attachment 8812603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/tests/TestPoisonArea.cpp
> @@ +157,5 @@
> >  #elif defined __aarch64__
> >  #define RETURN_INSTR 0xd65f03c0 /* ret */
> >  
> > +#elif defined __riscv
> > +#define RETURN_INSTR 0x8067 /* ret aka jalr zero, 0(ra) */
> 
> I see RETURN_INSTR is assigned into a RETURN_INSTR_TYPE*, and that macro
> appears not to be defined here -- thus defaulting to uint32_t.  But this
> constant is a 16-bit constant, no?  So isn't this going to fill the page not
> with this return instruction, but with this return instruction alternated
> with 16-bit zeroes?
> 
> Seems to me you want to #define RETURN_INSTR_TYPE uint16_t.  Or am I missing
> something here?

Actually it's a 32-bit instruction.  Would 0x00008067 be clearer?

> (It would be nice if you could point to a reference on RISC-V instruction
> formats, while you're at it -- right now I'm only assuming your
> comment/number are correct because of Linux/RISC-V not being a tier-1
> platform.)

https://content.riscv.org/wp-content/uploads/2016/06/riscv-spec-v2.1.pdf#page=64 has the JALR encoding

https://content.riscv.org/wp-content/uploads/2016/06/riscv-spec-v2.1.pdf#page=122 has the RET alias

Do these belong in the patch?

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8812600 [details] [diff] [review]
0001-update-config-guess.patch

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

While you're here, please also update config.sub.

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8812602 [details] [diff] [review]
0003-riscv-build-changes.patch

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

::: python/mozbuild/mozbuild/configure/constants.py
@@ +80,5 @@
>      ('s390x', '__s390x__'),
>      ('s390', '__s390__'),
>      ('ppc64', '__powerpc64__'),
>      ('ppc', '__powerpc__'),
> +    ('riscv64', '__riscv64'),

__riscv*__ are not defined? what's the output for `$CC -E -dM - < /dev/null | grep riscv` ?

Comment 22

[Stefan O'Rear]

(In reply to Mike Hommey [:glandium] from comment #21)
> __riscv*__ are not defined? what's the output for `$CC -E -dM - < /dev/null
> | grep riscv` ?

#define __riscv 1
#define __riscv_hard_float 1
#define __riscv_atomic 1
#define __RISCVEL 1
#define RISCVEL 1
#define __riscv_fdiv 1
#define _RISCV_SZPTR 64
#define _RISCV_SZINT 32
#define __riscv_muldiv 1
#define __riscv64 1
#define _RISCV_SZLONG 64
#define __riscv_fsqrt 1
#define __RISCVEL__ 1
#define _RISCV_SIM _ABI64
#define __riscv__ 1
#define _riscv 1

(We're still, but not much longer, in a position to change the macros, if there are important considerations.)

Comment 23

[Mike Hommey [:glandium]]

(In reply to Stefan O'Rear from comment #22)
> (We're still, but not much longer, in a position to change the macros, if
> there are important considerations.)

Is there a particular reason you don't have __$arch__ macros for riscv32 and riscv64? Most other platforms have them. And according to https://sourceforge.net/p/predef/wiki/Architectures/ the most recent addition I know of (aarch64) only has __$arch__, and no __$arch.

Comment 24

[Stefan O'Rear]

(In reply to Mike Hommey [:glandium] from comment #23)
> Is there a particular reason you don't have __$arch__ macros for riscv32 and
> riscv64? Most other platforms have them. And according to
> https://sourceforge.net/p/predef/wiki/Architectures/ the most recent
> addition I know of (aarch64) only has __$arch__, and no __$arch.

Counterpoint, having two macros that mean the same thing is probably a bad thing.  How about we get rid of __riscv__, _riscv, RISCVEL, __RISCVEL__ ?  Given that gcc has __BYTE_ORDER__ and __SIZEOF_LONG__ a few others seem redundant; having the same thing available in both architecture-specific and architecture-independent forms is also inviting future compatiblity problems.

Comment 25

[Mike Hommey [:glandium]]

(In reply to Stefan O'Rear from comment #24)
> (In reply to Mike Hommey [:glandium] from comment #23)
> > Is there a particular reason you don't have __$arch__ macros for riscv32 and
> > riscv64? Most other platforms have them. And according to
> > https://sourceforge.net/p/predef/wiki/Architectures/ the most recent
> > addition I know of (aarch64) only has __$arch__, and no __$arch.
> 
> Counterpoint, having two macros that mean the same thing is probably a bad
> thing.  How about we get rid of __riscv__, _riscv, RISCVEL, __RISCVEL__ ?

Or you could keep __riscv__ only and get rid of the others.
 
> Given that gcc has __BYTE_ORDER__ and __SIZEOF_LONG__

You mean __SIZEOF_POINTER__.

And then __riscv__ && __SIZEOF_POINTER__ == 4 is riscv32, and __riscv__ && __SIZEOF_POINTER__ == 8 is riscv64.

Comment 26

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #25)
> And then __riscv__ && __SIZEOF_POINTER__ == 4 is riscv32, and __riscv__ &&
> __SIZEOF_POINTER__ == 8 is riscv64.

This, BTW, would already work with the compiler you gave the output for in comment 22.

Comment 27

[Stefan O'Rear]

(In reply to Mike Hommey [:glandium] from comment #25)
> You mean __SIZEOF_POINTER__.

Delete _RISCV_SZPTR, because people should use __SIZEOF_POINTER__.

Delete _RISCV_SZLONG, because people should use __SIZEOF_LONG__.
 
> And then __riscv__ && __SIZEOF_POINTER__ == 4 is riscv32, and __riscv__ &&
> __SIZEOF_POINTER__ == 8 is riscv64.

__riscv64 actually tells the size of a register, which might not be the same as a long _or_ a pointer, because we're expecting to need to support something like x32 in the future.  So I should probably update this patch to look for __riscv && __SIZEOF_POINTER__ == 8 instead, thanks for bringing this up.

I don't see a compelling advantage to trailing underscores.  __RISCVEL, RISCVEL, __RISCVEL__, __riscv__, _riscv, _RISCV_SZLONG, _RISCV_SZINT, and _RISCV_SZPTR can all be removed.

_RISCV_SIM looks like something we inadvertently copied from the MIPS port.  I'll try to get that removed as well.

Comment 28

[Mike Hommey [:glandium]]

(In reply to Stefan O'Rear from comment #27)
> I don't see a compelling advantage to trailing underscores.

That's what we use for (most) platforms, so on that part, it would be consistent. But that's something on our side.
But as I mentioned in comment 23, the most recently added platform in GCC, according to that wiki link, *only* has the version with trailing underscores, so it would seem the form with a trailing underscore is preferred in GCC.

Comment 29

[John Paul Adrian Glaubitz]

Any chance we could move forward with this?

Shall I provide an updated patch?

Currently, we have to carry an extra patch in Debian for riscv64 and it would be nice if we could get rid of it.

The same applies to ia64 and m68k which could be added for building Spidermonkey only.

Comment 30

[John Paul Adrian Glaubitz]

I will pick this up once my two missing changes from https://bugzilla.mozilla.org/show_bug.cgi?id=1325771 have been picked up.

Comment 31

[John Paul Adrian Glaubitz]

Attachment: Bug 1318905 - build: Add riscv64 as target architecture to mozbuild r=glandium

Adds the basic definitions for riscv64 to mozbuild, allowing to build Spidermonkey.

Comment 32

[John Paul Adrian Glaubitz]

Attachment: Bug 1318905 - js:jit: Enable AtomicOperations-feeling-lucky.h on riscv64 r=lth

This allows the build on riscv64 to use the atomic operations provided by GCC.

Depends on D78623

Comment 33

[John Paul Adrian Glaubitz]

Attachment: Bug 1318905 - mfbt:tests: Define RETURN_INSTR for riscv64 in TestPoisonArea r=glandium

Define RETURN_INSTR for riscv64 in TestPoisonArea, i.e. the riscv64 assembly
opcodes for "ret ; ret".

Depends on D78624

Comment 34

[John Paul Adrian Glaubitz]

Comment on attachment 8812601 [details] [diff] [review]
0002-update-double-conversion.patch

No longer necessary.

Comment 35

[John Paul Adrian Glaubitz]

Comment on attachment 8812602 [details] [diff] [review]
0003-riscv-build-changes.patch

Superseded by change on Phabricator.

Comment 36

[John Paul Adrian Glaubitz]

Comment on attachment 8812603 [details] [diff] [review]
0004-riscv-TestPoisonArea.patch

Superseded by change on Phabricator.

Comment 37

[John Paul Adrian Glaubitz]

Dorel, any chance you could add this changeset for autoland? Thanks.

Comment 38

[Dorel Luca [:dluca]]

John, Should all the stack land? Meaning all 3 patches?

Comment 39

[John Paul Adrian Glaubitz]

Yes, we need all three patches to add build support for Spidermonkey for RISC-V. Thanks.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06d7e1b6b7e7
build: Add riscv64 as target architecture to mozbuild r=glandium
https://hg.mozilla.org/integration/autoland/rev/ec48f15d085c
js:jit: Enable AtomicOperations-feeling-lucky.h on riscv64 r=lth
https://hg.mozilla.org/integration/autoland/rev/6803dda74d33
mfbt:tests: Define RETURN_INSTR for riscv64 in TestPoisonArea r=glandium

Comment 41

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/06d7e1b6b7e7
https://hg.mozilla.org/mozilla-central/rev/ec48f15d085c
https://hg.mozilla.org/mozilla-central/rev/6803dda74d33