Closed
Bug 1392606
Opened 7 years ago
Closed 7 years ago
Fix MIPS64 simulator build compilation failures
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: miran.karic, Assigned: miran.karic)
Details
Attachments
(1 file, 2 obsolete files)
8.29 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.104 Safari/537.36 Steps to reproduce: MIPS32 simulator build has been fixed in Bug 1329650, here is a patch that fixes MIPS64. It contains the changes for MIPS64 from patches uploaded in the mentioned bug and new fixes.
Assignee | ||
Updated•7 years ago
|
Attachment #8899821 -
Flags: review?(bbouvier)
Comment 1•7 years ago
|
||
Comment on attachment 8899821 [details] [diff] [review] mips64.patch Review of attachment 8899821 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, looks good to me. Only one question below and I'd like feedback from somebody else. Nicolas, can you take a look at Architecture-mips64.h please? ::: js/src/jit/mips-shared/CodeGenerator-mips-shared.h @@ +282,5 @@ > OutOfLineBailout(LSnapshot* snapshot, uint32_t frameSize) > : snapshot_(snapshot), > frameSize_(frameSize) > + { > + (void)frameSize_; Why is this needed? ::: js/src/jit/mips64/Simulator-mips64.cpp @@ +1620,5 @@ > + > +void > +Simulator::handleWasmInterrupt() > +{ > + MOZ_CRASH("NIY"); Out of curiosity, do you intend to fix those?
Attachment #8899821 -
Flags: review?(bbouvier) → feedback?(nicolas.b.pierron)
Comment 2•7 years ago
|
||
Comment on attachment 8899821 [details] [diff] [review] mips64.patch Review of attachment 8899821 [details] [diff] [review]: ----------------------------------------------------------------- Architecture-mips64.h looks good. If I recall correctly MIPS64 has float registers which are like x64 xmm registers for (single & double), and not like arm32 vector registers.
Attachment #8899821 -
Flags: feedback?(nicolas.b.pierron) → feedback+
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #1) > ::: js/src/jit/mips-shared/CodeGenerator-mips-shared.h > @@ +282,5 @@ > > OutOfLineBailout(LSnapshot* snapshot, uint32_t frameSize) > > : snapshot_(snapshot), > > frameSize_(frameSize) > > + { > > + (void)frameSize_; > > Why is this needed? This is what I have taken from a patch by Lars T Hansen in bug 1329650. Not sure if it is needed, my guess is that it silenced some warning. Without this change the simulator build works and I also managed to build for a mips64 board. If you want I can remove it. > ::: js/src/jit/mips64/Simulator-mips64.cpp > @@ +1620,5 @@ > > + > > +void > > +Simulator::handleWasmInterrupt() > > +{ > > + MOZ_CRASH("NIY"); > > Out of curiosity, do you intend to fix those? Yes, but for the moment I will focus on fixing mips32.
Comment 4•7 years ago
|
||
(In reply to Miran Karic from comment #3) > (In reply to Benjamin Bouvier [:bbouvier] from comment #1) > > ::: js/src/jit/mips-shared/CodeGenerator-mips-shared.h > > @@ +282,5 @@ > > > OutOfLineBailout(LSnapshot* snapshot, uint32_t frameSize) > > > : snapshot_(snapshot), > > > frameSize_(frameSize) > > > + { > > > + (void)frameSize_; > > > > Why is this needed? > > This is what I have taken from a patch by Lars T Hansen in bug 1329650. Not > sure if it is needed, my guess is that it silenced some warning. Without > this change the simulator build works and I also managed to build for a > mips64 board. If you want I can remove it. Yes please. > > > ::: js/src/jit/mips64/Simulator-mips64.cpp > > @@ +1620,5 @@ > > > + > > > +void > > > +Simulator::handleWasmInterrupt() > > > +{ > > > + MOZ_CRASH("NIY"); > > > > Out of curiosity, do you intend to fix those? > > Yes, but for the moment I will focus on fixing mips32. Cool, you might be interested by the work being carried out in bug 1391248 then.
Assignee | ||
Comment 5•7 years ago
|
||
Here is a fixed version of the patch. Yes, I am aware of that work, Dragan and I work for the same company.
Attachment #8899821 -
Attachment is obsolete: true
Attachment #8900344 -
Flags: review?(bbouvier)
Comment 6•7 years ago
|
||
Comment on attachment 8900344 [details] [diff] [review] mips64-2.patch Review of attachment 8900344 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks. Can you format your patch so that it is a proper mercurial changeset, with a valid commit message like: Bug 1392606: Fix MIPS64 simulator build compilation failures; r=bbouvier (more instructions there: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F ) Once it's done, can you upload it again and tag me for review again, please?
Attachment #8900344 -
Flags: review?(bbouvier) → feedback+
Assignee | ||
Comment 7•7 years ago
|
||
Of course, here is it. Thank you!
Attachment #8900344 -
Attachment is obsolete: true
Attachment #8900672 -
Flags: review?(bbouvier)
Comment 8•7 years ago
|
||
Comment on attachment 8900672 [details] [diff] [review] MIPS64 build fix patch in the correct format Review of attachment 8900672 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! I'll set the checkin-needed keyword so it's pushed by one of our sheriffs in CI (inbound). Thanks!
Attachment #8900672 -
Flags: review?(bbouvier) → review+
Updated•7 years ago
|
Assignee: nobody → miran.karic
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/34a8455c373c Fix MIPS64 simulator build compilation failures. r=bbouvier
Keywords: checkin-needed
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/34a8455c373c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•