774760 - Fix casting of (double *) to aligned address in jsscript.cpp

Status: RESOLVED FIXED

Description

[Chao-ying Fu]

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]]

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]]

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]

(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]]

(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]]

(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]

(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]]

That looks good, but how about #if JS_BITS_PER_WORD == 32 to cover any 32-bit arch?

Comment 8

[Chao-ying Fu]

(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]]

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]

(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]]

Thanks for reporting and following through :)

Comment 12

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ead721069f4

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9ead721069f4