Bug 170494 – REGRESSION fix bad isWasm() test by ensuring proper Wasm callee bit pattern

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

https://build.webkit.org/builders/Apple%20Sierra%20Debug%20JSC%20%28Tests%29/builds/111
https://build.webkit.org/builders/Apple%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/182

Dozens of failures on each.

[Radar WebKit Bug Importer]

<rdar://problem/31446485>

[Saam Barati]

Looks like my patch. I'll check it out.

[Saam Barati]

Created attachment 306268 [details]
patch

[Yusuke Suzuki]

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).

[Mark Lam]

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?

[Saam Barati]

(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?

[Saam Barati]

(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.

[Saam Barati]

(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.

[Yusuke Suzuki]

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.

[Saam Barati]

(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.

[Saam Barati]

Created attachment 306307 [details]
patch

[Build Bot]

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.

[Saam Barati]

Created attachment 306309 [details]
patch

[Build Bot]

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.

[Yusuke Suzuki]

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.

[Mark Lam]

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.

[Michael Saboff]

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*

[Saam Barati]

Created attachment 306311 [details]
patch for landing

[Build Bot]

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.

[WebKit Commit Bot]

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

[Saam Barati]

Created attachment 306332 [details]
patch for landing

remove tab

[WebKit Commit Bot]

Comment on attachment 306332 [details]
patch for landing

Clearing flags on attachment: 306332

Committed r214979: <http://trac.webkit.org/changeset/214979>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.