diff mbox

[Ada] Fix bugs with volatile and components of aggregate types

Message ID 201106181255.09679.ebotcazou@adacore.com
State New
Headers show

Commit Message

Eric Botcazou June 18, 2011, 10:55 a.m. UTC 
This is the usual problem of volatile accesses not preserved under (heavy) 
optimization.  In Ada, we can put pragma Volatile on components of composite 
types without putting it on the enclosing type itself, but this doesn't really 
work when you're starting to optimize.

Mostly gigi changes, but this plugs a hole in type_internals_preclude_sra_p 
which already tests TREE_THIS_VOLATILE on fields of record and union types but 
doesn't perform the equivalent test for array types.

Tested on i586-suse-linux, applied on mainline, as obvious for the SRA bits.


2011-06-18  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-sra.c (type_internals_preclude_sra_p) <ARRAY_TYPE>: Return true
	if the element type is volatile.
ada/
	* gcc-interface/decl.c (gnat_to_gnu_component_type): Use GNAT_TYPE
	local variable throughout.  Remove useless call to Base_Type.
	(gnat_to_gnu_field): Use GNAT_FIELD_TYPE local variable throughout.
	Take it also into account for the volatileness of the field.  Set the
	TREE_SIDE_EFFECTS flag as well in this case.  Reorder some warnings.


2011-06-18  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/volatile6.adb: New test.
	* gnat.dg/volatile7.adb: Likewise.
	* gnat.dg/volatile8.adb: Likewise.
	* gnat.dg/volatile9.adb: Likewise.

Comments

Duncan Sands June 19, 2011, 12:54 p.m. UTC | #1 
Hi Eric,

> This is the usual problem of volatile accesses not preserved under (heavy)
> optimization.  In Ada, we can put pragma Volatile on components of composite
> types without putting it on the enclosing type itself,

if T is a non-volatile composite type with volatile components, and O is an
object of type T, are the optimizers allowed to remove the assignment "O := O"?

Ciao, Duncan.
Eric Botcazou June 19, 2011, 11:24 p.m. UTC | #2 
> if T is a non-volatile composite type with volatile components, and O is an
> object of type T, are the optimizers allowed to remove the assignment "O :=
> O"?

Good question, that I'm not really qualified to answer.  Any language lawyer?
Mike Stump June 19, 2011, 11:43 p.m. UTC | #3 
On Jun 19, 2011, at 4:24 PM, Eric Botcazou wrote:
>> if T is a non-volatile composite type with volatile components, and O is an
>> object of type T, are the optimizers allowed to remove the assignment "O :=
>> O"?
> 
> Good question, that I'm not really qualified to answer.  Any language lawyer?

I'd like to think that the optimizers are not allowed to remove the assignment (unless they synthesize of up the assignment of all volatile fields).  For unions, head explodes.  I think for unions, it can.  I could check the standard to ensure I have it right, but, well, that's annoying.  :-)  This answer is for the answer for C and C++, but, the middle end, _could_ decide to differ, if it had a compelling reason to.  I don't know of any reason.  Now, before someone tries to optimize structures, please be very careful.  Unions, transparent unions, frontend synthed structures with multiple offsets at the same location and the like come to mind.
Richard Biener June 20, 2011, 11:23 a.m. UTC | #4 
On Mon, Jun 20, 2011 at 1:24 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> if T is a non-volatile composite type with volatile components, and O is an
>> object of type T, are the optimizers allowed to remove the assignment "O :=
>> O"?
>
> Good question, that I'm not really qualified to answer.  Any language lawyer?

Optimizers will test the TREE_THIS_VOLATILE flag on the expression
codes performing the load/store.  So if O is not TREE_THIS_VOLATILE
they won't care.  If frontends want something different in the above case
they have to make sure to properly set TREE_THIS_VOLATILE.

Richard.

> --
> Eric Botcazou
>
Michael Matz June 20, 2011, 2:04 p.m. UTC | #5 
Hi,

On Sun, 19 Jun 2011, Mike Stump wrote:

> >> if T is a non-volatile composite type with volatile components, and O 
> >> is an object of type T, are the optimizers allowed to remove the 
> >> assignment "O := O"?
> > 
> > Good question, that I'm not really qualified to answer.  Any language 
> > lawyer?
> 
> I'd like to think that the optimizers are not allowed to remove the 
> assignment (unless they synthesize of up the assignment of all volatile 
> fields).  For unions, head explodes.  I think for unions, it can.  I 
> could check the standard to ensure I have it right, but, well, that's 
> annoying.  :-)  This answer is for the answer for C and C++, but, the 
> middle end, _could_ decide to differ, if it had a compelling reason to.  
> I don't know of any reason.  Now, before someone tries to optimize 
> structures, please be very careful.  Unions, transparent unions, 
> frontend synthed structures with multiple offsets at the same location 
> and the like come to mind.

