Bug 124913 – Do bytecode validation as part of testing

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 217924 [details]
the patch

[Nadav Rotem]

LGTM.   The FastVector change can go as a separate patch. 

What is this change:

-  unsigned numberOfVariables = unlinkedCodeBlock->m_numVars + 
+ unsigned numberOfVariables =

[Filip Pizlo]

(In reply to comment #2)
> LGTM.   The FastVector change can go as a separate patch. 

Yeah, it can land separately.  We often prefer not to land dead code - so if we add WTF methods that we use for feature X then the patch for feature X will usually include the WTF patch inside it.

I guess the theory is that if you branched WebKit at any arbitrary revision, not only would you expect to get a reasonably stable web engine, but also you would get one that didn't have unneeded code.

> 
> What is this change:
> 
> -  unsigned numberOfVariables = unlinkedCodeBlock->m_numVars + 
> + unsigned numberOfVariables =

A bug I found by adding the bytecode liveness analysis verifier.  I was surprised by the number of bits in the bitvector.  It turns out that the liveness analysis had a benign bug where it assumed that the number of variables was the sum of numVars and numCalleeRegs.  In fact, numCalleeRegs is numVars + numTemps, so adding numVars is incorrect.  Prior to this patch it would have been a benign bug because it really doesn't matter if you create a bitvector that is larger than you need.  I mean, you waste some memory, but probably not a lot.

[Build Bot]

Comment on attachment 217924 [details]
the patch

Attachment 217924 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/37828025

[Filip Pizlo]

Created attachment 217958 [details]
the patch

[Filip Pizlo]

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