Bug 124913 - Do bytecode validation as part of testing
Summary: Do bytecode validation as part of testing
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:
Blocks:
 
Reported: 2013-11-26 21:10 PST by Filip Pizlo
Modified: 2013-11-27 15:13 PST (History)
13 users (show)

See Also:


Attachments
the patch (16.89 KB, patch)
2013-11-26 21:13 PST, Filip Pizlo
buildbot: commit-queue-
Details | Formatted Diff | Diff
the patch (19.03 KB, patch)
2013-11-27 10:28 PST, Filip Pizlo
oliver: 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-26 21:10:17 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2013-11-26 21:13:58 PST
Created attachment 217924 [details]
the patch
Comment 2 Nadav Rotem 2013-11-26 21:38:36 PST
LGTM.   The FastVector change can go as a separate patch. 

What is this change:

-  unsigned numberOfVariables = unlinkedCodeBlock->m_numVars + 
+ unsigned numberOfVariables =
Comment 3 Filip Pizlo 2013-11-26 21:45:07 PST
(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.
Comment 4 Build Bot 2013-11-26 21:57:35 PST
Comment on attachment 217924 [details]
the patch

Attachment 217924 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/37828025
Comment 5 Filip Pizlo 2013-11-27 10:28:23 PST
Created attachment 217958 [details]
the patch
Comment 6 Filip Pizlo 2013-11-27 15:13:25 PST
Landed in http://trac.webkit.org/changeset/159825