Bug 133340

Summary: Arrayify neglects to inform the clobberizer that it might fire watchpoints
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, mark.lam, mhahnenberg, msaboff, oliver, ossy, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch mark.lam: review+

Description Filip Pizlo 2014-05-27 23:23:34 PDT
And the 32-bit LLInt gets array profiles all wrong, which is probably the reason why this only showed up in 32-bit debug, and it also makes testing this in general much harder.
Comment 1 Filip Pizlo 2014-05-27 23:24:42 PDT
Created attachment 232172 [details]
the patch
Comment 2 Mark Lam 2014-05-28 08:05:18 PDT
Comment on attachment 232172 [details]
the patch

r=me
Comment 3 Filip Pizlo 2014-05-28 12:01:02 PDT
Landed in http://trac.webkit.org/changeset/169428
Comment 4 Csaba Osztrogon√°c 2014-05-28 12:39:18 PDT
Comment on attachment 232172 [details]
the patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1408
> -    loadp JSCell::m_structureID[t3], t2
> +    loadp t3, t2

It broke the ARM Thumb2 Linux build:
[  4%] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o
/tmp/ccowkxrW.s: Assembler messages:
/tmp/ccowkxrW.s:3508: Error: cannot represent T32_OFFSET_IMM relocation in this object file format
make[2]: *** [Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o] Error 1

And it broke the ARM Traditional Linux build:
[  4%] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o
/tmp/ccJfKxfJ.s: Assembler messages:
/tmp/ccJfKxfJ.s:3449: Error: internal_relocation (type: OFFSET_IMM) not fixed up
make[2]: *** [Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o] Error 1
make[1]: *** [Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/all] Error 2

Didn't you mean move t3, t2 here as the other part of the patch?
Comment 5 Filip Pizlo 2014-05-28 12:40:04 PDT
(In reply to comment #4)
> (From update of attachment 232172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232172&action=review
> 
> > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1408
> > -    loadp JSCell::m_structureID[t3], t2
> > +    loadp t3, t2
> 
> It broke the ARM Thumb2 Linux build:
> [  4%] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o
> /tmp/ccowkxrW.s: Assembler messages:
> /tmp/ccowkxrW.s:3508: Error: cannot represent T32_OFFSET_IMM relocation in this object file format
> make[2]: *** [Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o] Error 1
> 
> And it broke the ARM Traditional Linux build:
> [  4%] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o
> /tmp/ccJfKxfJ.s: Assembler messages:
> /tmp/ccJfKxfJ.s:3449: Error: internal_relocation (type: OFFSET_IMM) not fixed up
> make[2]: *** [Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o] Error 1
> make[1]: *** [Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/all] Error 2
> 
> Didn't you mean move t3, t2 here as the other part of the patch?

Yes.
Comment 6 Filip Pizlo 2014-05-28 12:42:34 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 232172 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=232172&action=review
> > 
> > > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1408
> > > -    loadp JSCell::m_structureID[t3], t2
> > > +    loadp t3, t2
> > 
> > It broke the ARM Thumb2 Linux build:
> > [  4%] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o
> > /tmp/ccowkxrW.s: Assembler messages:
> > /tmp/ccowkxrW.s:3508: Error: cannot represent T32_OFFSET_IMM relocation in this object file format
> > make[2]: *** [Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o] Error 1
> > 
> > And it broke the ARM Traditional Linux build:
> > [  4%] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o
> > /tmp/ccJfKxfJ.s: Assembler messages:
> > /tmp/ccJfKxfJ.s:3449: Error: internal_relocation (type: OFFSET_IMM) not fixed up
> > make[2]: *** [Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o] Error 1
> > make[1]: *** [Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/all] Error 2
> > 
> > Didn't you mean move t3, t2 here as the other part of the patch?
> 
> Yes.

Fixed in http://trac.webkit.org/changeset/169431