Bug 131495 – LLINT loadisFromInstruction should handle the big endian case

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

The LLINT loadisFromInstruction macro aims to load the least significant 32-bit word from the 64-bit bytecode instruction stream and sign extend it.  For big endian machines, the current implementation would load the wrong 32-bit word.

[Mark Lam]

Created attachment 229055 [details]
The patch.

[Mark Lam]

Created attachment 229056 [details]
patch 2: updated comment.

[Mark Hahnenberg]

Comment on attachment 229056 [details]
patch 2: updated comment.

Does this fix all big endian issues? It seems to me like we do lots of loadi instructions assuming little endianness.

[Mark Lam]

(In reply to comment #3)
> (From update of attachment 229056 [details])
> Does this fix all big endian issues? It seems to me like we do lots of loadi instructions assuming little endianness.

I would not presume that.  Tomas has been analyzing big endian failures in https://bugs.webkit.org/show_bug.cgi?id=12874.  I’m just knocking them down one at a time when he reports them.  This one happens to be the next one.  It may or may not be the last.

[Mark Hahnenberg]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 229056 [details] [details])
> > Does this fix all big endian issues? It seems to me like we do lots of loadi instructions assuming little endianness.
> 
> I would not presume that.  Tomas has been analyzing big endian failures in https://bugs.webkit.org/show_bug.cgi?id=12874.  I’m just knocking them down one at a time when he reports them.  This one happens to be the next one.  It may or may not be the last.

Hmm. Assuming this isn't the last one, is there a better way to handle these bugs rather than slapping if BIG_ENDIAN everywhere? Could this be handled in the backend somehow?

[Mark Hahnenberg]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 229056 [details] [details] [details])
> > > Does this fix all big endian issues? It seems to me like we do lots of loadi instructions assuming little endianness.
> > 
> > I would not presume that.  Tomas has been analyzing big endian failures in https://bugs.webkit.org/show_bug.cgi?id=12874.  I’m just knocking them down one at a time when he reports them.  This one happens to be the next one.  It may or may not be the last.
> 
> Hmm. Assuming this isn't the last one, is there a better way to handle these bugs rather than slapping if BIG_ENDIAN everywhere? Could this be handled in the backend somehow?

Or maybe we just need a macro that provides a good abstraction for loading 32 bits that has a single if BIG_ENDIAN and then use that everywhere.

[Mark Lam]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 229056 [details] [details] [details])
> > > Does this fix all big endian issues? It seems to me like we do lots of loadi instructions assuming little endianness.
> > 
> > I would not presume that.  Tomas has been analyzing big endian failures in https://bugs.webkit.org/show_bug.cgi?id=12874.  I’m just knocking them down one at a time when he reports them.  This one happens to be the next one.  It may or may not be the last.
> 
> Hmm. Assuming this isn't the last one, is there a better way to handle these bugs rather than slapping if BIG_ENDIAN everywhere? Could this be handled in the backend somehow?

Correction on the url from my previous comment: it should be https://bugs.webkit.org/show_bug.cgi?id=128743.

That would be inappropriate to rig loadi in the backend because a loadi can be used to load the most significant 32-bit if that is the intent.  I’m going by the rule that the LLINT asm is assembly, and we should coding it the same way an assembly programmer would code.  In this case, the issue isolated to a macro, and that “if BIG_ENDIAN” is isolated.  Note: this is not a new idiom.  It was already in use previously, and I adopted it here to solve the issue.

[Mark Hahnenberg]

Comment on attachment 229056 [details]
patch 2: updated comment.

Okay, we'll refactor as we encounter new issues. YAGNI! r=me

[WebKit Commit Bot]

Comment on attachment 229056 [details]
patch 2: updated comment.

Clearing flags on attachment: 229056

Committed r167076: <http://trac.webkit.org/changeset/167076>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Lam]

For the record, this fix was invalid.  https://bugs.webkit.org/show_bug.cgi?id=132330 will undo it, and make it right.