Closed
Bug 774760
Opened 12 years ago
Closed 12 years ago
Fix casting of (double *) to aligned address in jsscript.cpp
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: fu, Unassigned)
Details
(Whiteboard: [js:t])
Attachments
(1 obsolete file)
User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/536.11 (KHTML, like Gecko) Chrome/20.0.1132.57 Safari/536.11 Steps to reproduce: I compiled js on a MIPS Linux system and ran jit-test. Actual results: Two tests failed because the address of MIPS "ldc1" (load double) instructions is not aligned to 8 bytes. Ex: FAILURES: -m -a -D -m /disk/fu/dev/mercurial/mozilla-central/js/src/jit-test/tests/jae ger/bug768313.js -m -a -D -m -n /disk/fu/dev/mercurial/mozilla-central/js/src/jit-test/tests/ jaeger/bug768313.js Expected results: The pointer of "cursor" in "js/src/jsscript.cpp" should not be casted to (double *) unless the pointer is aligned to 8 bytes for a MIPS system. I fixed the alignment of "cursor" when compiling for MIPS.
Reporter | ||
Updated•12 years ago
|
OS: Windows XP → Linux
Hardware: x86 → Other
Updated•12 years ago
|
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: Untriaged → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
Comment 1•12 years ago
|
||
Comment on attachment 643043 [details]
A patch to align cursor to 8 bytes
You have to request review from someone or your patch can be lost in bugzilla.
I'm trying Luke (hg log 4tw!)
Attachment #643043 -
Flags: review?(luke)
Comment 2•12 years ago
|
||
Hmm, calloc(n) is supposed to return an n-byte-aligned address, so that would imply that 'bytes' is less than 8. Does that match what you are seeing Chao-ying? If so, then I think the fix is to just round up the allocation. Incidentally, we already do this in a different place: see AllocScriptData. Perhaps you can hoist AllocScriptData above JSScript::initScriptCounts and call it directly? To be really safe, we should also add: JS_STATIC_ASSERT(sizeof(PCCounts) % sizeof(double) == 0);
Reporter | ||
Comment 3•12 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #2) > Hmm, calloc(n) is supposed to return an n-byte-aligned address, so that > would imply that 'bytes' is less than 8. Does that match what you are > seeing Chao-ying? I need to check what value is returned from cx->calloc_(n). I am not sure about how cx->calloc_(n) can align to a certain number of bytes. > > If so, then I think the fix is to just round up the allocation. > Incidentally, we already do this in a different place: see AllocScriptData. > Perhaps you can hoist AllocScriptData above JSScript::initScriptCounts and > call it directly? > > To be really safe, we should also add: > > JS_STATIC_ASSERT(sizeof(PCCounts) % sizeof(double) == 0); From MIPS build, sizeof(PCCounts) is 4. After "cursor += length * sizeof(PCCounts);", cursor is not aligned to 8 bytes for the MIPS failed tests. Thanks!
Comment 4•12 years ago
|
||
(In reply to Chao-ying Fu from comment #3) > From MIPS build, sizeof(PCCounts) is 4. After "cursor += length * > sizeof(PCCounts);", cursor is not aligned to 8 bytes for the MIPS failed > tests. Thanks! Ah, so that is the real problem, not that the return value of calloc() is unaligned. I think a better fix (which should also help x86-32) would be to inflate PCCounts up to 8 bytes when sizeof(void*) == 4 and add the static assert I mentioned.
Comment 5•12 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #2) > Hmm, calloc(n) is supposed to return an n-byte-aligned address Nope! It is supposed to be properly aligned for objects of any type, e.g.: http://stackoverflow.com/questions/8752546/how-does-malloc-understand-alignment That alignment is usually 8 bytes, though some implementations give you 16. Having said that, I think jemalloc breaks this rule and only provides N-byte alignment when you request N bytes, and N is 2 or 4.
Updated•12 years ago
|
Whiteboard: [js:t]
Reporter | ||
Comment 6•12 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #4) > (In reply to Chao-ying Fu from comment #3) > > From MIPS build, sizeof(PCCounts) is 4. After "cursor += length * > > sizeof(PCCounts);", cursor is not aligned to 8 bytes for the MIPS failed > > tests. Thanks! > > Ah, so that is the real problem, not that the return value of calloc() is > unaligned. I think a better fix (which should also help x86-32) would be to > inflate PCCounts up to 8 bytes when sizeof(void*) == 4 and add the static > assert I mentioned. In DEBUG mode, the alignment is correct due to the extra variable of "capacity". In non-DEBUG mode, I add a new variable to increase sizeof(PCCounts) to 8 bytes. Does the patch look ok? Thanks a lot! Ex: diff -r 6865bdd5ca5a js/src/jsopcode.h --- a/js/src/jsopcode.h Wed Jul 18 22:46:11 2012 -0400 +++ b/js/src/jsopcode.h Thu Jul 19 17:29:53 2012 -0700 @@ -497,6 +497,10 @@ double *counts; #ifdef DEBUG size_t capacity; +#else +#if defined(JS_CPU_MIPS) || defined(JS_CPU_X86) + double *dummy; /* Let sizeof(PCCounts) be 8 bytes */ +#endif #endif public: diff -r 6865bdd5ca5a js/src/jsscript.cpp --- a/js/src/jsscript.cpp Wed Jul 18 22:46:11 2012 -0400 +++ b/js/src/jsscript.cpp Thu Jul 19 17:29:53 2012 -0700 @@ -817,6 +817,10 @@ cursor += length * sizeof(PCCounts); for (pc = code; pc < code + length; pc = next) { +#if defined(JS_CPU_MIPS) || defined(JS_CPU_X86) + /* Check the alignemnt of cursor. */ + JS_ASSERT(((size_t)cursor & (sizeof(double) - 1)) == 0); +#endif scriptCounts.pcCountsVector[pc - code].counts = (double *) cursor; size_t capacity = PCCounts::numCounts(JSOp(*pc)); #ifdef DEBUG
Comment 7•12 years ago
|
||
That looks good, but how about #if JS_BITS_PER_WORD == 32 to cover any 32-bit arch?
Reporter | ||
Comment 8•12 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #7) > That looks good, but how about #if JS_BITS_PER_WORD == 32 to cover any > 32-bit arch? Sure. Checking JS_BITS_PER_WORD is better. Thanks! Ex: diff -r 82b6c5885345 js/src/jsopcode.h --- a/js/src/jsopcode.h Sun Jul 22 22:01:57 2012 -0400 +++ b/js/src/jsopcode.h Mon Jul 23 18:08:56 2012 -0700 @@ -497,6 +497,10 @@ double *counts; #ifdef DEBUG size_t capacity; +#else +#if JS_BITS_PER_WORD == 32 + double *dummy; /* Let sizeof(PCCounts) be 8 bytes */ +#endif #endif public: diff -r 82b6c5885345 js/src/jsscript.cpp --- a/js/src/jsscript.cpp Sun Jul 22 22:01:57 2012 -0400 +++ b/js/src/jsscript.cpp Mon Jul 23 18:08:56 2012 -0700 @@ -848,6 +848,8 @@ cursor += length * sizeof(PCCounts); for (pc = code; pc < code + length; pc = next) { + /* Check the alignemnt of cursor. */ + JS_ASSERT(((size_t)cursor & (sizeof(double) - 1)) == 0); scriptCounts.pcCountsVector[pc - code].counts = (double *) cursor; size_t capacity = PCCounts::numCounts(JSOp(*pc)); #ifdef DEBUG
Comment 9•12 years ago
|
||
Pushing to try to make sure the static assert passes on all platforms: https://hg.mozilla.org/try/rev/ac9c1d9cb216 Chao-ying: does this commit message (particularly, the author field) look good to you? If so, I'll push to inbound after the try results come back.
Updated•12 years ago
|
Attachment #643043 -
Attachment is obsolete: true
Attachment #643043 -
Flags: review?(luke)
Reporter | ||
Comment 10•12 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #9) > Pushing to try to make sure the static assert passes on all platforms: > https://hg.mozilla.org/try/rev/ac9c1d9cb216 > > Chao-ying: does this commit message (particularly, the author field) look > good to you? If so, I'll push to inbound after the try results come back. It's great. Thanks a lot for your help and your patch!
Comment 11•12 years ago
|
||
Thanks for reporting and following through :)
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ead721069f4
Target Milestone: --- → mozilla17
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9ead721069f4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•