Bug 104085 – Fix broken WinCE build due to call to add64 in JIT.cpp

• Status: CLOSED
• Resolution: DUPLICATE
• Component: JavaScriptCore

[Vivek Galatage]

The method add64() is called inside JIT::privateCompileMainPass(). This call should be under the #if USE(JSVALUE64) guard as its not implemented in WinCE MacroAssemblerARM.cpp

Patch follows.

[Vivek Galatage]

Created attachment 177666 [details]
Patch

[Patrick R. Gansterer]

Comment on attachment 177666 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177666&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        This call should be under the #if USE(JSVALUE64) guard as its not implemented in WinCE MacroAssemblerARM.cpp

This has _nothing_ to do with WinCE. It's a "problem" of the traditional ARM MacroAssembler.
Correct way to fix this is to implement add64() in the MacroAssembler.
Usually the szeged guys maintain the traditional ARM (e.g. https://trac.webkit.org/changeset/135610).

[Vivek Galatage]

(In reply to comment #2)
> (From update of attachment 177666 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177666&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        This call should be under the #if USE(JSVALUE64) guard as its not implemented in WinCE MacroAssemblerARM.cpp
> 
> This has _nothing_ to do with WinCE. It's a "problem" of the traditional ARM MacroAssembler.

Thank you Patrick, I wasn't aware of this.

> Correct way to fix this is to implement add64() in the MacroAssembler.
> Usually the szeged guys maintain the traditional ARM (e.g. https://trac.webkit.org/changeset/135610).

I added the macro guard as currently all the calls to add64 are under the #if USE(JSVALUE64) except this one. Hence thought this would fix the issue for the time being until a real add64 is implemented. But may be I am wrong with the approach. Thanks for pointing this.

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 177666 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=177666&action=review
> > 
> > > Source/JavaScriptCore/ChangeLog:9
> > > +        This call should be under the #if USE(JSVALUE64) guard as its not implemented in WinCE MacroAssemblerARM.cpp
> > 
> > This has _nothing_ to do with WinCE. It's a "problem" of the traditional ARM MacroAssembler.
> 
> Thank you Patrick, I wasn't aware of this.
> 
> > Correct way to fix this is to implement add64() in the MacroAssembler.
> > Usually the szeged guys maintain the traditional ARM (e.g. https://trac.webkit.org/changeset/135610).
> 
> I added the macro guard as currently all the calls to add64 are under the #if USE(JSVALUE64) except this one. Hence thought this would fix the issue for the time being until a real add64 is implemented. But may be I am wrong with the approach. Thanks for pointing this.

I don't recommend using this kind of thinking in the future. Your change breaks a new JSC feature on all 32-bit platforms just because one particular kind of 32-bit platform has a broken assembler. We don't usually like to fix bugs by introducing new ones.

[Vivek Galatage]

(In reply to comment #4)
> I don't recommend using this kind of thinking in the future. Your change breaks a new JSC feature on all 32-bit platforms just because one particular kind of 32-bit platform has a broken assembler. We don't usually like to fix bugs by introducing new ones.

I apologize for not considering the consequences of my changes. I will make sure to consider all these scenarios before submitting the patches going forward. Thanks for your feedback, really appreciated.

[Vivek Galatage]

*** This bug has been marked as a duplicate of bug 104103 ***