Summary: | ARMv7Assembler generates illegal instructions with RVCT | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Tapuska <dave+webkit> | ||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED DUPLICATE | ||||||||
Severity: | Normal | CC: | loki, oliver, staikos | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | Other | ||||||||
Attachments: |
|
Description
David Tapuska
2010-09-13 07:20:33 PDT
Created attachment 67406 [details]
Add an asInt member and ZI it
Comment on attachment 67406 [details] Add an asInt member and ZI it > + int16_t asInt; Shouldn't this be an 8-bit integer? Created attachment 67435 [details]
Add an asInt member and ZI it
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 I will keep a check on this tomorrow. Btw, David, which version of RVCT did you use? (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] (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. > 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;
(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... > 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. ;) Comment on attachment 67435 [details]
Add an asInt member and ZI it
Looks like its missing a ChangeLog.
*** This bug has been marked as a duplicate of bug 47356 *** |