Bug 45669 – ARMv7Assembler generates illegal instructions with RVCT

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[David Tapuska]

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.

[David Tapuska]

Created attachment 67406 [details]
Add an asInt member and ZI it

[Andreas Kling]

Comment on attachment 67406 [details]
Add an asInt member and ZI it

> +        int16_t asInt;

Shouldn't this be an 8-bit integer?

[David Tapuska]

Created attachment 67435 [details]
Add an asInt member and ZI it

[Oliver Hunt]

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

[Gabor Loki]

I will keep a check on this tomorrow.
Btw, David, which version of RVCT did you use?

[David Tapuska]

(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]

[David Tapuska]

(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.

[Gabor Loki]

> 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;

[David Tapuska]

(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...

[Gabor Loki]

> 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. ;)

[Eric Seidel (no email)]

Comment on attachment 67435 [details]
Add an asInt member and ZI it

Looks like its missing a ChangeLog.

[David Tapuska]

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