public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Strict volatile bit-fields clean-up
@ 2013-11-20 13:38 Bernd Edlinger
  2013-12-02 14:43 ` Richard Biener
  0 siblings, 1 reply; 11+ messages in thread
From: Bernd Edlinger @ 2013-11-20 13:38 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

Hello Richard,

as a follow-up patch to the bit-fields patch(es), I wanted to remove the dependencies on
the variable flag_strict_volatile_bitfields from expand_assignment and expand_expr_real_1.
Additionally I want the access mode of the field to be selected in the memory context,
instead of the structure's mode.

Boot-strapped and regression-tested on x86_64-linux-gnu.

OK for trunk?

Thanks
Bernd.


FYI - these are my in-flight patches, which would be nice to go into the GCC 4.9.0 release:


http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02046.html :[PATCH] reimplement -fstrict-volatile-bitfields v4, part 1/2
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02025.html :[PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html :
[PATCH, PR 57748] Check for out of bounds access, Part 2
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00581.html :[PATCH, PR 57748] Check for out of bounds access, Part 3
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00133.html:[PATCH] Fix PR58115 		 	   		  

[-- Attachment #2: changelog-bitfields-cleanup.txt --]
[-- Type: text/plain, Size: 213 bytes --]

2013-11-20  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* expr.c (expand_assignment): Remove dependency on
	flag_strict_volatile_bitfields. Always set the memory 
	access mode.
	(expand_expr_real_1): Likewise.


[-- Attachment #3: patch-bitfields-cleanup.diff --]
[-- Type: application/octet-stream, Size: 2666 bytes --]

--- gcc/expr.c.jj	2013-11-19 16:15:50.166853838 +0100
+++ gcc/expr.c	2013-11-20 10:41:16.793695891 +0100
@@ -4731,13 +4731,13 @@ expand_assignment (tree to, tree from, b
 
       to_rtx = expand_expr (tem, NULL_RTX, VOIDmode, EXPAND_WRITE);
 
-      /* If the bitfield is volatile, we want to access it in the
+      /* If the field has a mode, we want to access it in the
 	 field's mode, not the computed mode.
 	 If a MEM has VOIDmode (external with incomplete type),
 	 use BLKmode for it instead.  */
       if (MEM_P (to_rtx))
 	{
-	  if (volatilep && flag_strict_volatile_bitfields > 0)
+	  if (mode1 != VOIDmode)
 	    to_rtx = adjust_address (to_rtx, mode1, 0);
 	  else if (GET_MODE (to_rtx) == VOIDmode)
 	    to_rtx = adjust_address (to_rtx, BLKmode, 0);
@@ -9866,13 +9866,13 @@ expand_expr_real_1 (tree exp, rtx target
 			      modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier,
 			      NULL, true);
 
-	/* If the bitfield is volatile, we want to access it in the
+	/* If the field has a mode, we want to access it in the
 	   field's mode, not the computed mode.
 	   If a MEM has VOIDmode (external with incomplete type),
 	   use BLKmode for it instead.  */
 	if (MEM_P (op0))
 	  {
-	    if (volatilep && flag_strict_volatile_bitfields > 0)
+	    if (mode1 != VOIDmode)
 	      op0 = adjust_address (op0, mode1, 0);
 	    else if (GET_MODE (op0) == VOIDmode)
 	      op0 = adjust_address (op0, BLKmode, 0);
@@ -10002,17 +10002,13 @@ expand_expr_real_1 (tree exp, rtx target
 		&& modifier != EXPAND_CONST_ADDRESS
 		&& modifier != EXPAND_INITIALIZER
 		&& modifier != EXPAND_MEMORY)
-	    /* If the field is volatile, we always want an aligned
-	       access.  Do this in following two situations:
-	       1. the access is not already naturally
-	       aligned, otherwise "normal" (non-bitfield) volatile fields
-	       become non-addressable.
-	       2. the bitsize is narrower than the access size. Need
-	       to extract bitfields from the access.  */
-	    || (volatilep && flag_strict_volatile_bitfields > 0
-		&& (bitpos % GET_MODE_ALIGNMENT (mode) != 0 
-		    || (mode1 != BLKmode
-		        && bitsize < GET_MODE_SIZE (mode1) * BITS_PER_UNIT)))
+	    /* If the bitfield is volatile and the bitsize
+	       is narrower than the access size of the bitfield,
+	       we need to extract bitfields from the access.  */
+	    || (volatilep && TREE_CODE (exp) == COMPONENT_REF
+		&& DECL_BIT_FIELD_TYPE (TREE_OPERAND (exp, 1))
+		&& mode1 != BLKmode
+		&& bitsize < GET_MODE_SIZE (mode1) * BITS_PER_UNIT)
 	    /* If the field isn't aligned enough to fetch as a memref,
 	       fetch it as a bit field.  */
 	    || (mode1 != BLKmode

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Strict volatile bit-fields clean-up
  2013-11-20 13:38 [PATCH] Strict volatile bit-fields clean-up Bernd Edlinger
@ 2013-12-02 14:43 ` Richard Biener
  2013-12-03  9:47   ` Bernd Edlinger
  0 siblings, 1 reply; 11+ messages in thread
From: Richard Biener @ 2013-12-02 14:43 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: gcc-patches

On Wed, Nov 20, 2013 at 11:48 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hello Richard,
>
> as a follow-up patch to the bit-fields patch(es), I wanted to remove the dependencies on
> the variable flag_strict_volatile_bitfields from expand_assignment and expand_expr_real_1.
> Additionally I want the access mode of the field to be selected in the memory context,
> instead of the structure's mode.
>
> Boot-strapped and regression-tested on x86_64-linux-gnu.
>
> OK for trunk?

Ok.

Thanks,
Richard.

> Thanks
> Bernd.
>
>
> FYI - these are my in-flight patches, which would be nice to go into the GCC 4.9.0 release:
>
>
> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02046.html :[PATCH] reimplement -fstrict-volatile-bitfields v4, part 1/2
> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02025.html :[PATCH] Fix C++0x memory model for -fno-strict-volatile-bitfields on ARM
> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html :
> [PATCH, PR 57748] Check for out of bounds access, Part 2
> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00581.html :[PATCH, PR 57748] Check for out of bounds access, Part 3
> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00133.html:[PATCH] Fix PR58115

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Strict volatile bit-fields clean-up
  2013-12-02 14:43 ` Richard Biener
