144680 2015-05-06 03:05:13 -0700 [JSC] Erratum (843419) in Cortex-A53 2015-05-12 01:17:01 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 108645 1 loki loki commit-queue msaboff ossy oldest_to_newest 1092123 0 loki 2015-05-06 03:05:13 -0700 If there is a load or store instruction at a page boundary which uses the result of an ADRP instruction as a base it might access an incorrect address. There is a second variant of this if the base register is written by an instruction immediately after an ADRP to the same register, might also cause incorrect address access. 1092124 1 loki 2015-05-06 03:06:39 -0700 Although the adrp instruction is never used from ARM64Assembler.h, it is possible to add a simple workaround for this. I will upload a patch soon. 1092125 2 252464 loki 2015-05-06 03:12:59 -0700 Created attachment 252464 Patch 1092789 3 252464 msaboff 2015-05-07 15:24:00 -0700 Comment on attachment 252464 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252464&action=review > Source/JavaScriptCore/assembler/ARM64Assembler.h:3668 > + nop(); Why this third nop? I thought that the bug was an issue only for a dependent instruction at the page boundary. The check looks for the page boundary and inserts two nop's that should take care of the problem, making the last nop unnecessary. > Source/JavaScriptCore/assembler/ARM64Assembler.h:3670 > + ALWAYS_INLINE void nopCortexA53Fix843419() > + { > +#if CPU(ARM64_CORTEXA53) > + if (UNLIKELY((m_buffer.codeSize() & 0xFF8) == 0xFF8)) { > + nop(); > + nop(); > + } > + nop(); > +#endif > + } This nop padding needs to happen at link time. I don't think we have any guarantees that the page offset at assemble time will be the same after we link. Even if we did, there is some code compaction that can happen at link time, primarily due to branch type specialization. 1093120 4 loki 2015-05-08 01:49:15 -0700 There are two cases: 1) If the ADRP is near to the page boundary (last two instruction) you will need more than two instructions between the ADRP and the load or store. 2) If the ADRP is followed by an instruction which writes the result register you will need an addition register. So, there is no unnecessary nops. > This nop padding needs to happen at link time. I don't think we have any guarantees that the page offset at assemble time will be the same after we link. > Even if we did, there is some code compaction that can happen at link time, primarily due to branch type specialization. Sad to hear that. It was a long time when I last touched the JSC. I am sure that the appropriate fix will be to check all the conditions which cause the issue, but it surely slow down the code generation - because the load and store instructions are very frequent. If we do these checks as a later pass after code generation (link time), the effect to the performance will be the same. I think the most easiest way to fix this - which might not influence the performance much - is to generate three nops after ADRP. How does it sound? 1093744 5 252858 loki 2015-05-11 06:22:55 -0700 Created attachment 252858 Patch 1093748 6 msaboff 2015-05-11 07:08:40 -0700 (In reply to comment #4) > There are two cases: > 1) If the ADRP is near to the page boundary (last two instruction) you will > need more than two instructions between the ADRP and the load or store. > 2) If the ADRP is followed by an instruction which writes the result > register you will need an addition register. > > So, there is no unnecessary nops. > > > This nop padding needs to happen at link time. I don't think we have any guarantees that the page offset at assemble time will be the same after we link. > > Even if we did, there is some code compaction that can happen at link time, primarily due to branch type specialization. > > Sad to hear that. It was a long time when I last touched the JSC. > > I am sure that the appropriate fix will be to check all the conditions which > cause the issue, but it surely slow down the code generation - because the > load and store instructions are very frequent. If we do these checks as a > later pass after code generation (link time), the effect to the performance > will be the same. > > I think the most easiest way to fix this - which might not influence the > performance much - is to generate three nops after ADRP. > > How does it sound? Sounds fine. As a heads up, there has been some discussion to change the current movz/movk/movk absolute address materialization to adrp/add. 1093749 7 252858 msaboff 2015-05-11 07:09:02 -0700 Comment on attachment 252858 Patch r=me 1094112 8 252858 commit-queue 2015-05-12 01:16:54 -0700 Comment on attachment 252858 Patch Clearing flags on attachment: 252858 Committed r184170: <http://trac.webkit.org/changeset/184170> 1094113 9 commit-queue 2015-05-12 01:17:01 -0700 All reviewed patches have been landed. Closing bug. 252464 2015-05-06 03:12:59 -0700 2015-05-11 06:22:50 -0700 Patch bug-144680-20150506121141.patch text/plain 1990 loki U3VidmVyc2lvbiBSZXZpc2lvbjogMTgzODY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAz MTk3NDgyNjgzZDNkMDg5NjE0NjhkZjVmZTk1ZTc0YzBhMjIxZWE5Li44OTAyNzU3YzUzNWE3MDdh OTZhYTI1MzJjOGJmZDJkM2M1N2EzZTk2IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxNS0wNS0wNiAgR2Fib3IgTG9raSAgPGxva2lAd2Via2l0Lm9yZz4KKwor ICAgICAgICBXb3JrYXJvdW5kIGZvciBDb3J0ZXgtQTUzIGVycmF0dW0gODQzNDE5CisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDQ2ODAKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIHBhdGNoIGlzIGFi b3V0IHRvIGdpdmUgc2ltcGxlIHdvcmthcm91bmQgZm9yIENvcnRleC1BNTMgZXJyYXR1bSA4NDM0 MTkuCisgICAgICAgIEl0IGluc2VydHMgbm9wcyBhZnRlciBBRFJQIGluc3RydWN0aW9uIHRvIGF2 b2lkIHdyb25nIGFkZHJlc3MgYWNjZXNzZXMuCisKKyAgICAgICAgKiBhc3NlbWJsZXIvQVJNNjRB c3NlbWJsZXIuaDoKKyAgICAgICAgKEpTQzo6QVJNNjRBc3NlbWJsZXI6OmFkcnApOgorICAgICAg ICAoSlNDOjpBUk02NEFzc2VtYmxlcjo6bm9wQ29ydGV4QTUzRml4ODQzNDE5KToKKwogMjAxNS0w NS0wNSAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAgICAgIFB1dEdsb2Jh bFZhciBzaG91bGRuJ3QgaGF2ZSBhbiB1bmNvbmRpdGlvbmFsIHN0b3JlIGJhcnJpZXIKZGlmZiAt LWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvQVJNNjRBc3NlbWJsZXIuaCBi L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvQVJNNjRBc3NlbWJsZXIuaAppbmRleCBi N2Q0OWE2MzAxZGRiMmE0Mjg1NDMwNjhiNjU5ZGQ5MDNjNDRmOWQyLi4yMTQ5ZGFmNGFkNDVhYzkw OGI4MjA5YTc2ZDhmYzNkMzI3M2VhZDM2IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvYXNzZW1ibGVyL0FSTTY0QXNzZW1ibGVyLmgKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3Jl L2Fzc2VtYmxlci9BUk02NEFzc2VtYmxlci5oCkBAIC05NDcsNiArOTQ3LDcgQEAgcHVibGljOgog ICAgIHsKICAgICAgICAgQVNTRVJUKCEob2Zmc2V0ICYgMHhmZmYpKTsKICAgICAgICAgaW5zbihw Y1JlbGF0aXZlKHRydWUsIG9mZnNldCA+PiAxMiwgcmQpKTsKKyAgICAgICAgbm9wQ29ydGV4QTUz Rml4ODQzNDE5KCk7CiAgICAgfQogCiAgICAgdGVtcGxhdGU8aW50IGRhdGFzaXplLCBTZXRGbGFn cyBzZXRGbGFncyA9IERvbnRTZXRGbGFncz4KQEAgLTM2NTUsNiArMzY1NiwxOSBAQCBwcml2YXRl OgogI2VuZGlmCiAgICAgfQogCisgICAgLy8gV29ya2Fyb3VuZCBmb3IgQ29ydGV4LUE1MyBlcnJh dHVtICg4NDM0MTkpLiBFbWl0IGV4dHJhIG5vcHMgdG8gYXZvaWQKKyAgICAvLyB3cm9uZyBhZGRy ZXNzIGFjY2VzcyBhZnRlciBBRFJQIGluc3RydWN0aW9uLgorICAgIEFMV0FZU19JTkxJTkUgdm9p ZCBub3BDb3J0ZXhBNTNGaXg4NDM0MTkoKQorICAgIHsKKyNpZiBDUFUoQVJNNjRfQ09SVEVYQTUz KQorICAgICAgICBpZiAoVU5MSUtFTFkoKG1fYnVmZmVyLmNvZGVTaXplKCkgJiAweEZGOCkgPT0g MHhGRjgpKSB7CisgICAgICAgICAgICBub3AoKTsKKyAgICAgICAgICAgIG5vcCgpOworICAgICAg ICB9CisgICAgICAgIG5vcCgpOworI2VuZGlmCisgICAgfQorCiAgICAgQXNzZW1ibGVyQnVmZmVy IG1fYnVmZmVyOwogICAgIFZlY3RvcjxMaW5rUmVjb3JkLCAwLCBVbnNhZmVWZWN0b3JPdmVyZmxv dz4gbV9qdW1wc1RvTGluazsKICAgICBpbnQgbV9pbmRleE9mTGFzdFdhdGNocG9pbnQ7Cg== 252858 2015-05-11 06:22:55 -0700 2015-05-12 01:16:54 -0700 Patch bug-144680-20150511152131.patch text/plain 1906 loki U3VidmVyc2lvbiBSZXZpc2lvbjogMTgzODY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAz MTk3NDgyNjgzZDNkMDg5NjE0NjhkZjVmZTk1ZTc0YzBhMjIxZWE5Li4zOGY1YTg4YTdlYzI1Yzcy MTJiZGJjNjZmM2U3ZDZkODdjMDQyMTFiIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxNS0wNS0xMSAgR2Fib3IgTG9raSAgPGxva2lAd2Via2l0Lm9yZz4KKwor ICAgICAgICBXb3JrYXJvdW5kIGZvciBDb3J0ZXgtQTUzIGVycmF0dW0gODQzNDE5CisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDQ2ODAKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIHBhdGNoIGlzIGFi b3V0IHRvIGdpdmUgc2ltcGxlIHdvcmthcm91bmQgZm9yIENvcnRleC1BNTMgZXJyYXR1bSA4NDM0 MTkuCisgICAgICAgIEl0IGluc2VydHMgbm9wcyBhZnRlciBBRFJQIGluc3RydWN0aW9uIHRvIGF2 b2lkIHdyb25nIGFkZHJlc3MgYWNjZXNzZXMuCisKKyAgICAgICAgKiBhc3NlbWJsZXIvQVJNNjRB c3NlbWJsZXIuaDoKKyAgICAgICAgKEpTQzo6QVJNNjRBc3NlbWJsZXI6OmFkcnApOgorICAgICAg ICAoSlNDOjpBUk02NEFzc2VtYmxlcjo6bm9wQ29ydGV4QTUzRml4ODQzNDE5KToKKwogMjAxNS0w NS0wNSAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAgICAgIFB1dEdsb2Jh bFZhciBzaG91bGRuJ3QgaGF2ZSBhbiB1bmNvbmRpdGlvbmFsIHN0b3JlIGJhcnJpZXIKZGlmZiAt LWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvQVJNNjRBc3NlbWJsZXIuaCBi L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvQVJNNjRBc3NlbWJsZXIuaAppbmRleCBi N2Q0OWE2MzAxZGRiMmE0Mjg1NDMwNjhiNjU5ZGQ5MDNjNDRmOWQyLi42OGYzZWQwMzQyZGJjYmU1 YThkNzU1NDZmODNmY2Y1MDVhMTZiOTg4IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvYXNzZW1ibGVyL0FSTTY0QXNzZW1ibGVyLmgKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3Jl L2Fzc2VtYmxlci9BUk02NEFzc2VtYmxlci5oCkBAIC05NDcsNiArOTQ3LDcgQEAgcHVibGljOgog ICAgIHsKICAgICAgICAgQVNTRVJUKCEob2Zmc2V0ICYgMHhmZmYpKTsKICAgICAgICAgaW5zbihw Y1JlbGF0aXZlKHRydWUsIG9mZnNldCA+PiAxMiwgcmQpKTsKKyAgICAgICAgbm9wQ29ydGV4QTUz Rml4ODQzNDE5KCk7CiAgICAgfQogCiAgICAgdGVtcGxhdGU8aW50IGRhdGFzaXplLCBTZXRGbGFn cyBzZXRGbGFncyA9IERvbnRTZXRGbGFncz4KQEAgLTM2NTUsNiArMzY1NiwxNyBAQCBwcml2YXRl OgogI2VuZGlmCiAgICAgfQogCisgICAgLy8gV29ya2Fyb3VuZCBmb3IgQ29ydGV4LUE1MyBlcnJh dHVtICg4NDM0MTkpLiBFbWl0IGV4dHJhIG5vcHMgdG8gYXZvaWQKKyAgICAvLyB3cm9uZyBhZGRy ZXNzIGFjY2VzcyBhZnRlciBBRFJQIGluc3RydWN0aW9uLgorICAgIEFMV0FZU19JTkxJTkUgdm9p ZCBub3BDb3J0ZXhBNTNGaXg4NDM0MTkoKQorICAgIHsKKyNpZiBDUFUoQVJNNjRfQ09SVEVYQTUz KQorICAgICAgICBub3AoKTsKKyAgICAgICAgbm9wKCk7CisgICAgICAgIG5vcCgpOworI2VuZGlm CisgICAgfQorCiAgICAgQXNzZW1ibGVyQnVmZmVyIG1fYnVmZmVyOwogICAgIFZlY3RvcjxMaW5r UmVjb3JkLCAwLCBVbnNhZmVWZWN0b3JPdmVyZmxvdz4gbV9qdW1wc1RvTGluazsKICAgICBpbnQg bV9pbmRleE9mTGFzdFdhdGNocG9pbnQ7Cg==