Bug 124735 - BytecodeGenerator should align the stack according to native conventions
Summary: BytecodeGenerator should align the stack according to native conventions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 124736 124778
Blocks: 116888
  Show dependency treegraph
 
Reported: 2013-11-21 12:31 PST by Filip Pizlo
Modified: 2013-11-22 12:16 PST (History)
10 users (show)

See Also:


Attachments
the patch (3.62 KB, patch)
2013-11-21 14:25 PST, Filip Pizlo
mark.lam: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (445.52 KB, application/zip)
2013-11-21 15:17 PST, Build Bot
no flags Details
patch for landing (5.30 KB, patch)
2013-11-21 15:47 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
patch for relanding (6.75 KB, patch)
2013-11-22 10:28 PST, Filip Pizlo
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2013-11-21 12:31:15 PST
Patch forthcoming
Comment 1 Filip Pizlo 2013-11-21 14:25:37 PST
Created attachment 217613 [details]
the patch
Comment 2 Mark Lam 2013-11-21 15:17:14 PST
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.
Comment 3 Build Bot 2013-11-21 15:17:15 PST
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
Comment 4 Geoffrey Garen 2013-11-21 15:31:56 PST
NOTE: We also want to round up CodeBlock::m_numCalleeRegisters.
Comment 5 Filip Pizlo 2013-11-21 15:42:32 PST
(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.
Comment 6 Filip Pizlo 2013-11-21 15:47:43 PST
Created attachment 217621 [details]
patch for landing
Comment 7 Filip Pizlo 2013-11-21 15:53:47 PST
Landed in http://trac.webkit.org/changeset/159652
Comment 8 Mark Lam 2013-11-21 15:59:29 PST
(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?
Comment 9 Mark Lam 2013-11-21 17:52:44 PST
(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.
Comment 10 WebKit Commit Bot 2013-11-22 09:27:28 PST
Re-opened since this is blocked by bug 124778
Comment 11 Alexey Proskuryakov 2013-11-22 09:28:58 PST
EWS detected that this patch breaks a test, and you landed it anyway, seriously?

Where were bot watchers?
Comment 12 Filip Pizlo 2013-11-22 10:20:51 PST
(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?
Comment 13 Filip Pizlo 2013-11-22 10:28:58 PST
Created attachment 217697 [details]
patch for relanding
Comment 14 Mark Lam 2013-11-22 10:31:22 PST
Comment on attachment 217697 [details]
patch for relanding

Fil, don't you mean to update the result, not delete it?
Comment 15 Filip Pizlo 2013-11-22 12:13:29 PST
(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.
Comment 16 Filip Pizlo 2013-11-22 12:16:09 PST
Landed in http://trac.webkit.org/changeset/159705