@ 2013-12-03  9:47   ` Bernd Edlinger
  2013-12-03  9:59     ` Richard Biener
  2013-12-03 10:34     ` Eric Botcazou
  0 siblings, 2 replies; 11+ messages in thread
From: Bernd Edlinger @ 2013-12-03  9:47 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, Eric Botcazou

On Mon, 2 Dec 2013 15:42:48, Richard Biener wrote:
>
> On Wed, Nov 20, 2013 at 11:48 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Hello Richard,
>>
>> as a follow-up patch to the bit-fields patch(es), I wanted to remove the dependencies on
>> the variable flag_strict_volatile_bitfields from expand_assignment and expand_expr_real_1.
>> Additionally I want the access mode of the field to be selected in the memory context,
>> instead of the structure's mode.
>>
>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>
>> OK for trunk?
>
> Ok.
>
> Thanks,
> Richard.
>


Oops....

Sorry, tonight this patch caused an Ada regression, in pack17.adb and misaligned_nest.adb

So I'll have to put that on hold at the moment.

This ICE is very similar to PR59134.
It is again the recursion between store_fixed_bit_field and store_split_bit_field.

The trigger is a latent problem in the ada gcc_interface.

That is we have a bit-field of exactly 8 bit size, which is not byte-aligned,
but DECL_MODE=QImode, DECL_BIT_FIELD=false which looks quite strange,
and is totally different from how C structures look like. I should mention that there
are even some places in the target back-ends, where the attribute DECL_BIT_FIELD is
used for whatever.

Now, due to this hunk in the cleanup-patch we get the QImode selected in the memory
context:

       if (MEM_P (to_rtx))
        {
-         if (volatilep && flag_strict_volatile_bitfields> 0)
+         if (mode1 != VOIDmode)
            to_rtx = adjust_address (to_rtx, mode1, 0);

However even without that patch, I can arrange for "volatilep && flag_strict_volatile_bitfields> 0"
to be true in Ada (even on X86_64, or whatever platform you like):

-- { dg-do run }
-- { dg-options "-gnatp -fstrict-volatile-bitfields" }

procedure Misaligned_Volatile is

   type Byte is mod 2**8;

   type Block is record
      B : Boolean;
      V : Byte;
   end record;
   pragma Volatile (Block);
   pragma Pack (Block);
   for Block'Alignment use 1;

   type Pair is array (1 .. 2) of Block;

   P : Pair;
begin
   for K in P'Range loop
      P(K).V := 237;
   end loop;
   for K in P'Range loop
      if P(K).V /= 237 then
         raise Program_error;
      end if;
   end loop;
end;


This Ada test case causes either wrong code generation or an ICE at compile time,
if the -fstrict-volatile-bitfields option is either given by the user,
or by the target-specific default as it is on ARM for instance  (which is completely
pointless on Ada, I know!)...

Now I am preparing a new bitfields-update-patch which fixes this above test case and the
latent recursion problem.


Thanks  ...  for you patience :-(
Bernd. 		 	   		  

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Strict volatile bit-fields clean-up
  2013-12-03  9:47   ` Bernd Edlinger
