Closed
Bug 774760
Opened 13 years ago
Closed 13 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•13 years ago
|
OS: Windows XP → Linux
Hardware: x86 → Other
Updated•13 years ago
|
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: Untriaged → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
Comment 1•13 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•13 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•13 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•13 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•13 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•13 years ago
|
Whiteboard: [js:t]
Reporter | ||
Comment 6•13 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•13 years ago
|
||
That looks good, but how about #if JS_BITS_PER_WORD == 32 to cover any 32-bit arch?
Reporter | ||
Comment 8•13 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•13 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•13 years ago
|
Attachment #643043 -
Attachment is obsolete: true
Attachment #643043 -
Flags: review?(luke)
Reporter | ||
Comment 10•13 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•13 years ago
|
||
Thanks for reporting and following through :)
![]() |
||
Comment 12•13 years ago
|
||
Target Milestone: --- → mozilla17
Comment 13•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•