Bug 164432 – [JSC] The implementation of 8 bit operation in MacroAssembler should care about uint8_t / int8_t

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

In r203331, we fixed some bugs.
But the behavior is not changed. We just drop the meaningless assertions.

However, MacroAssembler still has a bug.
In ARM, ARM64 assemblers, 8bit operations are implementation like this.

Jump branch8(RelationCondition cord, Address left, TrustedImm32 right)
{
    TrustedImm32 right8(static_cast<int8_t>(right.m_value));
    load8(left, getCachedMemoryTempRegisterIDAndInvalidate());
    return branch32(cone, memoryTempRegister, right8);
}

The problem is, load8 does not perform sign extension. So if you pass signed 8bit value in |right|, the above code has a bad time. 32bit extended values becomes different. One is sign extended, another is not.

[Yusuke Suzuki]

https://trac.webkit.org/changeset/203331

[Yusuke Suzuki]

Current ARM64 branch8 always use load8 to load 8bit value to a register.
It means that we cannot perform signed operations like GreaterThan, LessThan etc.

addr holds -1 as int8_t.
branch8(LessThan, addr, 0) =>

load8(addr) => 32bit 255 is sotred in the register
255 < 0 (IN 32bit context).

=> false. But it should return true.

If we would like to make it work, we should select load8 / load8SignedExtendTo32 based on the condition.

[Yusuke Suzuki]

This issue actually causes https://bugs.webkit.org/show_bug.cgi?id=164004's crash.
Since now JSType(8bit) may have the highest bit.

[Yusuke Suzuki]

(In reply to comment #2)
> Current ARM64 branch8 always use load8 to load 8bit value to a register.
> It means that we cannot perform signed operations like GreaterThan, LessThan
> etc.
> 
> addr holds -1 as int8_t.
> branch8(LessThan, addr, 0) =>
> 
> load8(addr) => 32bit 255 is sotred in the register
> 255 < 0 (IN 32bit context).
> 
> => false. But it should return true.
> 
> If we would like to make it work, we should select load8 /
> load8SignedExtendTo32 based on the condition.

And also, we should select the way to mask the 32bit value.

[Filip Pizlo]

(In reply to comment #4)
> (In reply to comment #2)
> > Current ARM64 branch8 always use load8 to load 8bit value to a register.
> > It means that we cannot perform signed operations like GreaterThan, LessThan
> > etc.
> > 
> > addr holds -1 as int8_t.
> > branch8(LessThan, addr, 0) =>
> > 
> > load8(addr) => 32bit 255 is sotred in the register
> > 255 < 0 (IN 32bit context).
> > 
> > => false. But it should return true.
> > 
> > If we would like to make it work, we should select load8 /
> > load8SignedExtendTo32 based on the condition.
> 
> And also, we should select the way to mask the 32bit value.

Yes, exactly.

[Yusuke Suzuki]

Created attachment 293984 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 293984 [details]
Patch

WIP

[Yusuke Suzuki]

OK, I've ensured that this fixes https://bugs.webkit.org/show_bug.cgi?id=164004 problem.

[Yusuke Suzuki]

Created attachment 294018 [details]
Patch

[Yusuke Suzuki]

Created attachment 294072 [details]
Patch

[Yusuke Suzuki]

And we would like to introduce some testing mechanism like this in the separate patch. https://bugs.webkit.org/show_bug.cgi?id=164487

[Michael Saboff]

Comment on attachment 294072 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        Except for X86, our supported MacroAssemblers do not have the native 8bit instructions.

Remove "the".

> Source/JavaScriptCore/ChangeLog:20
> +        The problem is that we exclusively use zero-extended load instruction (load8). Even if
> +        RelationCondition is signedness aware one, we do not perform signed extension.

How about changing "Even if RelationCondition is signedness aware one" to "Even for signed RelationConditions."?  Change "signed extension" to "sign extension".

> Source/JavaScriptCore/ChangeLog:21
> +        It makes signedness aware operations with negative numbers incorrect!

Change "signedness aware operations" to "signed operations".

> Source/JavaScriptCore/ChangeLog:23
> +        into 32bit register. On the other hand, |right| will be signed extended. If you pass 0

Use "sign extended".

> Source/JavaScriptCore/ChangeLog:33
> +        This problem does not matter since it does not perform extension.

You probably can remove this sentence if you insert "signed" between "native" and "8bit" in the prior sentence.

[Yusuke Suzuki]

Comment on attachment 294072 [details]
Patch

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

Thanks! I'll land it once EWS is back.

>> Source/JavaScriptCore/ChangeLog:8
>> +        Except for X86, our supported MacroAssemblers do not have the native 8bit instructions.
> 
> Remove "the".

Dropped.

>> Source/JavaScriptCore/ChangeLog:20
>> +        RelationCondition is signedness aware one, we do not perform signed extension.
> 
> How about changing "Even if RelationCondition is signedness aware one" to "Even for signed RelationConditions."?  Change "signed extension" to "sign extension".

Sounds nice. Fixed.

>> Source/JavaScriptCore/ChangeLog:21
>> +        It makes signedness aware operations with negative numbers incorrect!
> 
> Change "signedness aware operations" to "signed operations".

Fixed.

>> Source/JavaScriptCore/ChangeLog:23
>> +        into 32bit register. On the other hand, |right| will be signed extended. If you pass 0
> 
> Use "sign extended".

Fixed.

>> Source/JavaScriptCore/ChangeLog:33
>> +        This problem does not matter since it does not perform extension.
> 
> You probably can remove this sentence if you insert "signed" between "native" and "8bit" in the prior sentence.

OK, insert "signed" between "native" and "8bit". And drop this sentence.

[Yusuke Suzuki]

Created attachment 294089 [details]
Patch for landing

[Yusuke Suzuki]

Committed r208450: <http://trac.webkit.org/changeset/208450>