@ 2013-12-03  9:59     ` Richard Biener
  2013-12-03 10:04       ` Richard Biener
  2013-12-03 10:09       ` Bernd Edlinger
  2013-12-03 10:34     ` Eric Botcazou
  1 sibling, 2 replies; 11+ messages in thread
From: Richard Biener @ 2013-12-03  9:59 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: gcc-patches, Eric Botcazou

On Tue, Dec 3, 2013 at 10:47 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On Mon, 2 Dec 2013 15:42:48, Richard Biener wrote:
>>
>> On Wed, Nov 20, 2013 at 11:48 AM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> Hello Richard,
>>>
>>> as a follow-up patch to the bit-fields patch(es), I wanted to remove the dependencies on
>>> the variable flag_strict_volatile_bitfields from expand_assignment and expand_expr_real_1.
>>> Additionally I want the access mode of the field to be selected in the memory context,
>>> instead of the structure's mode.
>>>
>>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>>
>>> OK for trunk?
>>
>> Ok.
>>
>> Thanks,
>> Richard.
>>
>
>
> Oops....
>
> Sorry, tonight this patch caused an Ada regression, in pack17.adb and misaligned_nest.adb
>
> So I'll have to put that on hold at the moment.
>
> This ICE is very similar to PR59134.
> It is again the recursion between store_fixed_bit_field and store_split_bit_field.
>
> The trigger is a latent problem in the ada gcc_interface.
>
> That is we have a bit-field of exactly 8 bit size, which is not byte-aligned,
> but DECL_MODE=QImode, DECL_BIT_FIELD=false which looks quite strange,

It's not required that DECL_BIT_FIELD is set I think, and the mode
is that of the type if it's not a bitfield ...

> and is totally different from how C structures look like. I should mention that there
> are even some places in the target back-ends, where the attribute DECL_BIT_FIELD is
> used for whatever.
>
> Now, due to this hunk in the cleanup-patch we get the QImode selected in the memory
> context:
>
>        if (MEM_P (to_rtx))
>         {
> -         if (volatilep && flag_strict_volatile_bitfields> 0)
> +         if (mode1 != VOIDmode)
>             to_rtx = adjust_address (to_rtx, mode1, 0);

Which I think is correct - the memory access is in QImode - that's what
get_inner_reference said.

So whatever issue we run into downstream points to the bug ...

OTOH Eric may know better what the middle-end can expect for
bit-aligned Ada records (apart from "all bets are off" which isn't
really a good solution ;))

Richard.

