diff mbox series

[Ada] Pragma Atomic wrongly rejected on composite component

Message ID 1672448.CyEKtT8OQq@polaris
State New
Headers show
Series [Ada] Pragma Atomic wrongly rejected on composite component | expand

Commit Message

Eric Botcazou Sept. 9, 2017, 12:30 p.m. UTC 
The compiler wrongly rejects a pragma Atomic on a component of a record whose 
type is composite, but it accepts the pragma on a variable of the same type.

Tested on x86_64-suse-linux, applied on the mainline and 7 branch.


2017-09-09  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc-interface/decl.c (promote_object_alignment): New function taken
	from...
	(gnat_to_gnu_entity) <E_Variable>: ...here.  Invoke it.
	(gnat_to_gnu_field): If the field is Atomic or VFA, invoke it and
	create a padding type on success before doing the atomic check.


2017-09-09  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/specs/atomic3.ads: New test.

Comments

Andreas Schwab Sept. 11, 2017, 6:35 p.m. UTC | #1 
On Sep 09 2017, Eric Botcazou <ebotcazou@adacore.com> wrote:

> 	* gcc-interface/decl.c (promote_object_alignment): New function taken
> 	from...
> 	(gnat_to_gnu_entity) <E_Variable>: ...here.  Invoke it.
> 	(gnat_to_gnu_field): If the field is Atomic or VFA, invoke it and
> 	create a padding type on success before doing the atomic check.

That regresses on gnat.dg/specs/atomic1.ads for aarch64/-mabi=ilp32,
missing the error on line 13.  The missing error on line 9 is
preexisting.

Andreas.
Eric Botcazou Sept. 11, 2017, 7:49 p.m. UTC | #2 
> That regresses on gnat.dg/specs/atomic1.ads for aarch64/-mabi=ilp32,
> missing the error on line 13.  The missing error on line 9 is
> preexisting.

That's sort of expected, since the point of the patch is to make the 2 
situations equivalent wrt atomicity.  Clearly the test is not portable and 
will fail on targets for which pointer size and word size are not equal.
diff mbox series

Patch

Index: gcc-interface/decl.c
===================================================================
--- gcc-interface/decl.c	(revision 251929)
+++ gcc-interface/decl.c	(working copy)
@@ -230,6 +230,7 @@  static vec<variant_desc> build_variant_l
 static tree validate_size (Uint, tree, Entity_Id, enum tree_code, bool, bool);
 static void set_rm_size (Uint, tree, Entity_Id);
 static unsigned int validate_alignment (Uint, Entity_Id, unsigned int);
+static unsigned int promote_object_alignment (tree, Entity_Id);
 static void check_ok_for_atomic_type (tree, Entity_Id, bool);
 static tree create_field_decl_from (tree, tree, tree, tree, tree,
 				    vec<subst_pair>);
@@ -856,45 +857,7 @@  gnat_to_gnu_entity (Entity_Id gnat_entit
 		    && No (Renamed_Object (gnat_entity))
 		    && No (Address_Clause (gnat_entity))))
 	    && TREE_CODE (TYPE_SIZE (gnu_type)) == INTEGER_CST)
