Changes to make SpiderMonkey build on Linux/RISC-V
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox79 fixed)
Tracking | Status | |
---|---|---|
firefox79 | --- | fixed |
People
(Reporter: sorear2, Assigned: glaubitz)
Details
Attachments
(3 files, 10 obsolete files)
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.
Reporter | ||
Comment 1•8 years ago
|
||
Reporter | ||
Comment 2•8 years ago
|
||
Reporter | ||
Comment 3•8 years ago
|
||
Reporter | ||
Comment 4•8 years ago
|
||
Comment 5•8 years ago
|
||
sorear2, could you create patch via mercurial, not git? repository is http://hg.mozilla.org/mozilla-central.
Reporter | ||
Comment 6•8 years ago
|
||
Reporter | ||
Comment 7•8 years ago
|
||
Reporter | ||
Comment 8•8 years ago
|
||
Reporter | ||
Comment 9•8 years ago
|
||
Reporter | ||
Comment 10•8 years ago
|
||
Reporter | ||
Comment 11•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
Comment on attachment 8812602 [details] [diff] [review] 0003-riscv-build-changes.patch This patch is from contributor.
Updated•8 years ago
|
Comment 14•8 years ago
|
||
Comment on attachment 8812603 [details] [diff] [review] 0004-riscv-TestPoisonArea.patch This patch is from contributor to support RISC-V.
Comment 15•8 years ago
|
||
Comment on attachment 8812604 [details] [diff] [review] 0005-fix-no-ion.patch This is from contributor...
Comment 16•8 years ago
|
||
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!
Updated•8 years ago
|
![]() |
||
Comment 17•8 years ago
|
||
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•8 years ago
|
||
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.)
Reporter | ||
Comment 19•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
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` ?
Reporter | ||
Comment 22•8 years ago
|
||
(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•8 years ago
|
||
(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.
Reporter | ||
Comment 24•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
(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.
Reporter | ||
Comment 27•8 years ago
|
||
(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•8 years ago
|
||
(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.
Updated•7 years ago
|
Assignee | ||
Comment 29•4 years ago
|
||
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.
Assignee | ||
Comment 30•4 years ago
|
||
I will pick this up once my two missing changes from https://bugzilla.mozilla.org/show_bug.cgi?id=1325771 have been picked up.
Assignee | ||
Comment 31•4 years ago
|
||
Adds the basic definitions for riscv64 to mozbuild, allowing to build Spidermonkey.
Updated•4 years ago
|
Assignee | ||
Comment 32•4 years ago
|
||
This allows the build on riscv64 to use the atomic operations provided by GCC.
Depends on D78623
Assignee | ||
Comment 33•4 years ago
|
||
Define RETURN_INSTR for riscv64 in TestPoisonArea, i.e. the riscv64 assembly
opcodes for "ret ; ret".
Depends on D78624
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 34•4 years ago
|
||
Comment on attachment 8812601 [details] [diff] [review] 0002-update-double-conversion.patch No longer necessary.
Assignee | ||
Comment 35•4 years ago
|
||
Comment on attachment 8812602 [details] [diff] [review] 0003-riscv-build-changes.patch Superseded by change on Phabricator.
Assignee | ||
Comment 36•4 years ago
|
||
Comment on attachment 8812603 [details] [diff] [review] 0004-riscv-TestPoisonArea.patch Superseded by change on Phabricator.
Assignee | ||
Comment 37•4 years ago
|
||
Dorel, any chance you could add this changeset for autoland? Thanks.
Comment 38•4 years ago
|
||
John, Should all the stack land? Meaning all 3 patches?
Assignee | ||
Comment 39•4 years ago
|
||
Yes, we need all three patches to add build support for Spidermonkey for RISC-V. Thanks.
Assignee | ||
Updated•4 years ago
|
Comment 40•4 years ago
|
||
Pushed by dluca@mozilla.com: 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
Updated•4 years ago
|
Comment 41•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06d7e1b6b7e7
https://hg.mozilla.org/mozilla-central/rev/ec48f15d085c
https://hg.mozilla.org/mozilla-central/rev/6803dda74d33
Description
•