> However even without that patch, I can arrange for "volatilep && flag_strict_volatile_bitfields> 0"
> to be true in Ada (even on X86_64, or whatever platform you like):
>
> -- { dg-do run }
> -- { dg-options "-gnatp -fstrict-volatile-bitfields" }
>
> procedure Misaligned_Volatile is
>
>    type Byte is mod 2**8;
>
>    type Block is record
>       B : Boolean;
>       V : Byte;
>    end record;
>    pragma Volatile (Block);
>    pragma Pack (Block);
>    for Block'Alignment use 1;
>
>    type Pair is array (1 .. 2) of Block;
>
>    P : Pair;
> begin
>    for K in P'Range loop
>       P(K).V := 237;
>    end loop;
>    for K in P'Range loop
>       if P(K).V /= 237 then
>          raise Program_error;
>       end if;
>    end loop;
> end;
>
>
> This Ada test case causes either wrong code generation or an ICE at compile time,
> if the -fstrict-volatile-bitfields option is either given by the user,
> or by the target-specific default as it is on ARM for instance  (which is completely
> pointless on Ada, I know!)...
>
> Now I am preparing a new bitfields-update-patch which fixes this above test case and the
> latent recursion problem.
>
>
> Thanks  ...  for you patience :-(
> Bernd.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Strict volatile bit-fields clean-up
  2013-12-03  9:59     ` Richard Biener
@ 2013-12-03 10:04       ` Richard Biener
  2013-12-03 10:09       ` Bernd Edlinger
  1 sibling, 0 replies; 11+ messages in thread
From: Richard Biener @ 2013-12-03 10:04 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: gcc-patches, Eric Botcazou

On Tue, Dec 3, 2013 at 10:59 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, Dec 3, 2013 at 10:47 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> On Mon, 2 Dec 2013 15:42:48, Richard Biener wrote:
>>>
>>> On Wed, Nov 20, 2013 at 11:48 AM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> Hello Richard,
>>>>
>>>> as a follow-up patch to the bit-fields patch(es), I wanted to remove the dependencies on
>>>> the variable flag_strict_volatile_bitfields from expand_assignment and expand_expr_real_1.
>>>> Additionally I want the access mode of the field to be selected in the memory context,
>>>> instead of the structure's mode.
>>>>
>>>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>>>
>>>> OK for trunk?
>>>
>>> Ok.
>>>
>>> Thanks,
>>> Richard.
>>>
>>
>>
>> Oops....
>>
>> Sorry, tonight this patch caused an Ada regression, in pack17.adb and misaligned_nest.adb
>>
>> So I'll have to put that on hold at the moment.
>>
>> This ICE is very similar to PR59134.
>> It is again the recursion between store_fixed_bit_field and store_split_bit_field.
>>
>> The trigger is a latent problem in the ada gcc_interface.
>>
>> That is we have a bit-field of exactly 8 bit size, which is not byte-aligned,
>> but DECL_MODE=QImode, DECL_BIT_FIELD=false which looks quite strange,
>
> It's not required that DECL_BIT_FIELD is set I think, and the mode
> is that of the type if it's not a bitfield ...

Btw, I see DECL_BIT_FIELD and DECL_BIT_FIELD_TYPE set (but QImode
used) for 'v' on x86_64 and the testcase "works".

>> and is totally different from how C structures look like. I should mention that there
>> are even some places in the target back-ends, where the attribute DECL_BIT_FIELD is
>> used for whatever.
>>
>> Now, due to this hunk in the cleanup-patch we get the QImode selected in the memory
>> context:
>>
>>        if (MEM_P (to_rtx))
>>         {
>> -         if (volatilep && flag_strict_volatile_bitfields> 0)
>> +         if (mode1 != VOIDmode)
>>             to_rtx = adjust_address (to_rtx, mode1, 0);
>
> Which I think is correct - the memory access is in QImode - that's what
> get_inner_reference said.
>
> So whatever issue we run into downstream points to the bug ...
>
> OTOH Eric may know better what the middle-end can expect for
> bit-aligned Ada records (apart from "all bets are off" which isn't
> really a good solution ;))
>
> Richard.
>
>> However even without that patch, I can arrange for "volatilep && flag_strict_volatile_bitfields> 0"
>> to be true in Ada (even on X86_64, or whatever platform you like):
>>
>> -- { dg-do run }
>> -- { dg-options "-gnatp -fstrict-volatile-bitfields" }
>>
>> procedure Misaligned_Volatile is
>>
>>    type Byte is mod 2**8;
>>
>>    type Block is record
>>       B : Boolean;
>>       V : Byte;
>>    end record;
>>    pragma Volatile (Block);
>>    pragma Pack (Block);
>>    for Block'Alignment use 1;
>>
>>    type Pair is array (1 .. 2) of Block;
>>
>>    P : Pair;
>> begin
>>    for K in P'Range loop
>>       P(K).V := 237;
>>    end loop;
>>    for K in P'Range loop
>>       if P(K).V /= 237 then
>>          raise Program_error;
>>       end if;
>>    end loop;
>> end;
>>
>>
>> This Ada test case causes either wrong code generation or an ICE at compile time,
>> if the -fstrict-volatile-bitfields option is either given by the user,
>> or by the target-specific default as it is on ARM for instance  (which is completely
>> pointless on Ada, I know!)...
>>
>> Now I am preparing a new bitfields-update-patch which fixes this above test case and the
>> latent recursion problem.
>>
>>
>> Thanks  ...  for you patience :-(
>> Bernd.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Strict volatile bit-fields clean-up
  2013-12-03  9:59     ` Richard Biener
  2013-12-03 10:04       ` Richard Biener
@ 2013-12-03 10:09       ` Bernd Edlinger
  2013-12-03 11:04         ` Eric Botcazou
  1 sibling, 1 reply; 11+ messages in thread
From: Bernd Edlinger @ 2013-12-03 10:09 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, Eric Botcazou

On Tue, 3 Dec 2013 10:59:15, Richard Biener wrote:
>
> On Tue, Dec 3, 2013 at 10:47 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> On Mon, 2 Dec 2013 15:42:48, Richard Biener wrote:
>>>
>>> On Wed, Nov 20, 2013 at 11:48 AM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> Hello Richard,
>>>>
>>>> as a follow-up patch to the bit-fields patch(es), I wanted to remove the dependencies on
>>>> the variable flag_strict_volatile_bitfields from expand_assignment and expand_expr_real_1.
>>>> Additionally I want the access mode of the field to be selected in the memory context,
>>>> instead of the structure's mode.
>>>>
>>>> Boot-strapped and regression-tested on x86_64-linux-gnu.
>>>>
>>>> OK for trunk?
>>>
>>> Ok.
>>>
>>> Thanks,
>>> Richard.
>>>
>>
>>
>> Oops....
>>
>> Sorry, tonight this patch caused an Ada regression, in pack17.adb and misaligned_nest.adb
>>
>> So I'll have to put that on hold at the moment.
>>
>> This ICE is very similar to PR59134.
>> It is again the recursion between store_fixed_bit_field and store_split_bit_field.
>>
>> The trigger is a latent problem in the ada gcc_interface.
>>
>> That is we have a bit-field of exactly 8 bit size, which is not byte-aligned,
>> but DECL_MODE=QImode, DECL_BIT_FIELD=false which looks quite strange,
>
> It's not required that DECL_BIT_FIELD is set I think, and the mode
> is that of the type if it's not a bitfield ...
>

I agree. But I hope the back-ends agree too.

>> and is totally different from how C structures look like. I should mention that there
>> are even some places in the target back-ends, where the attribute DECL_BIT_FIELD is
>> used for whatever.
>>
>> Now, due to this hunk in the cleanup-patch we get the QImode selected in the memory
>> context:
>>
>> if (MEM_P (to_rtx))
>> {
>> - if (volatilep && flag_strict_volatile_bitfields> 0)
>> + if (mode1 != VOIDmode)
>> to_rtx = adjust_address (to_rtx, mode1, 0);
>
> Which I think is correct - the memory access is in QImode - that's what
> get_inner_reference said.
>

I absolutely agree. I want it this way everything else is nonsense.

> So whatever issue we run into downstream points to the bug ...
>

That's for sure. I think I have a good idea, how to fix the root of the recursion.

I just have to write a change-log and will send an update in a few moments.

Thanks
Bernd.

> OTOH Eric may know better what the middle-end can expect for
> bit-aligned Ada records (apart from "all bets are off" which isn't
> really a good solution ;))
>

Yes. I somehow did expect DECL_BIT_FIELD to be something simple,
without dependency on if -fstrict-volatile-bitfields is given, or what
Language front-end generated it, or if STRICT_ALIGNMENT is defined.

> Richard.
>
>> However even without that patch, I can arrange for "volatilep && flag_strict_volatile_bitfields> 0"
>> to be true in Ada (even on X86_64, or whatever platform you like):
>>
>> -- { dg-do run }
>> -- { dg-options "-gnatp -fstrict-volatile-bitfields" }
>>
>> procedure Misaligned_Volatile is
>>
>> type Byte is mod 2**8;
>>
>> type Block is record
>> B : Boolean;
>> V : Byte;
>> end record;
>> pragma Volatile (Block);
>> pragma Pack (Block);
>> for Block'Alignment use 1;
>>
>> type Pair is array (1 .. 2) of Block;
>>
>> P : Pair;
>> begin
>> for K in P'Range loop
>> P(K).V := 237;
>> end loop;
>> for K in P'Range loop
>> if P(K).V /= 237 then
>> raise Program_error;
>> end if;
>> end loop;
>> end;
>>
>>
>> This Ada test case causes either wrong code generation or an ICE at compile time,
>> if the -fstrict-volatile-bitfields option is either given by the user,
>> or by the target-specific default as it is on ARM for instance (which is completely
>> pointless on Ada, I know!)...
>>
>> Now I am preparing a new bitfields-update-patch which fixes this above test case and the
>> latent recursion problem.
>>
>>
>> Thanks ... for you patience :-(
>> Bernd. 		 	   		  

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Strict volatile bit-fields clean-up
  2013-12-03  9:47   ` Bernd Edlinger
  2013-12-03  9:59     ` Richard Biener
@ 2013-12-03 10:34     ` Eric Botcazou
  1 sibling, 0 replies; 11+ messages in thread
From: Eric Botcazou @ 2013-12-03 10:34 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: gcc-patches, Richard Biener

> The trigger is a latent problem in the ada gcc_interface.
> 
> That is we have a bit-field of exactly 8 bit size, which is not
> byte-aligned, but DECL_MODE=QImode, DECL_BIT_FIELD=false which looks quite
> strange, and is totally different from how C structures look like. I should
> mention that there are even some places in the target back-ends, where the
> attribute DECL_BIT_FIELD is used for whatever.

This needs to be investigated because the intent is very clear in gigi, see 
create_field_decl and finish_record_type: we set DECL_BIT_FIELD conservatively
and clear it only when we know that we can.  This code is quite old now.

> However even without that patch, I can arrange for "volatilep &&
> flag_strict_volatile_bitfields> 0" to be true in Ada (even on X86_64, or
> whatever platform you like):
> 
> -- { dg-do run }
> -- { dg-options "-gnatp -fstrict-volatile-bitfields" }
> 
> procedure Misaligned_Volatile is
> 
>    type Byte is mod 2**8;
> 
>    type Block is record
>       B : Boolean;
>       V : Byte;
>    end record;
>    pragma Volatile (Block);
>    pragma Pack (Block);
>    for Block'Alignment use 1;
> 
>    type Pair is array (1 .. 2) of Block;
> 
>    P : Pair;
> begin
>    for K in P'Range loop
>       P(K).V := 237;
>    end loop;
>    for K in P'Range loop
>       if P(K).V /= 237 then
>          raise Program_error;
>       end if;
>    end loop;
> end;
> 
> 
> This Ada test case causes either wrong code generation or an ICE at compile
> time, if the -fstrict-volatile-bitfields option is either given by the
> user, or by the target-specific default as it is on ARM for instance 
> (which is completely pointless on Ada, I know!)...

