152784 2016-01-06 03:31:31 -0800 Add new or32 implementation to MacroAssemblerARM after r194613 2016-01-07 07:31:51 -0800 1 1 1 Unclassified WebKit JavaScriptCore Other Unspecified Unspecified RESOLVED FIXED P2 Normal --- 108645 132470 1 ossy ossy benjamin commit-queue ggaren keith_miller mark.lam msaboff ossy saam oldest_to_newest 1153020 0 ossy 2016-01-06 03:31:31 -0800 https://trac.webkit.org/changeset/194613 added the new or32 to ARMv7(Thumb2) and AArch64 assemblers, but not to ARMv7(ARM). 1153022 1 268365 ossy 2016-01-06 03:33:53 -0800 Created attachment 268365 Patch 1153026 2 268365 benjamin 2016-01-06 04:18:24 -0800 Comment on attachment 268365 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=268365&action=review > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:231 > + void or32(TrustedImm32 imm, AbsoluteAddress dest) Uh, that sucks :( > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:235 > + or32(imm, ARMRegisters::S1); I don't think that's fine. It will override S0 to load imm into register. 1153029 3 ossy 2016-01-06 04:44:52 -0800 (In reply to comment #2) > Comment on attachment 268365 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268365&action=review > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:231 > > + void or32(TrustedImm32 imm, AbsoluteAddress dest) > > Uh, that sucks :( > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:235 > > + or32(imm, ARMRegisters::S1); > > I don't think that's fine. It will override S0 to load imm into register. It is a copy/paste of the ARMv7(Thumb2) change: https://trac.webkit.org/changeset/194613/trunk/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h And JSC stress tests run without regression. 1153030 4 benjamin 2016-01-06 04:57:18 -0800 (In reply to comment #3) > > I don't think that's fine. It will override S0 to load imm into register. > > It is a copy/paste of the ARMv7(Thumb2) change: > https://trac.webkit.org/changeset/194613/trunk/Source/JavaScriptCore/ > assembler/MacroAssemblerARMv7.h The thumb version of or32() uses the "dataTempRegister" to load the immediate. It does not override the address. The ARM version you propose override the address. > And JSC stress tests run without regression. Looks like the test coverage is not good enough then. Please add a test. 1153389 5 268449 ossy 2016-01-07 03:20:20 -0800 Created attachment 268449 Patch Thanks for catching this bug around or32(imm,reg). I think the easiest fix is reloading the address to S0 after or32(imm,reg) overwrote it. Let me know if you have better idea to fix this issue. 1153390 6 ossy 2016-01-07 03:22:27 -0800 (In reply to comment #4) > (In reply to comment #3) > > And JSC stress tests run without regression. > Looks like the test coverage is not good enough then. Please add a test. It's not trivial at all to write a test in JS to trigger a behaviour in macroassembler and there is no test framework for macroassembler, so I don't think if it is a fair request. 1153394 7 benjamin 2016-01-07 04:51:28 -0800 (In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #3) > > > > And JSC stress tests run without regression. > > Looks like the test coverage is not good enough then. Please add a test. > > It's not trivial at all to write a test in JS to trigger a behaviour > in macroassembler and there is no test framework for macroassembler, > so I don't think if it is a fair request. You would need a stress-test that exercices the baseline-JIT/DFG part that uses this form of or32. I understand you don't want to do that. (In reply to comment #5) > Created attachment 268449 [details] > Patch > > Thanks for catching this bug around or32(imm,reg). I think the easiest fix > is reloading the address to S0 after or32(imm,reg) overwrote it. Let me know > if you have better idea to fix this issue. IMHO, that's perfectly reasonable. 1153408 8 268449 ossy 2016-01-07 07:31:42 -0800 Comment on attachment 268449 Patch Clearing flags on attachment: 268449 Committed r194698: <http://trac.webkit.org/changeset/194698> 1153409 9 ossy 2016-01-07 07:31:51 -0800 All reviewed patches have been landed. Closing bug. 268365 2016-01-06 03:33:53 -0800 2016-01-06 05:16:53 -0800 Patch bug-152784-20160106033319.patch text/plain 1570 ossy U3VidmVyc2lvbiBSZXZpc2lvbjogMTk0NjM1CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBi MmI2MzYwMTU4MDc4ODAzNDNjZGZjMGQwYzdhY2Q5NGNlZTcxMTc1Li45YWE1YzA1NWVlYTAwMTAz MmI3M2I1MjQwZDU3NGE5MzQ0ZGZiN2QzIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAxNi0wMS0wNiAgQ3NhYmEgT3N6dHJvZ29uw6FjICA8b3NzeUB3ZWJraXQu b3JnPgorCisgICAgICAgIEFkZCBuZXcgb3IzMiBpbXBsZW1lbnRhdGlvbiB0byBNYWNyb0Fzc2Vt YmxlckFSTSBhZnRlciByMTk0NjEzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0xNTI3ODQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICAqIGFzc2VtYmxlci9NYWNyb0Fzc2VtYmxlckFSTS5oOgorICAgICAgICAo SlNDOjpNYWNyb0Fzc2VtYmxlckFSTTo6b3IzMik6IEFkZGVkLgorCiAyMDE2LTAxLTA2ICBCZW5q YW1pbiBQb3VsYWluICA8YnBvdWxhaW5AYXBwbGUuY29tPgogCiAgICAgICAgIFtKU0NdIEdldCBt b3JlIG9mIHRlc3RiMyB0byBwYXNzIG9uIEFSTTY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNLmggYi9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNLmgKaW5kZXggZTU5YjEyZThlNDM5ZWE0 YjM5MDM2MjZiZDczNjA2YTBiZGZkNDZmMy4uOTliZWNjNWMxN2FkZDBhYzBmNGRlNTQyNjNiZWNm MzBiYmNhNzg0NiAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9N YWNyb0Fzc2VtYmxlckFSTS5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIv TWFjcm9Bc3NlbWJsZXJBUk0uaApAQCAtMjI4LDYgKzIyOCwxNCBAQCBwdWJsaWM6CiAgICAgICAg IHN0b3JlMzIoQVJNUmVnaXN0ZXJzOjpTMSwgQVJNUmVnaXN0ZXJzOjpTMCk7CiAgICAgfQogCisg ICAgdm9pZCBvcjMyKFRydXN0ZWRJbW0zMiBpbW0sIEFic29sdXRlQWRkcmVzcyBkZXN0KQorICAg IHsKKyAgICAgICAgbW92ZShUcnVzdGVkSW1tUHRyKGRlc3QubV9wdHIpLCBBUk1SZWdpc3RlcnM6 OlMwKTsKKyAgICAgICAgbG9hZDMyKEFkZHJlc3MoQVJNUmVnaXN0ZXJzOjpTMCksIEFSTVJlZ2lz dGVyczo6UzEpOworICAgICAgICBvcjMyKGltbSwgQVJNUmVnaXN0ZXJzOjpTMSk7CisgICAgICAg IHN0b3JlMzIoQVJNUmVnaXN0ZXJzOjpTMSwgQVJNUmVnaXN0ZXJzOjpTMCk7CisgICAgfQorCiAg ICAgdm9pZCBvcjMyKFRydXN0ZWRJbW0zMiBpbW0sIFJlZ2lzdGVySUQgZGVzdCkKICAgICB7CiAg ICAgICAgIG1fYXNzZW1ibGVyLm9ycnMoZGVzdCwgZGVzdCwgbV9hc3NlbWJsZXIuZ2V0SW1tKGlt bS5tX3ZhbHVlLCBBUk1SZWdpc3RlcnM6OlMwKSk7Cg== 268449 2016-01-07 03:20:20 -0800 2016-01-07 07:31:42 -0800 Patch bug-152784-20160107031947.patch text/plain 1713 ossy U3VidmVyc2lvbiBSZXZpc2lvbjogMTk0NjkxCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBj MjczZTk4ZWFjNGNiOWE3MDk1Y2MzZTRmNzBmZGQ3NmQzMWQ3ODEzLi4yNDM1MjhmMjQ0NmIxZjY4 NWU1NGI0YTM5ZTQ5MGJiYjUyNDdhYjJjIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAxNi0wMS0wNyAgQ3NhYmEgT3N6dHJvZ29uw6FjICA8b3NzeUB3ZWJraXQu b3JnPgorCisgICAgICAgIEFkZCBuZXcgb3IzMiBpbXBsZW1lbnRhdGlvbiB0byBNYWNyb0Fzc2Vt YmxlckFSTSBhZnRlciByMTk0NjEzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0xNTI3ODQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICAqIGFzc2VtYmxlci9NYWNyb0Fzc2VtYmxlckFSTS5oOgorICAgICAgICAo SlNDOjpNYWNyb0Fzc2VtYmxlckFSTTo6b3IzMik6CisKIDIwMTYtMDEtMDYgIE1hcmsgTGFtICA8 bWFyay5sYW1AYXBwbGUuY29tPgogCiAgICAgICAgIFJFR1JFU1NJT04ocjE5NDYxMyk6IEpJVE11 bEdlbmVyYXRvciBuZWVkcyBhIHNjcmF0Y2ggR1BSIG9uIDMyLWJpdCB0b28uCmRpZmYgLS1naXQg YS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNLmggYi9T b3VyY2UvSmF2YVNjcmlwdENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNLmgKaW5kZXgg ZTU5YjEyZThlNDM5ZWE0YjM5MDM2MjZiZDczNjA2YTBiZGZkNDZmMy4uOTBjOTA2NmY2ZTUwMzQ3 NWExMWVlMTQxYWYzMjE3MTQ1YzNiMGE2MSAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRD b3JlL2Fzc2VtYmxlci9NYWNyb0Fzc2VtYmxlckFSTS5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJBUk0uaApAQCAtMjI4LDYgKzIyOCwxNSBAQCBw dWJsaWM6CiAgICAgICAgIHN0b3JlMzIoQVJNUmVnaXN0ZXJzOjpTMSwgQVJNUmVnaXN0ZXJzOjpT MCk7CiAgICAgfQogCisgICAgdm9pZCBvcjMyKFRydXN0ZWRJbW0zMiBpbW0sIEFic29sdXRlQWRk cmVzcyBkZXN0KQorICAgIHsKKyAgICAgICAgbW92ZShUcnVzdGVkSW1tUHRyKGRlc3QubV9wdHIp LCBBUk1SZWdpc3RlcnM6OlMwKTsKKyAgICAgICAgbG9hZDMyKEFkZHJlc3MoQVJNUmVnaXN0ZXJz OjpTMCksIEFSTVJlZ2lzdGVyczo6UzEpOworICAgICAgICBvcjMyKGltbSwgQVJNUmVnaXN0ZXJz OjpTMSk7IC8vIEl0IHVzZXMgUzAgYXMgdGVtcG9yYXJ5IHJlZ2lzdGVyLCB3ZSBuZWVkIHRvIHJl bG9hZCB0aGUgYWRkcmVzcy4KKyAgICAgICAgbW92ZShUcnVzdGVkSW1tUHRyKGRlc3QubV9wdHIp LCBBUk1SZWdpc3RlcnM6OlMwKTsKKyAgICAgICAgc3RvcmUzMihBUk1SZWdpc3RlcnM6OlMxLCBB Uk1SZWdpc3RlcnM6OlMwKTsKKyAgICB9CisKICAgICB2b2lkIG9yMzIoVHJ1c3RlZEltbTMyIGlt bSwgUmVnaXN0ZXJJRCBkZXN0KQogICAgIHsKICAgICAgICAgbV9hc3NlbWJsZXIub3JycyhkZXN0 LCBkZXN0LCBtX2Fzc2VtYmxlci5nZXRJbW0oaW1tLm1fdmFsdWUsIEFSTVJlZ2lzdGVyczo6UzAp KTsK