Bug 68746 - wtf/BitVector.h has a variety of bugs which manifest when the vector grows beyond 63 bits
Summary: wtf/BitVector.h has a variety of bugs which manifest when the vector grows be...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-23 17:49 PDT by Filip Pizlo
Modified: 2011-09-23 20:24 PDT (History)
3 users (show)

See Also:


Attachments
the patch (16.87 KB, patch)
2011-09-23 17:54 PDT, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff
the patch - fix style, review (16.45 KB, patch)
2011-09-23 18:33 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-09-23 17:49:13 PDT
- The number of bytes that need to be malloc'd is miscomputed.

- The index into the uintptr_t* is miscomputed.

- resize() sometimes exits too early.

- Far too much code is inline, when it could be trivially out-of-line.
Comment 1 Filip Pizlo 2011-09-23 17:54:47 PDT
Created attachment 108570 [details]
the patch
Comment 2 WebKit Review Bot 2011-09-23 17:56:50 PDT
Attachment 108570 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/BitVector.h:143:  The parameter name "outOfLineBits" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Oliver Hunt 2011-09-23 18:00:08 PDT
Comment on attachment 108570 [details]
the patch

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

r=me, but it would be worth considering whether ensureSize() should be inline or outline before landing

> Source/JavaScriptCore/wtf/BitVector.h:79
> -    void ensureSize(size_t numBits)
> -    {
> -        if (numBits <= size())
> -            return;
> -        resizeOutOfLine(numBits);
> -    }
> +    void ensureSize(size_t numBits);

Do you really want to take ensureSize out of line? you already have resizeOutOfLine so it seems overkill to hoist the entire function out
Comment 4 Filip Pizlo 2011-09-23 18:02:36 PDT
(In reply to comment #3)
> (From update of attachment 108570 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108570&action=review
> 
> r=me, but it would be worth considering whether ensureSize() should be inline or outline before landing
> 
> > Source/JavaScriptCore/wtf/BitVector.h:79
> > -    void ensureSize(size_t numBits)
> > -    {
> > -        if (numBits <= size())
> > -            return;
> > -        resizeOutOfLine(numBits);
> > -    }
> > +    void ensureSize(size_t numBits);
> 
> Do you really want to take ensureSize out of line? you already have resizeOutOfLine so it seems overkill to hoist the entire function out

That's a really good point.  I'll change that so that ensureSize() inlines the trivial path, and land.
Comment 5 Filip Pizlo 2011-09-23 18:33:01 PDT
Created attachment 108573 [details]
the patch - fix style, review

This is the patch I intend to land manually once I finish running the last batch of tests.
Comment 6 Filip Pizlo 2011-09-23 18:36:58 PDT
It appears that this is a speed-up on a few benchmarks, presumably because previously we had corrupt liveness settings and OSR would fail spuriously.  I cannot find any other change in behavior, since any bugs arising from a corrupt OSR bitvector would be horribly annoying to track down.
Comment 7 Filip Pizlo 2011-09-23 19:07:22 PDT
Landed in r95895.
Comment 8 Darin Adler 2011-09-23 20:24:29 PDT
Comment on attachment 108570 [details]
the patch

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

> Source/JavaScriptCore/wtf/BitVector.cpp:38
> +    (*this) = other;

No need for these parentheses.

> Source/JavaScriptCore/wtf/BitVector.cpp:41
> +BitVector& BitVector::operator=(const BitVector& other)

It’s normally considered better style to write a swap function and a copy constructor and build the assignment operator from the copy constructor instead of the other way around.

Easier to get rid too.