The middle end really can't decide.  See also PR45472, from comment #7:

-----------------------------------------------
This all points to a deeper problem IMO, one we need to decide how to 
solve before tackling the details of this bug, namely: how are 
non-volatile objects containing volatile subobjects handled?  I don't know 
if the language standard says anything about this.

We have several possibilities:
1) make all objects containing volatile subobjects volatile itself
2) make instructions touching such objects volatile
3) make instructions touching the volatile components volatile

The first has the problem that volatile objects with aggregate type
implicitely contain only volatile subobjects, that is for such object:
  struct {int a; volatile int b;} half_volatile;
with solution (1) this would be equivalent to
  volatile struct {int a; volatile int b;} half_volatile;
and hence halt_volatile.a would be volatile too, probably unlike the
author intended.

The other two solutions are more work, especially because the
half-volatility wouldn't be reflected in the objects type.
-----------------------------------------------

Downwards we decided that it's really the languages business to define
half-volatility, so it either needs to or doesn't need to emit volatile
mem_refs according to its standard.


Ciao,
Michael.
diff mbox

Patch

Index: ada/gcc-interface/decl.c
===================================================================
--- ada/gcc-interface/decl.c	(revision 175136)
+++ ada/gcc-interface/decl.c	(working copy)
@@ -5229,7 +5229,8 @@  static tree
 gnat_to_gnu_component_type (Entity_Id gnat_array, bool definition,
 			    bool debug_info_p)
 {
-  tree gnu_type = gnat_to_gnu_type (Component_Type (gnat_array));
+  const Entity_Id gnat_type = Component_Type (gnat_array);
+  tree gnu_type = gnat_to_gnu_type (gnat_type);
   tree gnu_comp_size;
 
   /* Try to get a smaller form of the component if needed.  */
@@ -5237,7 +5238,7 @@  gnat_to_gnu_component_type (Entity_Id gn
        || Has_Component_Size_Clause (gnat_array))
       && !Is_Bit_Packed_Array (gnat_array)
       && !Has_Aliased_Components (gnat_array)
-      && !Strict_Alignment (Component_Type (gnat_array))
+      && !Strict_Alignment (gnat_type)
       && TREE_CODE (gnu_type) == RECORD_TYPE
       && !TYPE_FAT_POINTER_P (gnu_type)
       && host_integerp (TYPE_SIZE (gnu_type), 1))
@@ -5301,7 +5302,7 @@  gnat_to_gnu_component_type (Entity_Id gn
 			  debug_info_p, gnat_array);
     }
 
-  if (Has_Volatile_Components (Base_Type (gnat_array)))
+  if (Has_Volatile_Components (gnat_array))
     gnu_type
       = build_qualified_type (gnu_type,
 			      TYPE_QUALS (gnu_type) | TYPE_QUAL_VOLATILE);
