Last Comment Bug 774760 - Fix casting of (double *) to aligned address in jsscript.cpp
: Fix casting of (double *) to aligned address in jsscript.cpp
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: Other Linux
: -- normal (vote)
: mozilla17
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-17 11:11 PDT by Chao-ying Fu
Modified: 2012-07-25 08:11 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A patch to align cursor to 8 bytes (1.55 KB, text/plain)
2012-07-17 11:11 PDT, Chao-ying Fu
no flags Details

Description Chao-ying Fu 2012-07-17 11:11:58 PDT
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 1 Matthias Versen [:Matti] 2012-07-17 17:14:33 PDT
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!)
Comment 2 Luke Wagner [:luke] 2012-07-17 17:45:42 PDT
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);
Comment 3 Chao-ying Fu 2012-07-17 17:53:58 PDT
(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 Luke Wagner [:luke] 2012-07-17 18:25:09 PDT
(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 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-07-17 20:06:10 PDT
(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.
Comment 6 Chao-ying Fu 2012-07-19 17:55:10 PDT
(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 Luke Wagner [:luke] 2012-07-20 03:13:45 PDT
That looks good, but how about #if JS_BITS_PER_WORD == 32 to cover any 32-bit arch?
Comment 8 Chao-ying Fu 2012-07-23 18:11:02 PDT
(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 Luke Wagner [:luke] 2012-07-24 11:18:13 PDT
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.
Comment 10 Chao-ying Fu 2012-07-24 11:25:43 PDT
(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 Luke Wagner [:luke] 2012-07-24 11:31:37 PDT
Thanks for reporting and following through :)
Comment 13 Ed Morley [:emorley] 2012-07-25 08:11:07 PDT
https://hg.mozilla.org/mozilla-central/rev/9ead721069f4

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