Summary: | REGRESSION fix bad isWasm() test by ensuring proper Wasm callee bit pattern | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, commit-queue, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, ryanhaddad, saam, ticaiolima, webkit-bug-importer, ysuzuki | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Local Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2017-04-04 22:20:15 PDT
Looks like my patch. I'll check it out. Created attachment 306268 [details]
patch
Comment on attachment 306268 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306268&action=review This patch looks good. But I have a question about Wasm::Callee. > Source/JavaScriptCore/ChangeLog:20 > + the function a scarier name. I think we still have a room to fill a pointer to JSValue's NaN boxing bit patterns. Since Wasm::Callee is not gc-managed, dirty pointer in JSValue is OK without drastically changing the GC marking. Is there any problem when assigning a new tag to Wasm::Callee in JSValue? For example, we can choose the tag `0b11` (TagBitTypeOther | true). Comment on attachment 306268 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306268&action=review > Source/JavaScriptCore/interpreter/CallFrame.h:-92 > - ASSERT(!callee().isWasm()); Can't you just assert that: 1, the callee() looks like a cell (i.e. is a pointer), and 2. the callee() has WasmTag bit set? (In reply to Yusuke Suzuki from comment #4) > Comment on attachment 306268 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306268&action=review > > This patch looks good. But I have a question about Wasm::Callee. > > > Source/JavaScriptCore/ChangeLog:20 > > + the function a scarier name. > > I think we still have a room to fill a pointer to JSValue's NaN boxing bit > patterns. > Since Wasm::Callee is not gc-managed, dirty pointer in JSValue is OK without > drastically changing the GC marking. > Is there any problem when assigning a new tag to Wasm::Callee in JSValue? > For example, we can choose the tag `0b11` (TagBitTypeOther | true). If we used 0b011 to tag Wasm::Callee it'd work. The isWasm() test would need to be something like this: 0b011 in the lower 3 bits, and 0xffff000000000000 & ptr == 0 in the top bits. I'll switch to doing this. I think the full test for isWasm would be something like this: isWasm(uintptr_t x) { return x & 0xffff000000000007 == 3; } Does that look right to you? (In reply to Mark Lam from comment #5) > Comment on attachment 306268 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306268&action=review > > > Source/JavaScriptCore/interpreter/CallFrame.h:-92 > > - ASSERT(!callee().isWasm()); > > Can't you just assert that: > 1, the callee() looks like a cell (i.e. is a pointer), and > 2. the callee() has WasmTag bit set? The problem is that the callee might not look like this in this particular context. (In reply to Saam Barati from comment #6) > (In reply to Yusuke Suzuki from comment #4) > > Comment on attachment 306268 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=306268&action=review > > > > This patch looks good. But I have a question about Wasm::Callee. > > > > > Source/JavaScriptCore/ChangeLog:20 > > > + the function a scarier name. > > > > I think we still have a room to fill a pointer to JSValue's NaN boxing bit > > patterns. > > Since Wasm::Callee is not gc-managed, dirty pointer in JSValue is OK without > > drastically changing the GC marking. > > Is there any problem when assigning a new tag to Wasm::Callee in JSValue? > > For example, we can choose the tag `0b11` (TagBitTypeOther | true). > > If we used 0b011 to tag Wasm::Callee it'd work. The isWasm() test would need > to be something like this: 0b011 in the lower 3 bits, and 0xffff000000000000 > & ptr == 0 in the top bits. I'll switch to doing this. I think the full test > for isWasm would be something like this: > > isWasm(uintptr_t x) { > return x & 0xffff000000000007 == 3; > } > > Does that look right to you? I just talked through this with Mark, and it's totally wrong. But, we can tell by the upper 16 bytes and lower 3 bits if the thing is Wasm. Here is how. We tag all Wasm callee's with 1 at the bottom, as we're doing now. Notice the lower 3 bits of the other primitives are: undefined: 0b010 null: 0b010 true: 0b111 false: 0b110 Then, the test simply be this: isWasm(uintptr_t x) { return x & 0xffff000000000007 == 1; } Notice it'd fail for the above four immediate values. It'd also fail for every single tagged number. It would also fail for normal cells. Comment on attachment 306268 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306268&action=review >>>> Source/JavaScriptCore/ChangeLog:20 >>>> + the function a scarier name. >>> >>> I think we still have a room to fill a pointer to JSValue's NaN boxing bit patterns. >>> Since Wasm::Callee is not gc-managed, dirty pointer in JSValue is OK without drastically changing the GC marking. >>> Is there any problem when assigning a new tag to Wasm::Callee in JSValue? For example, we can choose the tag `0b11` (TagBitTypeOther | true). >> >> If we used 0b011 to tag Wasm::Callee it'd work. The isWasm() test would need to be something like this: 0b011 in the lower 3 bits, and 0xffff000000000000 & ptr == 0 in the top bits. I'll switch to doing this. I think the full test for isWasm would be something like this: >> >> isWasm(uintptr_t x) { >> return x & 0xffff000000000007 == 3; >> } >> >> Does that look right to you? > > I just talked through this with Mark, and it's totally wrong. But, we can tell by the upper 16 bytes and lower 3 bits if the thing is Wasm. Here is how. We tag all Wasm callee's with 1 at the bottom, as we're doing now. > Notice the lower 3 bits of the other primitives are: > > undefined: 0b010 > null: 0b010 > true: 0b111 > false: 0b110 > > Then, the test simply be this: > isWasm(uintptr_t x) { > return x & 0xffff000000000007 == 1; > } > > Notice it'd fail for the above four immediate values. It'd also fail for every single tagged number. It would also fail for normal cells. I think having TagBitTypeOther (0b10) too is good. We use TagMask (TagTypeNumber | TagBitTypeOther) to distinguish Cells from the other values. So, by categorizing Wasm::Callee to Other, we can still keep the fast `isCell()` correct. (it will return false!) And 0b11 also does not have TagTypeNumber, TagBitUndefined and TagBitBool. So other predicates (like isBoolean(), isUndefinedOrNull(), isUndefined()) just work I think. (In reply to Yusuke Suzuki from comment #9) > Comment on attachment 306268 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306268&action=review > > >>>> Source/JavaScriptCore/ChangeLog:20 > >>>> + the function a scarier name. > >>> > >>> I think we still have a room to fill a pointer to JSValue's NaN boxing bit patterns. > >>> Since Wasm::Callee is not gc-managed, dirty pointer in JSValue is OK without drastically changing the GC marking. > >>> Is there any problem when assigning a new tag to Wasm::Callee in JSValue? For example, we can choose the tag `0b11` (TagBitTypeOther | true). > >> > >> If we used 0b011 to tag Wasm::Callee it'd work. The isWasm() test would need to be something like this: 0b011 in the lower 3 bits, and 0xffff000000000000 & ptr == 0 in the top bits. I'll switch to doing this. I think the full test for isWasm would be something like this: > >> > >> isWasm(uintptr_t x) { > >> return x & 0xffff000000000007 == 3; > >> } > >> > >> Does that look right to you? > > > > I just talked through this with Mark, and it's totally wrong. But, we can tell by the upper 16 bytes and lower 3 bits if the thing is Wasm. Here is how. We tag all Wasm callee's with 1 at the bottom, as we're doing now. > > Notice the lower 3 bits of the other primitives are: > > > > undefined: 0b010 > > null: 0b010 > > true: 0b111 > > false: 0b110 > > > > Then, the test simply be this: > > isWasm(uintptr_t x) { > > return x & 0xffff000000000007 == 1; > > } > > > > Notice it'd fail for the above four immediate values. It'd also fail for every single tagged number. It would also fail for normal cells. > > I think having TagBitTypeOther (0b10) too is good. > We use TagMask (TagTypeNumber | TagBitTypeOther) to distinguish Cells from > the other values. > So, by categorizing Wasm::Callee to Other, we can still keep the fast > `isCell()` correct. (it will return false!) > > And 0b11 also does not have TagTypeNumber, TagBitUndefined and TagBitBool. > So other predicates (like isBoolean(), isUndefinedOrNull(), isUndefined()) > just work I think. Yeah we might as well do this too. We won't use this property right now, but I could see it coming in handy. Created attachment 306307 [details]
patch
Attachment 306307 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:437: preprocessor directives (e.g., #ifdef, #define, #import) should never be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:438: preprocessor directives (e.g., #ifdef, #define, #import) should never be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:444: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:445: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:446: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5]
Total errors found: 6 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 306309 [details]
patch
Attachment 306309 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:437: preprocessor directives (e.g., #ifdef, #define, #import) should never be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:438: preprocessor directives (e.g., #ifdef, #define, #import) should never be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:444: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:445: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:446: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5]
Total errors found: 6 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 306309 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306309&action=review r=me, nice. > Source/JavaScriptCore/interpreter/CalleeBits.h:66 > +#if ENABLE(WEBASSEMBLY) > + return (bitwise_cast<uintptr_t>(m_ptr) & TagWasmMask) == TagBitsWasm; > +#else > + return false; > +#endif I think this if-def is not necessary. Comment on attachment 306309 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306309&action=review r=me too with suggestions. > Source/JavaScriptCore/ChangeLog:24 > + The test succeeds for all of these because non have just the value 3 in their lower 3 bits. > + The test also succeeds for numbers, because they have non-zero upper 16 bits. > + The test also succeeds for normal cells because they won't have the number 3 as Ditto. Same feedback as below ... recommend use "rejects" instead of "succeeds". >> Source/JavaScriptCore/interpreter/CalleeBits.h:66 >> +#endif > > I think this if-def is not necessary. It is needed for 32-bit builds. > Source/JavaScriptCore/runtime/JSCJSValue.h:449 > + // The test succeeds for all of these because non have just the value 3 in their lower 3 bits. > + // The test also succeeds for numbers, because they have non-zero upper 16 bits. > + // The test also succeeds for normal cells because they won't have the number 3 as "succeeds" here reads like the "will return true". How about saying "The test rejects ..." instead. Comment on attachment 306309 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=306309&action=review > Source/JavaScriptCore/ChangeLog:22 > + The test succeeds for all of these because non have just the value 3 in their lower 3 bits. *none* Created attachment 306311 [details]
patch for landing
Attachment 306311 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:437: preprocessor directives (e.g., #ifdef, #define, #import) should never be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:438: preprocessor directives (e.g., #ifdef, #define, #import) should never be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:444: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:445: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:446: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5]
Total errors found: 6 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 306311 [details] patch for landing Rejecting attachment 306311 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 306311, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 214970 = d15f8e3db6e722ee6988a296363be1763b6c1140 r214973 = ea5477036d9c4bc0e8af899bffec4d6afa6c1c70 r214974 = 7245903ff93c5e131d3b24f80e4b3956f3fd74d5 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://webkit-queues.webkit.org/results/3481314 Created attachment 306332 [details]
patch for landing
remove tab
Comment on attachment 306332 [details] patch for landing Clearing flags on attachment: 306332 Committed r214979: <http://trac.webkit.org/changeset/214979> All reviewed patches have been landed. Closing bug. |