Fix endian issues with handles

RESOLVED FIXED in mozilla16



7 years ago
7 years ago


(Reporter: spectre, Assigned: spectre)


(Blocks 1 bug)

15 Branch
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [js:t], URL)


(1 attachment, 1 obsolete attachment)



7 years ago
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 (note 39 and following). This small patch should work on both little- and big-endian, and passes all tests with the PowerPC backend.


7 years ago
Attachment #639454 - Attachment is patch: true
Attachment #639454 - Flags: review?(dmandelin)
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.
Attachment #639454 - Flags: review?(dmandelin) → review?(bhackett1024)
Attachment #639454 - Flags: review?(bhackett1024) → review+

Comment 2

7 years ago
Thanks, David and Brian!
Assignee: general → spectre
Keywords: checkin-needed
QA Contact: spectre


7 years ago
QA Contact: spectre

Bonus points next time if your patches include the needed metadata. It makes life easier for checkin monkeys like me :-)
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla16

Compiling is also a nice feature for a patch to have. Backed out due to red.

/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
Target Milestone: mozilla16 → ---
FWIW, all the build failures were 64bit.

Comment 6

7 years ago
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

7 years ago
Actually, I take that back -- it seems easier just to make this 32-bit only. Would that be the right approach, Brian?
(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

7 years ago
Okay, patched PunboxAssembler.h also.

Unfortunately, I do not have access to push to try.
Attachment #639454 - Attachment is obsolete: true
Attachment #640116 - Flags: review?(bhackett1024)
Comment on attachment 640116 [details] [diff] [review]
Use payloadOf/PAYLOAD_OFFSET when constructing PIC stubs (v2)

Pushed to try @
Attachment #640116 - Flags: review?(bhackett1024) → review+
Whiteboard: [js:t]

Comment 12

7 years ago
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.
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.