Bug 45669

Summary: ARMv7Assembler generates illegal instructions with RVCT
Product: WebKit Reporter: David Tapuska <dave+webkit>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: loki, oliver, staikos
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
Add an asInt member and ZI it
none
Add an asInt member and ZI it eric: review-

Description David Tapuska 2010-09-13 07:20:33 PDT
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.
Comment 1 David Tapuska 2010-09-13 07:29:03 PDT
Created attachment 67406 [details]
Add an asInt member and ZI it
Comment 2 Andreas Kling 2010-09-13 09:36:42 PDT
Comment on attachment 67406 [details]
Add an asInt member and ZI it

> +        int16_t asInt;

Shouldn't this be an 8-bit integer?
Comment 3 David Tapuska 2010-09-13 11:14:16 PDT
Created attachment 67435 [details]
Add an asInt member and ZI it
Comment 4 Oliver Hunt 2010-09-13 11:52:55 PDT
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
Comment 5 Gabor Loki 2010-09-13 12:10:20 PDT
I will keep a check on this tomorrow.
Btw, David, which version of RVCT did you use?
Comment 6 David Tapuska 2010-09-13 12:26:14 PDT
(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]
Comment 7 David Tapuska 2010-09-13 12:28:52 PDT
(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.
Comment 8 Gabor Loki 2010-09-14 01:24:30 PDT
> 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;
Comment 9 David Tapuska 2010-09-14 06:24:57 PDT
(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...
Comment 10 Gabor Loki 2010-09-14 09:23:13 PDT
> 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 11 Eric Seidel (no email) 2010-09-14 23:53:10 PDT
Comment on attachment 67435 [details]
Add an asInt member and ZI it

Looks like its missing a ChangeLog.
Comment 12 David Tapuska 2011-01-24 07:57:33 PST

*** This bug has been marked as a duplicate of bug 47356 ***