1213146 - IonMonkey: MIPS: Fix build failure caused by bug 1194139

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

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;

Comment 1

[hev [:hev]]

Attachment: case1.patch

Comment 2

[hev [:hev]]

Attachment: case2.patch

Comment 3

[hev [:hev]]

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

Sorry, this patch is invalid, it can't fix another errors.

Comment 4

[Nicolas B. Pierron [:nbp]]

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.

Comment 5

[hev [:hev]]

(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

Comment 6

[Nicolas B. Pierron [:nbp]]

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.

Comment 7

[hev [:hev]]

(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?

Comment 8

[hev [:hev]]

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

Or Can you r+ the case 1 if check style passed?

Comment 9

[Nicolas B. Pierron [:nbp]]

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

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

Keep the original header.

Comment 10

[Nicolas B. Pierron [:nbp]]

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?

Comment 11

[hev [:hev]]

(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?

Comment 12

[hev [:hev]]

Attachment: Part 2: Remove AssemblerMIPSShared::asAsm.

Comment 13

[hev [:hev]]

Attachment: Part 3: Move Assembler::PatchDataWithValueCheck to architecture specific.

Comment 14

[hev [:hev]]

Attachment: Part 1: Fix build failure caused by bug 1194139.

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8679271 [details] [diff] [review]
Part 1: Fix build failure caused by bug 1194139.

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

Thanks :)

Comment 16

[Lars T Hansen [:lth]]

Comment on attachment 8679271 [details] [diff] [review]
Part 1: Fix build failure caused by bug 1194139.

nbp reviewed.

Comment 17

[Lars T Hansen [:lth]]

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.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7459205ed7c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffaccc986c34
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b33ded5271

Comment 19

[hev [:hev]]

I'm so bad :( I forgot modify the last two arguments of AssemblerMIPSShared::bind to generic.

Comment 20

[hev [:hev]]

Attachment: Part 4: Modify last 2 args of Assembler::bind to generic type.

Comment 21

[Tooru Fujisawa [:arai]]

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

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/076d3a7e781a

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7459205ed7c3
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ffaccc986c34
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/75b33ded5271
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/076d3a7e781a

Comment 25

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion