Last Comment Bug 771320 - Fix endian issues with handles
: Fix endian issues with handles
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 15 Branch
: PowerPC Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Cameron Kaiser [:spectre]
:
: Jason Orendorff [:jorendorff]
Mentors:
https://code.google.com/p/tenfourfox/...
Depends on:
Blocks: 731110 750733
  Show dependency treegraph
 
Reported: 2012-07-05 13:40 PDT by Cameron Kaiser [:spectre]
Modified: 2012-07-10 20:31 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use payloadOf/PAYLOAD_OFFSET when constructing PIC stubs (2.34 KB, patch)
2012-07-05 13:40 PDT, Cameron Kaiser [:spectre]
bhackett1024: review+
Details | Diff | Splinter Review
Use payloadOf/PAYLOAD_OFFSET when constructing PIC stubs (v2) (2.95 KB, patch)
2012-07-08 16:53 PDT, Cameron Kaiser [:spectre]
bhackett1024: review+
Details | Diff | Splinter Review

Description Cameron Kaiser [:spectre] 2012-07-05 13:40:08 PDT
Created attachment 639454 [details] [diff] [review]
Use payloadOf/PAYLOAD_OFFSET when constructing PIC stubs

TenFourFox: Bigger Endian is Better(tm)

Bug 750733 introduced handles widely as part of the improved GC, but the current implementation makes assumptions about the word order, causing crashes with things like argument objects on big-endian platforms. See also https://code.google.com/p/tenfourfox/issues/detail?id=165#c39 (note 39 and following). This small patch should work on both little- and big-endian, and passes all tests with the PowerPC backend.
Comment 1 David Mandelin [:dmandelin] 2012-07-06 18:43:44 PDT
Comment on attachment 639454 [details] [diff] [review]
Use payloadOf/PAYLOAD_OFFSET when constructing PIC stubs

I think Brian's a better reviewer for this one.
Comment 2 Cameron Kaiser [:spectre] 2012-07-08 12:54:29 PDT
Thanks, David and Brian!
Comment 3 Ryan VanderMeulen [:RyanVM] 2012-07-08 13:34:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dd36c1cd662

Bonus points next time if your patches include the needed metadata. It makes life easier for checkin monkeys like me :-)
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-07-08 13:53:14 PDT
http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound

Compiling is also a nice feature for a patch to have. Backed out due to red.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08fb8d6997bc

https://tbpl.mozilla.org/php/getParsedLog.php?id=13334873&tree=Mozilla-Inbound

PolyIC.cpp
/usr/bin/ccache /tools/gcc-4.5-0moz3/bin/g++ -o PolyIC.o -c  -I./../../dist/system_wrappers_js -include /builds/slave/m-in-lnx64/build/js/src/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DEXPORT_JS_API -DJS_HAS_CTYPES -DDLL_PREFIX=\"lib\" -DDLL_SUFFIX=\".so\" -DNO_NSPR_10_SUPPORT -Ictypes/libffi/include -I.  -I/builds/slave/m-in-lnx64/build/js/src/../../mfbt/double-conversion -I/builds/slave/m-in-lnx64/build/js/src -I. -I./../../dist/include  -I/builds/slave/m-in-lnx64/build/obj-firefox/dist/include/nspr      -I/builds/slave/m-in-lnx64/build/js/src -I/builds/slave/m-in-lnx64/build/js/src/assembler -I/builds/slave/m-in-lnx64/build/js/src/yarr  -fPIC  -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -pthread -pipe  -DNDEBUG -DTRIMMED -g -O3 -freorder-blocks -finline-limit=50 -fno-omit-frame-pointer  -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DENABLE_JIT=1   -DMOZILLA_CLIENT -include ./js-confdefs.h -MD -MF .deps/PolyIC.o.pp /builds/slave/m-in-lnx64/build/js/src/methodjit/PolyIC.cpp
../../../js/src/methodjit/PunboxAssembler.h: In member function 'void js::mjit::GetPropCompiler::generateGetterStub(js::mjit::Assembler&, js::Shape*, jsid, js::mjit::MacroAssemblerTypedefs::Label, js::Vector<JSC::AbstractMacroAssembler<JSC::X86Assembler>::Jump, 8ul>&)':
../../../js/src/methodjit/PunboxAssembler.h:52:44: error: 'const uint32_t js::mjit::PunboxAssembler::PAYLOAD_OFFSET' is private
../../../js/src/methodjit/PolyIC.cpp:1102:80: error: within this context
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-07-08 14:01:00 PDT
FWIW, all the build failures were 64bit.
Comment 6 Cameron Kaiser [:spectre] 2012-07-08 15:27:34 PDT
error: 'const uint32_t js::mjit::PunboxAssembler::PAYLOAD_OFFSET' is private? Should I make that public also?

Sorry about the metadata; I'll just generate checkin-ready versions separately.
Comment 7 Cameron Kaiser [:spectre] 2012-07-08 15:28:04 PDT
Actually, I take that back -- it seems easier just to make this 32-bit only. Would that be the right approach, Brian?
Comment 8 Brian Hackett (:bhackett) 2012-07-08 15:32:15 PDT
(In reply to Cameron Kaiser from comment #7)
> Actually, I take that back -- it seems easier just to make this 32-bit only.
> Would that be the right approach, Brian?

I think that making PAYLOAD_OFFSET public in PunboxAssembler.h should be fine (and seems simple?), it'd be nice to avoid any #ifdef JS_BLAH in the IC methods.
Comment 9 Cameron Kaiser [:spectre] 2012-07-08 16:53:53 PDT
Created attachment 640116 [details] [diff] [review]
Use payloadOf/PAYLOAD_OFFSET when constructing PIC stubs (v2)

Okay, patched PunboxAssembler.h also.

Unfortunately, I do not have access to push to try.
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-07-08 17:13:57 PDT
Still missing commit info :-P
https://tbpl.mozilla.org/?tree=Try&rev=689f9db78714
Comment 11 Brian Hackett (:bhackett) 2012-07-08 17:13:59 PDT
Comment on attachment 640116 [details] [diff] [review]
Use payloadOf/PAYLOAD_OFFSET when constructing PIC stubs (v2)

Pushed to try @ https://tbpl.mozilla.org/?tree=Try&rev=7520fdb6c037
Comment 12 Cameron Kaiser [:spectre] 2012-07-10 10:28:59 PDT
On bhackett's try, I see a single red mochitest for Android and I don't know if that has anything to do with this, plus some oranges under Linux 64bit. OS X64 and Win64 are green. I don't think this is the cause of the fail/oranges.
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-07-10 15:36:35 PDT
It builds, good enough for me :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3145f3e7f44
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-07-10 20:31:12 PDT
https://hg.mozilla.org/mozilla-central/rev/f3145f3e7f44

Note You need to log in before you can comment on or make changes to this bug.