@@ -6716,12 +6717,16 @@  static tree
 gnat_to_gnu_field (Entity_Id gnat_field, tree gnu_record_type, int packed,
 		   bool definition, bool debug_info_p)
 {
+  const Entity_Id gnat_field_type = Etype (gnat_field);
+  tree gnu_field_type = gnat_to_gnu_type (gnat_field_type);
   tree gnu_field_id = get_entity_name (gnat_field);
-  tree gnu_field_type = gnat_to_gnu_type (Etype (gnat_field));
   tree gnu_field, gnu_size, gnu_pos;
+  bool is_volatile
+    = (Treat_As_Volatile (gnat_field) || Treat_As_Volatile (gnat_field_type));
   bool needs_strict_alignment
-    = (Is_Aliased (gnat_field) || Strict_Alignment (Etype (gnat_field))
-       || Treat_As_Volatile (gnat_field));
+    = (is_volatile
+       || Is_Aliased (gnat_field)
+       || Strict_Alignment (gnat_field_type));
 
   /* If this field requires strict alignment, we cannot pack it because
      it would very likely be under-aligned in the record.  */
@@ -6737,7 +6742,7 @@  gnat_to_gnu_field (Entity_Id gnat_field,
     gnu_size = validate_size (Esize (gnat_field), gnu_field_type,
 			      gnat_field, FIELD_DECL, false, true);
   else if (packed == 1)
-    gnu_size = validate_size (RM_Size (Etype (gnat_field)), gnu_field_type,
+    gnu_size = validate_size (RM_Size (gnat_field_type), gnu_field_type,
 			      gnat_field, FIELD_DECL, false, true);
   else
     gnu_size = NULL_TREE;
@@ -6829,7 +6834,7 @@  gnat_to_gnu_field (Entity_Id gnat_field,
 	  if (gnu_size
 	      && !operand_equal_p (gnu_size, TYPE_SIZE (gnu_field_type), 0))
 	    {
-	      if (Is_Atomic (gnat_field) || Is_Atomic (Etype (gnat_field)))
+	      if (Is_Atomic (gnat_field) || Is_Atomic (gnat_field_type))
 		post_error_ne_tree
 		  ("atomic field& must be natural size of type{ (^)}",
 		   Last_Bit (Component_Clause (gnat_field)), gnat_field,
@@ -6841,7 +6846,7 @@  gnat_to_gnu_field (Entity_Id gnat_field,
 		   Last_Bit (Component_Clause (gnat_field)), gnat_field,
 		   TYPE_SIZE (gnu_field_type));
 
-	      else if (Strict_Alignment (Etype (gnat_field)))
+	      else if (Strict_Alignment (gnat_field_type))
 		post_error_ne_tree
 		  ("size of & with aliased or tagged components not ^ bits",
 		   Last_Bit (Component_Clause (gnat_field)), gnat_field,
@@ -6854,19 +6859,19 @@  gnat_to_gnu_field (Entity_Id gnat_field,
 			      (TRUNC_MOD_EXPR, gnu_pos,
 			       bitsize_int (TYPE_ALIGN (gnu_field_type)))))
 	    {
-	      if (Is_Aliased (gnat_field))
+	      if (is_volatile)
 		post_error_ne_num
-		  ("position of aliased field& must be multiple of ^ bits",
+		  ("position of volatile field& must be multiple of ^ bits",
 		   First_Bit (Component_Clause (gnat_field)), gnat_field,
 		   TYPE_ALIGN (gnu_field_type));
 
-	      else if (Treat_As_Volatile (gnat_field))
+	      else if (Is_Aliased (gnat_field))
 		post_error_ne_num
-		  ("position of volatile field& must be multiple of ^ bits",
+		  ("position of aliased field& must be multiple of ^ bits",
 		   First_Bit (Component_Clause (gnat_field)), gnat_field,
 		   TYPE_ALIGN (gnu_field_type));
 
-	      else if (Strict_Alignment (Etype (gnat_field)))
+	      else if (Strict_Alignment (gnat_field_type))
 		post_error_ne_num
   ("position of & with aliased or tagged components not multiple of ^ bits",
 		   First_Bit (Component_Clause (gnat_field)), gnat_field,
@@ -6901,7 +6906,7 @@  gnat_to_gnu_field (Entity_Id gnat_field,
   if (TREE_CODE (gnu_field_type) == RECORD_TYPE
       && !gnu_size
       && CONTAINS_PLACEHOLDER_P (TYPE_SIZE (gnu_field_type))
-      && !Is_Constrained (Underlying_Type (Etype (gnat_field))))
+      && !Is_Constrained (Underlying_Type (gnat_field_type)))
     {
       gnu_size = max_size (TYPE_SIZE (gnu_field_type), true);
       packed = 0;
@@ -6953,7 +6958,7 @@  gnat_to_gnu_field (Entity_Id gnat_field,
     = create_field_decl (gnu_field_id, gnu_field_type, gnu_record_type,
 			 gnu_size, gnu_pos, packed, Is_Aliased (gnat_field));
   Sloc_to_locus (Sloc (gnat_field), &DECL_SOURCE_LOCATION (gnu_field));
-  TREE_THIS_VOLATILE (gnu_field) = Treat_As_Volatile (gnat_field);
+  TREE_THIS_VOLATILE (gnu_field) = TREE_SIDE_EFFECTS (gnu_field) = is_volatile;
 
   if (Ekind (gnat_field) == E_Discriminant)
     DECL_DISCRIMINANT_NUMBER (gnu_field)
Index: tree-sra.c
===================================================================
--- tree-sra.c	(revision 175136)
+++ tree-sra.c	(working copy)
@@ -671,8 +671,7 @@  type_internals_preclude_sra_p (tree type
 		    && int_bit_position (fld) % BITS_PER_UNIT != 0))
 	      return true;
 
-	    if (AGGREGATE_TYPE_P (ft)
-		&& type_internals_preclude_sra_p (ft))
+	    if (AGGREGATE_TYPE_P (ft) && type_internals_preclude_sra_p (ft))
 	      return true;
 	  }
 
@@ -681,10 +680,13 @@  type_internals_preclude_sra_p (tree type
     case ARRAY_TYPE:
       et = TREE_TYPE (type);
 
-      if (AGGREGATE_TYPE_P (et))
-	return type_internals_preclude_sra_p (et);
-      else
-	return false;
+      if (TYPE_VOLATILE (et))
+	return true;
+
+      if (AGGREGATE_TYPE_P (et) && type_internals_preclude_sra_p (et))
+	return true;
+
+      return false;
 
     default:
       return false;