45669 2010-09-13 07:20:33 -0700 ARMv7Assembler generates illegal instructions with RVCT 2011-01-24 07:57:33 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Other Other RESOLVED DUPLICATE 47356 P2 Normal --- 0 dave+webkit webkit-unassigned loki oliver staikos oldest_to_newest 278192 0 dave+webkit 2010-09-13 07:20:33 -0700 Using RVCT to compile ARMv7Assembler illegal instructions occur because the ShiftTypeAmount class does not zero initialize all bits in the union. The type and amount values take up 7 bits leaving an extra unassigned bit in the union. GCC zero initializes bitfields but RVCT does not. 278200 1 67406 dave+webkit 2010-09-13 07:29:03 -0700 Created attachment 67406 Add an asInt member and ZI it 278252 2 67406 kling 2010-09-13 09:36:42 -0700 Comment on attachment 67406 Add an asInt member and ZI it > + int16_t asInt; Shouldn't this be an 8-bit integer? 278319 3 67435 dave+webkit 2010-09-13 11:14:16 -0700 Created attachment 67435 Add an asInt member and ZI it 278358 4 oliver 2010-09-13 11:52:55 -0700 Sigh, i really wish people would stop using RCVT it's clearly a broken compiler based on the amount of workarounds we've had to do over time. That said i fail to see how this patch will zero-init the entire union 278376 5 loki 2010-09-13 12:10:20 -0700 I will keep a check on this tomorrow. Btw, David, which version of RVCT did you use? 278397 6 dave+webkit 2010-09-13 12:26:14 -0700 (In reply to comment #5) > I will keep a check on this tomorrow. > Btw, David, which version of RVCT did you use? $ armcc ARM C/C++ Compiler, RVCT4.0 [Build 771] 278399 7 dave+webkit 2010-09-13 12:28:52 -0700 (In reply to comment #4) > Sigh, i really wish people would stop using RCVT it's clearly a broken compiler based on the amount of workarounds we've had to do over time. > > That said i fail to see how this patch will zero-init the entire union The ctors were changed to zero the asInt value. (This follows similar convention used in this header file for other structs with bitfields) The storage on the asInt should take up 8 bits which corresponds to the 4 bits on lo4 and 4 bits on hi4. The problem is with the code that the type and amount only take up 7 bits. 278768 8 loki 2010-09-14 01:24:30 -0700 > The ctors were changed to zero the asInt value. (This follows similar convention used in this header file for other structs with bitfields) The storage on the asInt should take up 8 bits which corresponds to the 4 bits on lo4 and 4 bits on hi4. The problem is with the code that the type and amount only take up 7 bits. Although I did not find any initialization problem in a small example (very similar to ShiftTypeAndAmount with the very same compiler), I feel this 4/4 and 2/5 bits difference could lead to a bug. So, it should be fixed. I have one comment to your patch. It is really looks like a workaround. I suggest you try to fix it in less hacky way. For example: - unsigned hi4() { return m_u.s1.hi4; } + unsigned hi4() { return m_u.s1.hi4 & 0x7; } or - unsigned hi4 : 4; + unsigned hi4 : 3; 278852 9 dave+webkit 2010-09-14 06:24:57 -0700 (In reply to comment #8) > > The ctors were changed to zero the asInt value. (This follows similar convention used in this header file for other structs with bitfields) The storage on the asInt should take up 8 bits which corresponds to the 4 bits on lo4 and 4 bits on hi4. The problem is with the code that the type and amount only take up 7 bits. > > Although I did not find any initialization problem in a small example (very similar to ShiftTypeAndAmount with the very same compiler), I feel this 4/4 and 2/5 bits difference could lead to a bug. So, it should be fixed. > > I have one comment to your patch. It is really looks like a workaround. I suggest you try to fix it in less hacky way. For example: > > - unsigned hi4() { return m_u.s1.hi4; } > + unsigned hi4() { return m_u.s1.hi4 & 0x7; } > > or > > - unsigned hi4 : 4; > + unsigned hi4 : 3; Did you try changing your memory allocator to hand out 0xff memset memory? You likely should hit it right away. Also you can inspect the asm generated by both and you'll see the difference. The reason I didn't make these changes because it doesn't match the method names. hi4 --> 4 bits. hi4 member --> 4 bits.. And this asInt style was used throughout the file as well to memset unions that don't fill out all the bits as well. Look at ThumbImmediateValue... 278964 10 loki 2010-09-14 09:23:13 -0700 > Did you try changing your memory allocator to hand out 0xff memset memory? You likely should hit it right away. Also you can inspect the asm generated by both and you'll see the difference. No, but I see the problem. > The reason I didn't make these changes because it doesn't match the method names. hi4 --> 4 bits. hi4 member --> 4 bits.. Probably this was a wrong naming-convention. The shifts and rotates are encoded into 2 type bits and 5 immediate bits for immediate shifts and rotates (in both ARM and Thumb-2 instruction set cases). It is maybe misleading that those Thumb-2 instructions where an immediate shift can be applied contain a zero in the 15th bit place (right above the imm3 part of shift value). So, the hi4 should be a hi3 in this case. > And this asInt style was used throughout the file as well to memset unions that don't fill out all the bits as well. Look at ThumbImmediateValue... Well, those asInt members are not just standing there for initialization/clearing unused bits. You can set and get value through asInt. ;) 279416 11 67435 eric 2010-09-14 23:53:10 -0700 Comment on attachment 67435 Add an asInt member and ZI it Looks like its missing a ChangeLog. 339015 12 dave+webkit 2011-01-24 07:57:33 -0800 *** This bug has been marked as a duplicate of bug 47356 *** 67406 2010-09-13 07:29:03 -0700 2010-09-13 11:14:16 -0700 Add an asInt member and ZI it 45669.txt text/plain 1574 dave+webkit Y29tbWl0IGY2ODRhYjVhMDE0NzhhMTBiNTNlOWQzY2VjMzI4NmQ1N2EyODYyMTMKQXV0aG9yOiBE YXZlIFRhcHVza2EgPGR0YXB1c2thQHJpbS5jb20+CkRhdGU6ICAgTW9uIFNlcCAxMyAxMDoyODow NiAyMDEwIC0wNDAwCgogICAgMjAxMC0wOS0xMCAgRGF2ZSBUYXB1c2thICA8ZHRhcHVza2FAcmlt LmNvbT4KICAgIAogICAgICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KICAgIAog ICAgICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDU2NjkK ICAgIAogICAgICAgICAgICBSVkNUIGRvZXNuJ3QgWkkgYml0ZmllbGRzLiBFbnN1cmUgdGhhdCB3 ZSBaSSBhbGwgdGhlIGJpdHMgb3IKICAgICAgICAgICAgd2UgY2FuIGVuZCB1cCB3aXRoIGEgYm9n dXMgYml0IGFuZCBnZW5lcmF0aW5nIGJhZCBpbnN0cnVjdGlvbnMuCiAgICAKICAgICAgICAgICAg KiBKYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvQVJNdjdBc3NlbWJsZXIuaDoKCmRpZmYgLS1naXQg YS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvQVJNdjdBc3NlbWJsZXIuaCBiL0phdmFTY3JpcHRD b3JlL2Fzc2VtYmxlci9BUk12N0Fzc2VtYmxlci5oCmluZGV4IGJhYjg5MjEuLmJmMjE2NzAgMTAw NjQ0Ci0tLSBhL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9BUk12N0Fzc2VtYmxlci5oCisrKyBi L0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9BUk12N0Fzc2VtYmxlci5oCkBAIC0xLDYgKzEsNyBA QAogLyoKICAqIENvcHlyaWdodCAoQykgMjAwOSBBcHBsZSBJbmMuIEFsbCByaWdodHMgcmVzZXJ2 ZWQuCiAgKiBDb3B5cmlnaHQgKEMpIDIwMTAgVW5pdmVyc2l0eSBvZiBTemVnZWQKKyAqIENvcHly aWdodCAoQykgUmVzZWFyY2ggSW4gTW90aW9uIExpbWl0ZWQgMjAxMC4gQWxsIHJpZ2h0cyByZXNl cnZlZC4KICAqCiAgKiBSZWRpc3RyaWJ1dGlvbiBhbmQgdXNlIGluIHNvdXJjZSBhbmQgYmluYXJ5 IGZvcm1zLCB3aXRoIG9yIHdpdGhvdXQKICAqIG1vZGlmaWNhdGlvbiwgYXJlIHBlcm1pdHRlZCBw cm92aWRlZCB0aGF0IHRoZSBmb2xsb3dpbmcgY29uZGl0aW9ucwpAQCAtNDIxLDEyICs0MjIsMTIg QEAgY2xhc3MgU2hpZnRUeXBlQW5kQW1vdW50IHsKIHB1YmxpYzoKICAgICBTaGlmdFR5cGVBbmRB bW91bnQoKQogICAgIHsKLSAgICAgICAgbV91LnR5cGUgPSAoQVJNU2hpZnRUeXBlKTA7Ci0gICAg ICAgIG1fdS5hbW91bnQgPSAwOworICAgICAgICBtX3UuYXNJbnQgPSAwOwogICAgIH0KICAgICAK ICAgICBTaGlmdFR5cGVBbmRBbW91bnQoQVJNU2hpZnRUeXBlIHR5cGUsIHVuc2lnbmVkIGFtb3Vu dCkKICAgICB7CisgICAgICAgIG1fdS5hc0ludCA9IDA7CiAgICAgICAgIG1fdS50eXBlID0gdHlw ZTsKICAgICAgICAgbV91LmFtb3VudCA9IGFtb3VudCAmIDMxOwogICAgIH0KQEAgLTQzNiw2ICs0 MzcsNyBAQCBwdWJsaWM6CiAgICAgCiBwcml2YXRlOgogICAgIHVuaW9uIHsKKyAgICAgICAgaW50 MTZfdCBhc0ludDsKICAgICAgICAgc3RydWN0IHsKICAgICAgICAgICAgIHVuc2lnbmVkIGxvNCA6 IDQ7CiAgICAgICAgICAgICB1bnNpZ25lZCBoaTQgOiA0Owo= 67435 2010-09-13 11:14:16 -0700 2010-09-14 23:53:10 -0700 Add an asInt member and ZI it 45669.txt text/plain 1574 dave+webkit Y29tbWl0IDA4NzBmNDFkOTE0ZWYxYmNkN2FhOTBjMmI5MGQ0MjY5Mjc2ZDcwZTgKQXV0aG9yOiBE YXZlIFRhcHVza2EgPGR0YXB1c2thQHJpbS5jb20+CkRhdGU6ICAgTW9uIFNlcCAxMyAxMDoyODow NiAyMDEwIC0wNDAwCgogICAgMjAxMC0wOS0xMCAgRGF2ZSBUYXB1c2thICA8ZHRhcHVza2FAcmlt LmNvbT4KICAgIAogICAgICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KICAgIAog ICAgICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDU2NjkK ICAgIAogICAgICAgICAgICBSVkNUIGRvZXNuJ3QgWkkgYml0ZmllbGRzLiBFbnN1cmUgdGhhdCB3 ZSBaSSBhbGwgdGhlIGJpdHMgb3IKICAgICAgICAgICAgd2UgY2FuIGVuZCB1cCB3aXRoIGEgYm9n dXMgYml0IGFuZCBnZW5lcmF0aW5nIGJhZCBpbnN0cnVjdGlvbnMuCiAgICAKICAgICAgICAgICAg KiBKYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvQVJNdjdBc3NlbWJsZXIuaDoKCmRpZmYgLS1naXQg YS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvQVJNdjdBc3NlbWJsZXIuaCBiL0phdmFTY3JpcHRD b3JlL2Fzc2VtYmxlci9BUk12N0Fzc2VtYmxlci5oCmluZGV4IDQ4ZWVmNTMuLjI0MGQzNjMgMTAw NjQ0Ci0tLSBhL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9BUk12N0Fzc2VtYmxlci5oCisrKyBi L0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9BUk12N0Fzc2VtYmxlci5oCkBAIC0xLDYgKzEsNyBA QAogLyoKICAqIENvcHlyaWdodCAoQykgMjAwOSBBcHBsZSBJbmMuIEFsbCByaWdodHMgcmVzZXJ2 ZWQuCiAgKiBDb3B5cmlnaHQgKEMpIDIwMTAgVW5pdmVyc2l0eSBvZiBTemVnZWQKKyAqIENvcHly aWdodCAoQykgUmVzZWFyY2ggSW4gTW90aW9uIExpbWl0ZWQgMjAxMC4gQWxsIHJpZ2h0cyByZXNl cnZlZC4KICAqCiAgKiBSZWRpc3RyaWJ1dGlvbiBhbmQgdXNlIGluIHNvdXJjZSBhbmQgYmluYXJ5 IGZvcm1zLCB3aXRoIG9yIHdpdGhvdXQKICAqIG1vZGlmaWNhdGlvbiwgYXJlIHBlcm1pdHRlZCBw cm92aWRlZCB0aGF0IHRoZSBmb2xsb3dpbmcgY29uZGl0aW9ucwpAQCAtNDE5LDEyICs0MjAsMTIg QEAgY2xhc3MgU2hpZnRUeXBlQW5kQW1vdW50IHsKIHB1YmxpYzoKICAgICBTaGlmdFR5cGVBbmRB bW91bnQoKQogICAgIHsKLSAgICAgICAgbV91LnR5cGUgPSAoQVJNU2hpZnRUeXBlKTA7Ci0gICAg ICAgIG1fdS5hbW91bnQgPSAwOworICAgICAgICBtX3UuYXNJbnQgPSAwOwogICAgIH0KICAgICAK ICAgICBTaGlmdFR5cGVBbmRBbW91bnQoQVJNU2hpZnRUeXBlIHR5cGUsIHVuc2lnbmVkIGFtb3Vu dCkKICAgICB7CisgICAgICAgIG1fdS5hc0ludCA9IDA7CiAgICAgICAgIG1fdS50eXBlID0gdHlw ZTsKICAgICAgICAgbV91LmFtb3VudCA9IGFtb3VudCAmIDMxOwogICAgIH0KQEAgLTQzNCw2ICs0 MzUsNyBAQCBwdWJsaWM6CiAgICAgCiBwcml2YXRlOgogICAgIHVuaW9uIHsKKyAgICAgICAgdWlu dDhfdCBhc0ludDsKICAgICAgICAgc3RydWN0IHsKICAgICAgICAgICAgIHVuc2lnbmVkIGxvNCA6 IDQ7CiAgICAgICAgICAgICB1bnNpZ25lZCBoaTQgOiA0Owo=