Summary: | Fix code origin when lowering offlineasm instructions on MIPS/ARM64E | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Angelos Oikonomopoulos <angelos> | ||||||||||||||||
Component: | New Bugs | Assignee: | Angelos Oikonomopoulos <angelos> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | clopez, ddkilzer, ews-watchlist, guijemont, keith_miller, mark.lam, msaboff, pmatos, saam, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 210639 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Angelos Oikonomopoulos
2020-02-06 06:25:44 PST
Created attachment 389946 [details]
Patch
Created attachment 389947 [details]
Patch
Created attachment 389948 [details]
Patch
Created attachment 394368 [details]
Patch
Rebased and re-tested, no functional changes. Created attachment 395933 [details]
Patch
Comment on attachment 395933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395933&action=review The patch login generally looks good but the names don't describe what is actually being done. I suggested replacement names. r- for now because I don't think this patch is ready for landing. > Source/JavaScriptCore/offlineasm/arm64e.rb:57 > + newList << Instruction.duplicate_with_operands(operands) 1. Let's use camel case for the function name because we've been using camel case everywhere else for functions that we define. 2. I suggest calling this cloneWithNewOperands() instead. I think this is a better name because it tells us that we're effectively cloning the instruction, except that we're giving it new operands. "duplicate" implies that we're keeping the original. We're not. We'll be replacing it: returning wasHandled true tells the client that it doesn't need to process this instruction and can skip it, where "processing" this instruction may mean just adding it to the newList as is. > Source/JavaScriptCore/offlineasm/ast.rb:921 > + def duplicate_with_operands(operands) Let's call this cloneWithNewOperands(), and let's rename the argument to newOperands. > Source/JavaScriptCore/offlineasm/mips.rb:560 > + def mipsAsRegister(preList, postList, operandIndex, needRestore) How about calling this mipsCloneWithOperandLowered()? > Source/JavaScriptCore/offlineasm/risc.rb:431 > +def riscOperandAsRegister(insn, preList, postList, operandIndex, suffix, needStore) The old riscAsRegister() name wasn't a very good name to begin with. The new one isn't better. Let's call it riscLowerOperandToRegister() instead. > Source/JavaScriptCore/offlineasm/risc.rb:443 > +def riscOperandsAsRegisters(insn, preList, postList, suffix) Let's call this riscLowerOperandsToRegisters(). > Source/JavaScriptCore/offlineasm/risc.rb:455 > + def riscAsRegister(preList, postList, operandIndex, suffix, needStore) Let's call this riscCloneWithOperandLowered() instead. > Source/JavaScriptCore/offlineasm/risc.rb:461 > + def riscAsRegisters(preList, postList, suffix) Let's call this riscCloneWithOperandsLowered() instead. > Source/JavaScriptCore/offlineasm/risc.rb:483 > + newList << node.riscAsRegisters(newList, postInstructions, "i") riscAsRegisters() is a bad name here. This operation is actually doing 2 things: (1) cloning the instruction, and (2) lowering the operands to registers based on the suffix. How about something like riscCloneWithOperandsLowered()? > Source/JavaScriptCore/offlineasm/risc.rb:498 > + newList << node.riscAsRegister(newList, postInstructions, 0, "p", false) riscAsRegister() is also a bad name here. The operation is actually (1) cloning the instruction with (2) the operand lowered to a register. How about calling this riscCloneWithOperandLowered()? Created attachment 396753 [details]
Patch
Lastest patch implements the suggested naming changes. Thanks! Comment on attachment 396753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396753&action=review r=me with fixes. > Source/JavaScriptCore/offlineasm/arm64e.rb:48 > + tag = riscLowerOperandToRegister(node, newList, postInstructions, > + 1, "p", false) Please fix alignment or preferably, put on 1 line. > Source/JavaScriptCore/offlineasm/risc.rb:449 > + newOperands << riscLowerOperandToRegister(insn, preList, postList, > + index, suffix, > + index == insn.operands.size - 1) This alignment is off. Please fix the alignment, or preferably, just put this on 1 line. > Source/JavaScriptCore/offlineasm/risc.rb:458 > + operand = riscLowerOperandToRegister(self, preList, postList, > + operandIndex, suffix, > + needStore) This alignment is off. Please fix the alignment, or preferably, just put this on 1 line. Just a reminder that https://bugs.webkit.org/show_bug.cgi?id=210639 logically conflicts with this patch. The changes may merge cleanly, but it will be broken unless fixed. This patch will have to be updated because there's a new use of riscAsRegisters() added in 210639. Created attachment 396846 [details]
Patch
(In reply to Mark Lam from comment #11) > Just a reminder that https://bugs.webkit.org/show_bug.cgi?id=210639 > logically conflicts with this patch. The changes may merge cleanly, but it > will be broken unless fixed. This patch will have to be updated because > there's a new use of riscAsRegisters() added in 210639. Thanks. Applied the whitespace changes and merged it with 210639. Comment on attachment 396846 [details]
Patch
r=me
Committed r260310: <https://trac.webkit.org/changeset/260310> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396846 [details]. This commit appears to have broken some of Apple's internal bots building for iOS 13.4: /bin/sh -c JavaScriptCore.roots/BuildRecords/JavaScriptCore_install/TempContent/Objects/JavaScriptCore.build/Offline\\\ Assembler.build/Script-65788AA018B409EB00C189FF.sh JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/arm64e.rb:56:in `lowerMisplacedAddressesARM64E': undefined method `cloneWithNewOperands' for Instruction:Class (NoMethodError) from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/risc.rb:470:in `block in riscLowerMisplacedAddresses' from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/risc.rb:466:in `each' from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/risc.rb:466:in `riscLowerMisplacedAddresses' from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/arm64e.rb:31:in `getModifiedListARM64E' from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/backends.rb:189:in `lower' from JavaScriptCore/offlineasm/asm.rb:406:in `block (5 levels) in <main>' from JavaScriptCore/offlineasm/asm.rb:107:in `inAsm' from JavaScriptCore/offlineasm/asm.rb:405:in `block (4 levels) in <main>' from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/settings.rb:210:in `emitCodeInConfiguration' from JavaScriptCore/offlineasm/asm.rb:403:in `block (3 levels) in <main>' from JavaScriptCore.roots/Sources/JavaScriptCore/offlineasm/settings.rb:107:in `forSettings' from JavaScriptCore/offlineasm/asm.rb:390:in `block (2 levels) in <main>' from JavaScriptCore/offlineasm/asm.rb:386:in `each' from JavaScriptCore/offlineasm/asm.rb:386:in `block in <main>' from JavaScriptCore/offlineasm/asm.rb:376:in `open' from JavaScriptCore/offlineasm/asm.rb:376:in `<main>' Command PhaseScriptExecution failed with a nonzero exit code (Apologies for the truncated paths; was necessary to sanitize the output.) Committed r260314: <https://trac.webkit.org/changeset/260314> |