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.
https://trac.webkit.org/changeset/203331
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.
This issue actually causes https://bugs.webkit.org/show_bug.cgi?id=164004's crash. Since now JSType(8bit) may have the highest bit.
(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.
(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.
Created attachment 293984 [details] Patch
Comment on attachment 293984 [details] Patch WIP
OK, I've ensured that this fixes https://bugs.webkit.org/show_bug.cgi?id=164004 problem.
Created attachment 294018 [details] Patch
Created attachment 294072 [details] Patch
And we would like to introduce some testing mechanism like this in the separate patch. https://bugs.webkit.org/show_bug.cgi?id=164487
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.
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.
Created attachment 294089 [details] Patch for landing
Committed r208450: <http://trac.webkit.org/changeset/208450>