-	  {
-	    unsigned int size_cap, align_cap;
-
-	    /* No point in promoting the alignment if this doesn't prevent
-	       BLKmode access to the object, in particular block copy, as
-	       this will for example disable the NRV optimization for it.
-	       No point in jumping through all the hoops needed in order
-	       to support BIGGEST_ALIGNMENT if we don't really have to.
-	       So we cap to the smallest alignment that corresponds to
-	       a known efficient memory access pattern of the target.  */
-	    if (Is_Atomic_Or_VFA (gnat_entity))
-	      {
-		size_cap = UINT_MAX;
-		align_cap = BIGGEST_ALIGNMENT;
-	      }
-	    else
-	      {
-		size_cap = MAX_FIXED_MODE_SIZE;
-		align_cap = get_mode_alignment (ptr_mode);
-	      }
-
-	    if (!tree_fits_uhwi_p (TYPE_SIZE (gnu_type))
-		|| compare_tree_int (TYPE_SIZE (gnu_type), size_cap) > 0)
-	      align = 0;
-	    else if (compare_tree_int (TYPE_SIZE (gnu_type), align_cap) > 0)
-	      align = align_cap;
-	    else
-	      align = ceil_pow2 (tree_to_uhwi (TYPE_SIZE (gnu_type)));
-
-	    /* But make sure not to under-align the object.  */
-	    if (align <= TYPE_ALIGN (gnu_type))
-	      align = 0;
-
-	    /* And honor the minimum valid atomic alignment, if any.  */
-#ifdef MINIMUM_ATOMIC_ALIGNMENT
-	    else if (align < MINIMUM_ATOMIC_ALIGNMENT)
-	      align = MINIMUM_ATOMIC_ALIGNMENT;
-#endif
-	  }
+	  align = promote_object_alignment (gnu_type, gnat_entity);
 
 	/* If the object is set to have atomic components, find the component
 	   type and validate it.
@@ -6891,7 +6854,15 @@  gnat_to_gnu_field (Entity_Id gnat_field,
     }
 
   if (Is_Atomic_Or_VFA (gnat_field))
-    check_ok_for_atomic_type (gnu_field_type, gnat_field, false);
+    {
+      const unsigned int align
+	= promote_object_alignment (gnu_field_type, gnat_field);
+      if (align > 0)
+	gnu_field_type
+	  = maybe_pad_type (gnu_field_type, NULL_TREE, align, gnat_field,
+			    false, false, definition, true);
+      check_ok_for_atomic_type (gnu_field_type, gnat_field, false);
+    }
 
   if (Present (Component_Clause (gnat_field)))
     {
@@ -8807,6 +8778,53 @@  validate_alignment (Uint alignment, Enti
 
   return align;
 }
+
+/* Promote the alignment of GNU_TYPE corresponding to GNAT_ENTITY.  Return
+   a positive value on success or zero on failure.  */
+
+static unsigned int
+promote_object_alignment (tree gnu_type, Entity_Id gnat_entity)
+{
+  unsigned int align, size_cap, align_cap;
+
+  /* No point in promoting the alignment if this doesn't prevent BLKmode access
+     to the object, in particular block copy, as this will for example disable
+     the NRV optimization for it.  No point in jumping through all the hoops
+     needed in order to support BIGGEST_ALIGNMENT if we don't really have to.
+     So we cap to the smallest alignment that corresponds to a known efficient
+     memory access pattern, except for Atomic and Volatile_Full_Access.  */
+  if (Is_Atomic_Or_VFA (gnat_entity))
+    {
+      size_cap = UINT_MAX;
+      align_cap = BIGGEST_ALIGNMENT;
+    }
+  else
+    {
+      size_cap = MAX_FIXED_MODE_SIZE;
+      align_cap = get_mode_alignment (ptr_mode);
+    }
+
+  /* Do the promotion within the above limits.  */
+  if (!tree_fits_uhwi_p (TYPE_SIZE (gnu_type))
+      || compare_tree_int (TYPE_SIZE (gnu_type), size_cap) > 0)
+    align = 0;
+  else if (compare_tree_int (TYPE_SIZE (gnu_type), align_cap) > 0)
+    align = align_cap;
+  else
+    align = ceil_pow2 (tree_to_uhwi (TYPE_SIZE (gnu_type)));
+
+  /* But make sure not to under-align the object.  */
+  if (align <= TYPE_ALIGN (gnu_type))
+    align = 0;
+
+   /* And honor the minimum valid atomic alignment, if any.  */
+#ifdef MINIMUM_ATOMIC_ALIGNMENT
+  else if (align < MINIMUM_ATOMIC_ALIGNMENT)
+    align = MINIMUM_ATOMIC_ALIGNMENT;
+#endif
+
+  return align;
+}
 
 /* Verify that TYPE is something we can implement atomically.  If not, issue
    an error for GNAT_ENTITY.  COMPONENT_P is true if we are being called to