Bug 123799 – REGRESSION(r158315): register mixup in JIT::compileOpCall

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Julien Brianceau]

Changeset 158315 (http://trac.webkit.org/changeset/158315) is crashing architectures where JSInterfaceJIT::regT3 is different from GPRInfo::regT3, like in mips.

[Julien Brianceau]

Created attachment 216026 [details]
Use regT3 instead of GPRInfo::regT3 in JIT::compileOpCall

[Mark Lam]

Comment on attachment 216026 [details]
Use regT3 instead of GPRInfo::regT3 in JIT::compileOpCall

r=me

[WebKit Commit Bot]

Comment on attachment 216026 [details]
Use regT3 instead of GPRInfo::regT3 in JIT::compileOpCall

Clearing flags on attachment: 216026

Committed r158672: <http://trac.webkit.org/changeset/158672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Why does MIPS use a different definition of regT3?

Why not fix that problem?

[Mark Lam]

(In reply to comment #5)
> Why does MIPS use a different definition of regT3?
> 
> Why not fix that problem?

I agree that we should make the register assignments consistent.  However, MIPS isn't the only CPU port that has this issue.  For now, I r+ the change to resolve the immediate action.  Longer term, we should look into unifying the register assignments in GPRInfo.h and JSInterfaceJIT.h.

[Filip Pizlo]

(In reply to comment #6)
> (In reply to comment #5)
> > Why does MIPS use a different definition of regT3?
> > 
> > Why not fix that problem?
> 
> I agree that we should make the register assignments consistent.  However, MIPS isn't the only CPU port that has this issue.  For now, I r+ the change to resolve the immediate action.  Longer term, we should look into unifying the register assignments in GPRInfo.h and JSInterfaceJIT.h.

Which others have this problem?

[Julien Brianceau]

(In reply to comment #5)
> Why does MIPS use a different definition of regT3?
Good question, I don't know why MIPS backend has been designed like this.
 
> Why not fix that problem?
I can try if you think it's worth to do so.

[Mark Lam]

(In reply to comment #7)
> Which others have this problem?

Apart from MIPS, at a glance ...

For this exact problem:

CPU(ARM64):  regT3:  x23    vs    x3

For other discrepancies between GPRInfo.h and JSInterfaceJIT.h:

CPU(X86_86): regT4:   r10     vs    edi
CPU(ARM):      regT4:   r7      vs    r8
CPU(ARM64):  regT4:   x24    vs    x4

[Filip Pizlo]

(In reply to comment #9)
> (In reply to comment #7)
> > Which others have this problem?
> 
> Apart from MIPS, at a glance ...
> 
> For this exact problem:
> 
> CPU(ARM64):  regT3:  x23    vs    x3
> 
> For other discrepancies between GPRInfo.h and JSInterfaceJIT.h:
> 
> CPU(X86_86): regT4:   r10     vs    edi
> CPU(ARM):      regT4:   r7      vs    r8
> CPU(ARM64):  regT4:   x24    vs    x4

We should fix that by getting rid of the regT* definitions in JSInterfaceJIT.h

If some backend can't handle it then that's a separate problem.

[Mark Lam]

(In reply to comment #10)
> We should fix that by getting rid of the regT* definitions in JSInterfaceJIT.h

I'll work on it in https://bugs.webkit.org/show_bug.cgi?id=123806.

[Julien Brianceau]

As discussed with Mark on IRC, I'll submit a patch soon to fix MIPS backend in https://bugs.webkit.org/show_bug.cgi?id=123807