PING: Default to -fstrict-volatile-bitfields for ARM EABI

[Jie Zhang]

On 10/23/2010 02:03 AM, Jie Zhang wrote:
> On 10/22/2010 05:40 PM, Richard Guenther wrote:
>> On Fri, 22 Oct 2010, Jie Zhang wrote:
>>
>>> Since the regression has been fixed now, it's time to reconsider this
>>> patch:
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00155.html
>>>
>>> Regression tested on latest trunk for arm-none-eabi with c,lto,c++.
>>>
>>> ARM maintainers: is the ARM port part OK?
>>>
>>> Richard G: Could you help me review the stor-layout.c part?
>>
>> Why is the stor-layout.c part necessary? Why does it not possibly
>> cause layout changes? I'm not very familiar with the stor-layout.c
>> code, and given that questions I think I'm not the right person
>> to review this patch.
>
> Thanks for your questions! It's not necessary indeed. I just retested
> the patch with the stor-layout.c change dropped. It has no regressions
> on arm-none-eabi with c,c++,lto and it passes all three new tests. I
> don't remember the exact details why I changed stor-layout.c at that
> time. So now the patch is only for ARM port. The updated patch is attached.
>
I have been trying to recall the reason of the stor-layout.c change for 
the last two days. Now I know. The regular expressions used in the three 
new new tests have a bug. The "." matches "\n" in expect, so "ldr\[\\t 
\]+.*,\[\\t \]*\\\[.*\\\]" will match

         ldr     r3, .L2
         ldrb    r0, [r3, #1]    @ zero_extendqisi2

but it's intended to match one instruction like

         ldr    r0, [r3, #0]

So the PASSes are fake after dropping the stor-layout.c change.

Now I can explain the stor-layout.c change. When layout_decl processes 
"b" bit-field in

typedef struct {
   volatile unsigned long a:8;
   volatile unsigned long b:8;
   volatile unsigned long c:16;
} BitStruct;

, this piece of code

	  /* See if we can use an ordinary integer mode for a bit-field.
	     Conditions are: a fixed size that is correct for another mode,
	     occupying a complete byte or bytes on proper boundary,
	     and not volatile or not -fstrict-volatile-bitfields.  */
	  if (TYPE_SIZE (type) != 0
	      && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
	      && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT)
	    {
	      enum machine_mode xmode
		= mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1);
	      unsigned int xalign = GET_MODE_ALIGNMENT (xmode);

	      if (xmode != BLKmode
		  && !(xalign > BITS_PER_UNIT && DECL_PACKED (decl))
		  && (known_align == 0 || known_align >= xalign))
		{
		  DECL_ALIGN (decl) = MAX (xalign, DECL_ALIGN (decl));
		  DECL_MODE (decl) = xmode;
		  DECL_BIT_FIELD (decl) = 0;
		}
	    }

will change the mode of "b" to QImode and clear DECL_BIT_FIELD (decl). 
Then later access to "b" will use byte load instruction instead of word 
load instruction as required by -fstrict-volatile-bitfields.

The attached patch fixes the tests and is resubmitted for approval for 
stor-layout.c change. Is it OK?


Regards,
-- 
Jie Zhang
CodeSourcery
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-arm-default-strict-volatile-bitfields-4.diff
Type: text/x-patch
Size: 3865 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20101024/40130ca9/attachment.bin>