IonMonkey: MIPS: Fix build failure caused by bug 1194139

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: hev, Assigned: hev)

Tracking

Trunk
mozilla45
Other
Linux
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
mips64el-unknown-linux-gnu-g++ -o Unified_cpp_js_src18.o -c  -I../../dist/system_wrappers -include /home/heiher/git/mips64/gecko-dev/config/gcc_hidden.h -DENABLE_BINARYDATA -DENABLE_SHARED_AR
RAY_BUFFER -DEXPORT_JS_API -DAB_CD= -DNO_NSPR_10_SUPPORT -I/home/heiher/git/mips64/gecko-dev/js/src -I.  -I../../dist/include   -I/usr/include/nspr        -fPIC   -DMOZILLA_CLIENT -include ..
/../js/src/js-confdefs.h -MD -MP -MF .deps/Unified_cpp_js_src18.o.pp  -Wall -Wsign-compare -Wtype-limits -Wno-invalid-offsetof -Wcast-align -fno-rtti -fno-exceptions -fno-math-errno -std=gnu+
+0x -pthread -pipe  -DNDEBUG -DTRIMMED -g -O2 -march=loongson3a -mabi=32 -fdiagnostics-color=auto -mno-branch-likely -w -fomit-frame-pointer  -I/opt/mips64el-toolchain/platforms/current/usr/i
nclude   /home/heiher/git/mips64/gecko-dev/js/src/mips32-release/js/src/Unified_cpp_js_src18.cpp
In file included from /home/heiher/git/mips64/gecko-dev/js/src/jit/mips-shared/Architecture-mips-shared.cpp:7:0,
                 from /home/heiher/git/mips64/gecko-dev/js/src/mips32-release/js/src/Unified_cpp_js_src18.cpp:2:
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips-shared/Architecture-mips-shared.h:125:41: error: 'REGISTERS_ALLOCATABLE' was not declared in this scope
     static const uint32_t Allocatable = REGISTERS_ALLOCATABLE;
                                         ^
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips-shared/Architecture-mips-shared.h:130:39: error: 'REGISTERS_ARGREGMASK' was not declared in this scope
     static const SetType ArgRegMask = REGISTERS_ARGREGMASK;
                                       ^
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips-shared/Architecture-mips-shared.h:182:39: error: 'REGISTERS_JSCALLMASK' was not declared in this scope
     static const SetType JSCallMask = REGISTERS_JSCALLMASK;
                                       ^
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips-shared/Architecture-mips-shared.h:186:37: error: 'REGISTERS_CALLMASK' was not declared in this scope
     static const SetType CallMask = REGISTERS_CALLMASK;
                                     ^
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips-shared/Architecture-mips-shared.h: In static member function 'static const char* js::jit::Registers::GetName(js::jit::Registers::Code)':
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips-shared/Architecture-mips-shared.h:112:45: error: 'REGISTERS_NAMES' was not declared in this scope
         static const char * const Names[] = REGISTERS_NAMES;
                                             ^
In file included from /home/heiher/git/mips64/gecko-dev/js/src/jit/mips-shared/Assembler-mips-shared.cpp:7:0,
                 from /home/heiher/git/mips64/gecko-dev/js/src/mips32-release/js/src/Unified_cpp_js_src18.cpp:11:
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips-shared/Assembler-mips-shared.h: At global scope:
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips-shared/Assembler-mips-shared.h:90:58: error: 'CALL_TEMP_NON_ARG_REGS' was not declared in this scope
 static MOZ_CONSTEXPR_VAR Register CallTempNonArgRegs[] = CALL_TEMP_NON_ARG_REGS;
                                                          ^
/home/heiher/git/mips64/gecko-dev/js/src/jit/mips-shared/Assembler-mips-shared.h:1239:39: error: 'NUM_INT_ARG_REGS' was not declared in this scope
 static const uint32_t NumIntArgRegs = NUM_INT_ARG_REGS;
(Assignee)

Comment 1

3 years ago
Created attachment 8671720 [details] [diff] [review]
case1.patch
Attachment #8671720 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 2

