Bug 131495

Summary: LLINT loadisFromInstruction should handle the big endian case
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren, mhahnenberg, mmirman, msaboff, oliver, tpopela
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 128743    
Attachments:
Description Flags
The patch.
none
patch 2: updated comment. none

Description Mark Lam 2014-04-10 09:56:20 PDT
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.
Comment 1 Mark Lam 2014-04-10 10:01:55 PDT
Created attachment 229055 [details]
The patch.
Comment 2 Mark Lam 2014-04-10 10:03:47 PDT
Created attachment 229056 [details]
patch 2: updated comment.
Comment 3 Mark Hahnenberg 2014-04-10 10:12:37 PDT
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.
Comment 4 Mark Lam 2014-04-10 10:14:22 PDT
(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.
Comment 5 Mark Hahnenberg 2014-04-10 10:15:31 PDT
(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?
Comment 6 Mark Hahnenberg 2014-04-10 10:18:46 PDT
(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.
Comment 7 Mark Lam 2014-04-10 10:19:50 PDT
(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.
Comment 8 Mark Hahnenberg 2014-04-10 10:20:42 PDT
Comment on attachment 229056 [details]
patch 2: updated comment.

Okay, we'll refactor as we encounter new issues. YAGNI! r=me
Comment 9 WebKit Commit Bot 2014-04-10 10:50:42 PDT
Comment on attachment 229056 [details]
patch 2: updated comment.

Clearing flags on attachment: 229056

Committed r167076: <http://trac.webkit.org/changeset/167076>
Comment 10 WebKit Commit Bot 2014-04-10 10:50:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Mark Lam 2014-04-29 06:13:34 PDT
For the record, this fix was invalid.  https://bugs.webkit.org/show_bug.cgi?id=132330 will undo it, and make it right.