Bug 133340

Summary:

Arrayify neglects to inform the clobberizer that it might fire watchpoints

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, ggaren, mark.lam, mhahnenberg, msaboff, oliver, ossy, sam

Priority:

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