The test indeed raises Program_Error on x86-64.

-- 
Eric Botcazou

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Strict volatile bit-fields clean-up
  2013-12-03 10:09       ` Bernd Edlinger
@ 2013-12-03 11:04         ` Eric Botcazou
  2013-12-03 11:51           ` Bernd Edlinger
  0 siblings, 1 reply; 11+ messages in thread
From: Eric Botcazou @ 2013-12-03 11:04 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: gcc-patches, Richard Biener

> Yes. I somehow did expect DECL_BIT_FIELD to be something simple,
> without dependency on if -fstrict-volatile-bitfields is given, or what
> Language front-end generated it, or if STRICT_ALIGNMENT is defined.

Yes, it should be that way, and everything else is a bug.

-- 
Eric Botcazou

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Strict volatile bit-fields clean-up
  2013-12-03 11:04         ` Eric Botcazou
@ 2013-12-03 11:51           ` Bernd Edlinger
  2013-12-03 12:01             ` Eric Botcazou
  0 siblings, 1 reply; 11+ messages in thread
From: Bernd Edlinger @ 2013-12-03 11:51 UTC (permalink / raw)
  To: Eric Botcazou; +Cc: gcc-patches, Richard Biener

>
>> Yes. I somehow did expect DECL_BIT_FIELD to be something simple,
>> without dependency on if -fstrict-volatile-bitfields is given, or what
>> Language front-end generated it, or if STRICT_ALIGNMENT is defined.
>
> Yes, it should be that way, and everything else is a bug.
>

