Summary: | BitVector isInline check could fail | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yuqiang Xian <yuqiang.xian> | ||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | barraclough, fpizlo, ggaren, webkit.review.bot | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Yuqiang Xian
2011-10-23 05:16:56 PDT
Created attachment 112108 [details]
proposed patch
> One possible fix is to use the lowest bit of m_bitsOrPointer to indicate inline or outofline, based on the assumption that the pointer to OutOfLineBits should be 4 or 8 byte aligned (please correct me if we can safely make this assumption).
I think this is a reasonable assumption.
You can do two things to make your assumption clearer to the reader:
(1) Add an ASSERT(!isInline()) at the end of BitVector::resizeOutOfLine()
(2) Add a comment above the declaration of m_bitsOrPointer explaining what the low bit means:
"The low bit of m_bitsOrPointer is a flag indicating whether this field is inline bits or a pointer to out of line bits. If the flag is set, the field is inline bits. This works because the low bit in a pointer is always unset."
Comment on attachment 112108 [details]
proposed patch
The approach here is good, but I think a few tweaks would make this patch even better.
Created attachment 112133 [details]
the patch
Patch addressing the review comments. Thanks.
Comment on attachment 112133 [details]
the patch
r=me
Comment on attachment 112133 [details] the patch Clearing flags on attachment: 112133 Committed r98268: <http://trac.webkit.org/changeset/98268> All reviewed patches have been landed. Closing bug. This fix was actually wrong in two ways: 1) It adds one to the index in the bitops only in the mask part, so for example accessing bit 31 on 32-bit systems will instead access bit 0 in the first word. 2) It does not account for the "lost bit" in the out-of-line bit storage. I think that the simpler fix is to: 1) Switch back to the way we did things before, since fixing both (1) and (2) seems like begging for off-by-one errors. 2) Fix the original bug by storing the out-of-line bits pointer right-shifted by 1, to make room for the is-inline marker in the top bit. Created attachment 117365 [details]
the patch
Landed in http://trac.webkit.org/changeset/101639 Comment on attachment 117365 [details]
the patch
Clearing flags.
Oops... sorry for introducing the bug... |