Bug 124735 – BytecodeGenerator should align the stack according to native conventions

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming

[Filip Pizlo]

Created attachment 217613 [details]
the patch

[Mark Lam]

Comment on attachment 217613 [details]
the patch

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

r=me

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:72
> +        unsigned registerOffset() { return -(m_argv[0]->index() - JSStack::CallFrameHeaderSize); }

You're doing double negate on JSStack::CallFrameHeaderSize here.  Instead, please change this to:
    return -m_argv[0]->index() + JSStack::CallFrameHeaderSize;

Also, though the name registerOffset() is probably outdated.  From looking at the code, I think it would be more appropriately renamed numberOfRegisters() or registerCount().  If you agree, would you mind changing that as well?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:440
> +    

Please remove the blank spaces on this line.

[Build Bot]

Created attachment 217620 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Geoffrey Garen]

NOTE: We also want to round up CodeBlock::m_numCalleeRegisters.

[Filip Pizlo]

(In reply to comment #2)
> (From update of attachment 217613 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217613&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:72
> > +        unsigned registerOffset() { return -(m_argv[0]->index() - JSStack::CallFrameHeaderSize); }
> 
> You're doing double negate on JSStack::CallFrameHeaderSize here.  Instead, please change this to:
>     return -m_argv[0]->index() + JSStack::CallFrameHeaderSize;
> 
> Also, though the name registerOffset() is probably outdated.

I'll change it to stackOffset.

> From looking at the code, I think it would be more appropriately renamed numberOfRegisters() or registerCount().  If you agree, would you mind changing that as well?

Nope.  It's no the number of registers.  It's an offset into the frame.

> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:440
> > +    
> 
> Please remove the blank spaces on this line.

OK.

[Filip Pizlo]

Created attachment 217621 [details]
patch for landing

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/159652

[Mark Lam]

(In reply to comment #7)
> Landed in http://trac.webkit.org/changeset/159652

What about Geoff's comment about the CodeBlock::m_numCalleeRegisters?   Will that be handled in a separate patch?

[Mark Lam]

(In reply to comment #8)
> What about Geoff's comment about the CodeBlock::m_numCalleeRegisters?   Will that be handled in a separate patch?

Let's address the CodeBlock::m_numCalleeRegisters part in https://bugs.webkit.org/show_bug.cgi?id=124754.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 124778

[Alexey Proskuryakov]

EWS detected that this patch breaks a test, and you landed it anyway, seriously?

Where were bot watchers?

[Filip Pizlo]

(In reply to comment #11)
> EWS detected that this patch breaks a test, and you landed it anyway, seriously?

Yes. Seriously.

> 
> Where were bot watchers?

Did you seriously just roll out a patch because it failed in this way:

--- /Volumes/Data/slave/mountainlion-debug-tests-wk1/build/layout-test-results/fast/dom/gc-attribute-node-expected.txt
+++ /Volumes/Data/slave/mountainlion-debug-tests-wk1/build/layout-test-results/fast/dom/gc-attribute-node-actual.txt
@@ -4,7 +4,7 @@
 
 
 PASS a.prop is "set"
-FAIL a.prop should be set (of type string). Was undefined (of type undefined).
+PASS a.prop is "set"
 PASS successfullyParsed is true
 
 TEST COMPLETE

I'm not an expert on layout tests but replacing "FAIL" with "PASS" means that this patch fixed things, right?

[Filip Pizlo]

Created attachment 217697 [details]
patch for relanding

[Mark Lam]

Comment on attachment 217697 [details]
patch for relanding

Fil, don't you mean to update the result, not delete it?

[Filip Pizlo]

(In reply to comment #14)
> (From update of attachment 217697 [details])
> Fil, don't you mean to update the result, not delete it?

I meant to delete it.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/159705