I tried something like this, and it did boot-strap, but failed in some acats tests:

      /* If the bitfield is volatile, we want to access it in the
         field's mode, not the computed mode.
         If a MEM has VOIDmode (external with incomplete type),
         use BLKmode for it instead.  */
      if (MEM_P (to_rtx))
        {
+          gcc_assert ((TREE_CODE (to) == COMPONENT_REF && DECL_BIT_FIELD (TREE_OPERAND (to, 1)))
+                             || (bitpos % BITS_PER_UNIT == 0 && bitsize % BITS_PER_UNIT == 0));
          if (volatilep && flag_strict_volatile_bitfields> 0)
            to_rtx = adjust_address (to_rtx, mode1, 0);



Bernd. 		 	   		  

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Strict volatile bit-fields clean-up
  2013-12-03 11:51           ` Bernd Edlinger
@ 2013-12-03 12:01             ` Eric Botcazou
  2013-12-03 12:04               ` Bernd Edlinger
  0 siblings, 1 reply; 11+ messages in thread
From: Eric Botcazou @ 2013-12-03 12:01 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: gcc-patches, Richard Biener

> I tried something like this, and it did boot-strap, but failed in some acats
> tests:

Am I supposed to guess which ones?  Note that you can have non-byte-aligned 
aggregate fields in Ada, i.e. arrays, so ARRAY_REFs can presumably reach here.

-- 
Eric Botcazou

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] Strict volatile bit-fields clean-up
  2013-12-03 12:01             ` Eric Botcazou
@ 2013-12-03 12:04               ` Bernd Edlinger
  0 siblings, 0 replies; 11+ messages in thread
From: Bernd Edlinger @ 2013-12-03 12:04 UTC (permalink / raw)
  To: Eric Botcazou; +Cc: gcc-patches, Richard Biener

----------------------------------------
> From: ebotcazou@adacore.com
> To: bernd.edlinger@hotmail.de
> CC: gcc-patches@gcc.gnu.org; richard.guenther@gmail.com
> Subject: Re: [PATCH] Strict volatile bit-fields clean-up
> Date: Tue, 3 Dec 2013 13:00:25 +0100
>
>> I tried something like this, and it did boot-strap, but failed in some acats
>> tests:
>
> Am I supposed to guess which ones? Note that you can have non-byte-aligned
> aggregate fields in Ada, i.e. arrays, so ARRAY_REFs can presumably reach here.
>
> --
> Eric Botcazou

hmm, probably yes. that was a long night. sorry. they all had to do with nested views,
I think. 		 	   		  

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-12-03 12:04 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-20 13:38 [PATCH] Strict volatile bit-fields clean-up Bernd Edlinger
2013-12-02 14:43 ` Richard Biener
2013-12-03  9:47   ` Bernd Edlinger
2013-12-03  9:59     ` Richard Biener
2013-12-03 10:04       ` Richard Biener
2013-12-03 10:09       ` Bernd Edlinger
2013-12-03 11:04         ` Eric Botcazou
2013-12-03 11:51           ` Bernd Edlinger
2013-12-03 12:01             ` Eric Botcazou
2013-12-03 12:04               ` Bernd Edlinger
2013-12-03 10:34     ` Eric Botcazou

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).