Closed Bug 1213146 Opened 4 years ago Closed 4 years ago

IonMonkey: MIPS: Fix build failure caused by bug 1194139

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: hev, Assigned: hev)

Details

Attachments

(4 files, 2 obsolete files)

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;
Attached patch case1.patch (obsolete) — Splinter Review
Attachment #8671720 - Flags: review?(nicolas.b.pierron)
Attached patch case2.patch (obsolete) — Splinter Review
Attachment #8671721 - Flags: review?(nicolas.b.pierron)
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)
(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.
(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?
Attachment #8671721 - Attachment is obsolete: false
Attachment #8671721 - Flags: review?(nicolas.b.pierron)
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)
(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?
Attachment #8671720 - Attachment is obsolete: true
Attachment #8671721 - Attachment is obsolete: true
Summary: IonMonkey: MIPS: Fix build failure caused by macros not declared → IonMonkey: MIPS: Fix build failure caused by macros bug 1194139
Attachment #8679254 - Flags: review?(arai.unmht)
Summary: IonMonkey: MIPS: Fix build failure caused by macros bug 1194139 → IonMonkey: MIPS: Fix build failure caused by bug 1194139
Attachment #8679271 - Flags: review?(nicolas.b.pierron)
Attachment #8679271 - Flags: review?(lhansen)
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+
I'm so bad :( I forgot modify the last two arguments of AssemblerMIPSShared::bind to generic.
Keywords: leave-open
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+
Keywords: leave-open
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.