Created attachment 643043 [details] A patch to align cursor to 8 bytes 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.
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!)
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);
(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!
(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 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.
(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
That looks good, but how about #if JS_BITS_PER_WORD == 32 to cover any 32-bit arch?
(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
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.
(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!
Thanks for reporting and following through :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ead721069f4
https://hg.mozilla.org/mozilla-central/rev/9ead721069f4
or