3 years ago
Created attachment 8671721 [details] [diff] [review]
case2.patch
Attachment #8671721 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 3

3 years ago
Comment on attachment 8671721 [details] [diff] [review]
case2.patch

Sorry, this patch is invalid, it can't fix another errors.
Attachment #8671721 - Attachment is obsolete: true
Attachment #8671721 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8671720 [details] [diff] [review]
case1.patch

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

This would not pass the check-style phony target of the makefile.

I am not sure to understand, why do you need these extra headers in the shared implementation?

::: js/src/jit/mips-shared/Architecture-mips-shared.cpp
@@ -4,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -#include "jit/mips-shared/Architecture-mips-shared.h"

You need to keep these headers in the corresponding cpp files to satisfy the check-style target.
Attachment #8671720 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 5

3 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> Comment on attachment 8671720 [details] [diff] [review]
> case1.patch
> 
> Review of attachment 8671720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This would not pass the check-style phony target of the makefile.
> 
> I am not sure to understand, why do you need these extra headers in the
> shared implementation?
> 
> ::: js/src/jit/mips-shared/Architecture-mips-shared.cpp
> @@ -4,4 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > -#include "jit/mips-shared/Architecture-mips-shared.h"
> 
> You need to keep these headers in the corresponding cpp files to satisfy the
> check-style target.

I don't known what's wrong, I'm sure is good at one time, may be Architecuter-mips32.h/Assembler-mips32.h moved to another unified file after new files landed. or some new compiler arguments applied.

Two reasons:
1. Erros in comment 0.
2. We must tell the compiler class Assembler is subclass of AssemblerMIPSShared, because a static cast AssemblerMIPSShared* -> Assembler* in Assembler-mips-shared.cpp:

 113 Assembler&
 114 AssemblerMIPSShared::asAsm()
 115 {
 116     return *static_cast<Assembler*>(this);
 117 }

I have been run check style tests, and all passed:
[heiher@heiher-dev src]$ python2 ../../config/check_spidermonkey_style.py
TEST-PASS | check_spidermonkey_style.py | ok
[heiher@heiher-dev src]$ python2 ../../config/check_macroassembler_style.py
TEST-PASS | check_macroassembler_style.py | ok
Anyway, you should keep the previous header as the first header of the file, and if you need any other then you add them in addition to the previous one.

