Summary: | [JSC] The implementation of 8 bit operation in MacroAssembler should care about uint8_t / int8_t | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | fpizlo, ggaren, jfbastien, joepeck, keith_miller, mark.lam, msaboff, ossy, saam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 164004, 164487 | ||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2016-11-04 14:11:29 PDT
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> |