Closed
Bug 1213146
Opened 9 years ago
Closed 9 years ago
IonMonkey: MIPS: Fix build failure caused by bug 1194139
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: hev, Assigned: hev)
Details
Attachments
(4 files, 2 obsolete files)
4.63 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
8.86 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
17.52 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
5.71 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Attachment #8671720 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8671721 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 3•9 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 4•9 years ago
|
||
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•9 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
Comment 6•9 years ago
|
||
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•9 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•9 years ago
|
Attachment #8671721 -
Attachment is obsolete: false
Attachment #8671721 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 8•9 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 9•9 years ago
|
||
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 10•9 years ago
|
||
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•9 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•9 years ago
|
Attachment #8671720 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8671721 -
Attachment is obsolete: true
Assignee | ||
Updated•9 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•9 years ago
|
||
Attachment #8679254 -
Flags: review?(arai.unmht)
Assignee | ||
Updated•9 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•9 years ago
|
||
Attachment #8679255 -
Flags: review?(lhansen)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8679271 -
Flags: review?(nicolas.b.pierron)
Attachment #8679271 -
Flags: review?(lhansen)
Updated•9 years ago
|
Attachment #8679254 -
Flags: review?(arai.unmht) → review+
Comment 15•9 years ago
|
||
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 16•9 years ago
|
||
Comment on attachment 8679271 [details] [diff] [review] Part 1: Fix build failure caused by bug 1194139. nbp reviewed.
Attachment #8679271 -
Flags: review?(lhansen)
Comment 17•9 years ago
|
||
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+
Comment 18•9 years ago
|
||
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
Assignee | ||
Comment 19•9 years ago
|
||
I'm so bad :( I forgot modify the last two arguments of AssemblerMIPSShared::bind to generic.
Keywords: leave-open
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8681620 -
Flags: review?(arai.unmht)
Comment 21•9 years ago
|
||
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•9 years ago
|
Keywords: leave-open
Comment 23•9 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
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 24•9 years ago
|
||
bugherder uplift |
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
status-b2g-v2.5:
--- → fixed
Comment 25•9 years ago
|
||
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.
Description
•