(In reply to Heiher [:hev] from comment #5)
> 2. We must tell the compiler class Assembler is subclass of
> AssemblerMIPSShared, because a static cast AssemblerMIPSShared* ->
> Assembler* in Assembler-mips-shared.cpp:
> 
>  113 Assembler&
>  114 AssemblerMIPSShared::asAsm()
>  115 {
>  116     return *static_cast<Assembler*>(this);
>  117 }
> 

Why do you need this function in the Assembler ?!  The Assembler is supposed to be incrementally build to encode instructions and nothing more.  Shouldn't your function be moved to the MacroAssembler then?

The only reason we have asMasm() today is to do a large amount of changing incrementaly and to get rid of it.  So I would appreciate if we do not add any other down-casting.
(Assignee)

Comment 7

3 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Anyway, you should keep the previous header as the first header of the file,
> and if you need any other then you add them in addition to the previous one.
> 
> (In reply to Heiher [:hev] from comment #5)
> > 2. We must tell the compiler class Assembler is subclass of
> > AssemblerMIPSShared, because a static cast AssemblerMIPSShared* ->
> > Assembler* in Assembler-mips-shared.cpp:
> > 
> >  113 Assembler&
> >  114 AssemblerMIPSShared::asAsm()
> >  115 {
> >  116     return *static_cast<Assembler*>(this);
> >  117 }
> > 
> 
> Why do you need this function in the Assembler ?!  The Assembler is supposed
> to be incrementally build to encode instructions and nothing more. 
> Shouldn't your function be moved to the MacroAssembler then?

Call functions defined in subclass Assembler. Should I declare as pure virtual function in parent class and override in subclass?
http://lxr.mozilla.org/mozilla-central/source/js/src/jit/mips-shared/Assembler-mips-shared.cpp#141
http://lxr.mozilla.org/mozilla-central/source/js/src/jit/mips32/Assembler-mips32.h#132

> 
> The only reason we have asMasm() today is to do a large amount of changing
> incrementaly and to get rid of it.  So I would appreciate if we do not add
> any other down-casting.

If removed asAsm(), Do you think case 2(obsoleted) is ok?
(Assignee)

Updated

3 years ago
Attachment #8671721 - Attachment is obsolete: false
Attachment #8671721 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8671720 [details] [diff] [review]
case1.patch

Or Can you r+ the case 1 if check style passed?
Attachment #8671720 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8671720 [details] [diff] [review]
case1.patch

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

Keep the original header.
Attachment #8671720 - Flags: review?(nicolas.b.pierron) → review-
Comment on attachment 8671721 [details] [diff] [review]
case2.patch

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

Is there any reasons to have these macro here instead of defining the variables which are below with the values associated with these macros?
Attachment #8671721 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 11

3 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> Comment on attachment 8671721 [details] [diff] [review]
> case2.patch
> 
> Review of attachment 8671721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there any reasons to have these macro here instead of defining the
> variables which are below with the values associated with these macros?

I think we should as less as possible use conditional macros from the previous conversation. If not, many #ifdef JS_CODEGEN_MIPSXX might be inserted. Do you think ok?
(Assignee)

Updated

3 years ago
Attachment #8671720 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8671721 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Summary: IonMonkey: MIPS: Fix build failure caused by macros not declared → IonMonkey: MIPS: Fix build failure caused by macros bug 1194139
(Assignee)

Comment 12

3 years ago
Created attachment 8679254 [details] [diff] [review]
Part 2: Remove AssemblerMIPSShared::asAsm.
Attachment #8679254 - Flags: review?(arai.unmht)
(Assignee)

Updated

3 years ago
Summary: IonMonkey: MIPS: Fix build failure caused by macros bug 1194139 → IonMonkey: MIPS: Fix build failure caused by bug 1194139
(Assignee)

Comment 13

3 years ago
Created attachment 8679255 [details] [diff] [review]
Part 3: Move Assembler::PatchDataWithValueCheck to architecture specific.
Attachment #8679255 - Flags: review?(lhansen)
(Assignee)

Comment 14

3 years ago
Created attachment 8679271 [details] [diff] [review]
Part 1: Fix build failure caused by bug 1194139.
Attachment #8679271 - Flags: review?(nicolas.b.pierron)
Attachment #8679271 - Flags: review?(lhansen)

Updated

3 years ago
Attachment #8679254 - Flags: review?(arai.unmht) → review+
Comment on attachment 8679271 [details] [diff] [review]
Part 1: Fix build failure caused by bug 1194139.

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

Thanks :)
Attachment #8679271 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8679271 [details] [diff] [review]
Part 1: Fix build failure caused by bug 1194139.

nbp reviewed.
Attachment #8679271 - Flags: review?(lhansen)
Comment on attachment 8679255 [details] [diff] [review]
Part 3: Move Assembler::PatchDataWithValueCheck to architecture specific.

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

Sorry about the delay.
Attachment #8679255 - Flags: review?(lhansen) → review+
(Assignee)

Comment 19

3 years ago
I'm so bad :( I forgot modify the last two arguments of AssemblerMIPSShared::bind to generic.
Keywords: leave-open
(Assignee)

Comment 20

3 years ago
Created attachment 8681620 [details] [diff] [review]
Part 4: Modify last 2 args of Assembler::bind to generic type.
Attachment #8681620 - Flags: review?(arai.unmht)
Comment on attachment 8681620 [details] [diff] [review]
Part 4: Modify last 2 args of Assembler::bind to generic type.

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

thanks!
I should've noticed that
Attachment #8681620 - Flags: review?(arai.unmht) → review+
(Assignee)

Updated

3 years ago
Keywords: leave-open

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7459205ed7c3
https://hg.mozilla.org/mozilla-central/rev/ffaccc986c34
https://hg.mozilla.org/mozilla-central/rev/75b33ded5271
https://hg.mozilla.org/mozilla-central/rev/076d3a7e781a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.