diff mbox series

[Ada] Fix wrong code with small array in packed record

Message ID 4190051.ZsBJIy5eaQ@polaris
State New
Headers show
Series [Ada] Fix wrong code with small array in packed record | expand

Commit Message

Eric Botcazou June 29, 2019, 7:17 a.m. UTC
This fixes a long-standing issue with small arrays that end up being not 
aligned on byte boundaries in packed record types but whose size still is a 
multiple of a byte.  This case reveals a small loophole between the front-end 
and the middle-end that gigi needs to plug.  There is also a trivial fixlet 
for expr.c to make things work on big-endian architectures too.

Tested on x86-64/Linux, applied on mainline, as obvious for the expr.c hunk.


2019-06-29  Eric Botcazou  <ebotcazou@adacore.com>

	* expr.c (expand_expr_real_1) <BIT_FIELD_REF>: Apply the big-endian
	adjustment for bit-fields to all aggregate types.


2019-06-29  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc-interface/gigi.h (make_packable_type): Remove default value.
	(value_factor_p): Tweak prototype.
	* gcc-interface/decl.c (gnat_to_gnu_entity): Add comment.
	(gnat_to_gnu_component_type): Likewise.
	(gnat_to_gnu_field): Likewise.  Fetch the position of the field earlier
	and simplify the condition under which the type is packed.  Declare
 	local variable is_bitfield.  Pass 1 as max_align to make_packable_type
	if it is set to true.
	(copy_and_substitute_in_layout): Pass 0 to make_packable_type.
	* gcc-interface/utils.c (make_packable_array_type): New function.
	(make_packable_type): Use it to rewrite the type of array field.
	(maybe_pad_type): Pass align parameter to make_packable_type.
	(create_field_decl): Minor tweaks.
	(value_factor_p): Assert that FACTOR is a power of 2 and replace the
	modulo computation by a masking operation.


2019-06-29  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/array35.adb: New test.
	* gnat.dg/array36.adb: Likewise.
diff mbox series

Patch

Index: expr.c
===================================================================
--- expr.c	(revision 272633)
+++ expr.c	(working copy)
@@ -10893,12 +10893,12 @@  expand_expr_real_1 (tree exp, rtx target
 	    if (MEM_P (op0) && REG_P (XEXP (op0, 0)))
 	      mark_reg_pointer (XEXP (op0, 0), MEM_ALIGN (op0));
 
-	    /* If the result has a record type and the extraction is done in
+	    /* If the result has aggregate type and the extraction is done in
 	       an integral mode, then the field may be not aligned on a byte
 	       boundary; in this case, if it has reverse storage order, it
 	       needs to be extracted as a scalar field with reverse storage
 	       order and put back into memory order afterwards.  */
-	    if (TREE_CODE (type) == RECORD_TYPE
+	    if (AGGREGATE_TYPE_P (type)
 		&& GET_MODE_CLASS (ext_mode) == MODE_INT)
 	      reversep = TYPE_REVERSE_STORAGE_ORDER (type);
 
@@ -10908,13 +10908,13 @@  expand_expr_real_1 (tree exp, rtx target
 				      ? NULL_RTX : target),
 				     ext_mode, ext_mode, reversep, alt_rtl);
 
-	    /* If the result has a record type and the mode of OP0 is an
+	    /* If the result has aggregate type and the mode of OP0 is an
 	       integral mode then, if BITSIZE is narrower than this mode
 	       and this is for big-endian data, we must put the field
 	       into the high-order bits.  And we must also put it back
 	       into memory order if it has been previously reversed.  */
 	    scalar_int_mode op0_mode;
-	    if (TREE_CODE (type) == RECORD_TYPE
+	    if (AGGREGATE_TYPE_P (type)
 		&& is_int_mode (GET_MODE (op0), &op0_mode))
 	      {
 		HOST_WIDE_INT size = GET_MODE_BITSIZE (op0_mode);
Index: ada/gcc-interface/decl.c
===================================================================
--- ada/gcc-interface/decl.c	(revision 272637)
+++ ada/gcc-interface/decl.c	(working copy)
@@ -4481,6 +4481,7 @@  gnat_to_gnu_entity (Entity_Id gnat_entit
 		}
 	    }
 
+      /* Now check if the type allows atomic access.  */
       if (Is_Atomic_Or_VFA (gnat_entity))
 	check_ok_for_atomic_type (gnu_type, gnat_entity, false);
 
@@ -5100,6 +5101,7 @@  gnat_to_gnu_component_type (Entity_Id gn
 	}
     }
 
+  /* Now check if the type of the component allows atomic access.  */
   if (Has_Atomic_Components (gnat_array) || Is_Atomic_Or_VFA (gnat_type))
     check_ok_for_atomic_type (gnu_type, gnat_array, true);
 
@@ -6901,6 +6903,7 @@  gnat_to_gnu_field (Entity_Id gnat_field,
      boundaries, but that should be guaranteed by the GCC memory model.  */
   const bool needs_strict_alignment
     = (is_atomic || is_aliased || is_independent || is_strict_alignment);
+  bool is_bitfield;
   tree gnu_field_type = gnat_to_gnu_type (gnat_field_type);
   tree gnu_field_id = get_entity_name (gnat_field);
   tree gnu_field, gnu_size, gnu_pos;
@@ -6915,7 +6918,7 @@  gnat_to_gnu_field (Entity_Id gnat_field,
   /* If a size is specified, use it.  Otherwise, if the record type is packed,
      use the official RM size.  See "Handling of Type'Size Values" in Einfo
      for further details.  */
-  if (Known_Esize (gnat_field) || Present (gnat_clause))
+  if (Present (gnat_clause) || Known_Esize (gnat_field))
     gnu_size = validate_size (Esize (gnat_field), gnu_field_type, gnat_field,
 			      FIELD_DECL, false, true);
   else if (packed == 1)
@@ -6927,12 +6930,36 @@  gnat_to_gnu_field (Entity_Id gnat_field,
   else
     gnu_size = NULL_TREE;
 
-  /* If we have a specified size that is smaller than that of the field's type,
-     or a position is specified, and the field's type is a record that doesn't
-     require strict alignment, see if we can get either an integral mode form
-     of the type or a smaller form.  If we can, show a size was specified for
-     the field if there wasn't one already, so we know to make this a bitfield
-     and avoid making things wider.
+  /* Likewise for the position.  */
+  if (Present (gnat_clause))
+    {
+      gnu_pos = UI_To_gnu (Component_Bit_Offset (gnat_field), bitsizetype);
+      is_bitfield = !value_factor_p (gnu_pos, BITS_PER_UNIT);
+    }
+
+  /* If the record has rep clauses and this is the tag field, make a rep
+     clause for it as well.  */
+  else if (Has_Specified_Layout (gnat_record_type)
+	   && Chars (gnat_field) == Name_uTag)
+    {
+      gnu_pos = bitsize_zero_node;
+      gnu_size = TYPE_SIZE (gnu_field_type);
+      is_bitfield = false;
+    }
+
+  else
+    {
+      gnu_pos = NULL_TREE;
+      is_bitfield = false;
+    }
+
+  /* If the field's type is a fixed-size record that does not require strict
+     alignment, and the record is packed or we have a position specified for
+     the field that makes it a bitfield or we have a specified size that is
+     smaller than that of the field's type, then see if we can get either an
+     integral mode form of the field's type or a smaller form.  If we can,
+     consider that a size was specified for the field if there wasn't one
+     already, so we know to make it a bitfield and avoid making things wider.
 
      Changing to an integral mode form is useful when the record is packed as
      we can then place the field at a non-byte-aligned position and so achieve
@@ -6954,14 +6981,12 @@  gnat_to_gnu_field (Entity_Id gnat_field,
       && !TYPE_FAT_POINTER_P (gnu_field_type)
       && tree_fits_uhwi_p (TYPE_SIZE (gnu_field_type))
       && (packed == 1
+	  || is_bitfield
 	  || (gnu_size
-	      && (tree_int_cst_lt (gnu_size, TYPE_SIZE (gnu_field_type))
-		  || (Present (gnat_clause)
-		      && !(UI_To_Int (Component_Bit_Offset (gnat_field))
-			   % BITS_PER_UNIT == 0
-			   && value_factor_p (gnu_size, BITS_PER_UNIT)))))))
+	      && tree_int_cst_lt (gnu_size, TYPE_SIZE (gnu_field_type)))))
     {
-      tree gnu_packable_type = make_packable_type (gnu_field_type, true);
+      tree gnu_packable_type
+	= make_packable_type (gnu_field_type, true, is_bitfield ? 1 : 0);
       if (gnu_packable_type != gnu_field_type)
 	{
 	  gnu_field_type = gnu_packable_type;
@@ -6970,6 +6995,7 @@  gnat_to_gnu_field (Entity_Id gnat_field,
 	}
     }
 
+  /* Now check if the type of the field allows atomic access.  */
   if (Is_Atomic_Or_VFA (gnat_field))
     {
       const unsigned int align
@@ -6981,12 +7007,11 @@  gnat_to_gnu_field (Entity_Id gnat_field,
       check_ok_for_atomic_type (gnu_field_type, gnat_field, false);
     }
 
-  if (Present (gnat_clause))
+  /* If a position is specified, check that it is valid.  */
+  if (gnu_pos)
     {
       Entity_Id gnat_parent = Parent_Subtype (gnat_record_type);
 
-      gnu_pos = UI_To_gnu (Component_Bit_Offset (gnat_field), bitsizetype);
-
       /* Ensure the position does not overlap with the parent subtype, if there
 	 is one.  This test is omitted if the parent of the tagged type has a
 	 full rep clause since, in this case, component clauses are allowed to
@@ -7092,19 +7117,8 @@  gnat_to_gnu_field (Entity_Id gnat_field,
 	}
     }
 
-  /* If the record has rep clauses and this is the tag field, make a rep
-     clause for it as well.  */
-  else if (Has_Specified_Layout (gnat_record_type)
-	   && Chars (gnat_field) == Name_uTag)
-    {
-      gnu_pos = bitsize_zero_node;
-      gnu_size = TYPE_SIZE (gnu_field_type);
-    }
-
   else
     {
-      gnu_pos = NULL_TREE;
-
       /* If we are packing the record and the field is BLKmode, round the
 	 size up to a byte boundary.  */
       if (packed && TYPE_MODE (gnu_field_type) == BLKmode && gnu_size)
@@ -9681,7 +9695,7 @@  copy_and_substitute_in_layout (Entity_Id
 	    if (RECORD_OR_UNION_TYPE_P (gnu_field_type)
 		&& !TYPE_FAT_POINTER_P (gnu_field_type)
 		&& tree_fits_uhwi_p (TYPE_SIZE (gnu_field_type)))
-	      gnu_field_type = make_packable_type (gnu_field_type, true);
+	      gnu_field_type = make_packable_type (gnu_field_type, true, 0);
 	  }
 
 	else
Index: ada/gcc-interface/gigi.h
===================================================================
--- ada/gcc-interface/gigi.h	(revision 272637)
+++ ada/gcc-interface/gigi.h	(working copy)
@@ -126,7 +126,7 @@  extern tree make_aligning_type (tree typ
    MAX_ALIGN alignment if the value is non-zero.  If so, return the new
    type; if not, return the original type.  */
 extern tree make_packable_type (tree type, bool in_record,
-				unsigned int max_align = 0);
+				unsigned int max_align);
 
 /* Given a type TYPE, return a new type whose size is appropriate for SIZE.
    If TYPE is the best type, return it.  Otherwise, make a new type.  We
@@ -837,7 +837,7 @@  extern unsigned int known_alignment (tre
 
 /* Return true if VALUE is a multiple of FACTOR. FACTOR must be a power
    of 2.  */
-extern bool value_factor_p (tree value, HOST_WIDE_INT factor);
+extern bool value_factor_p (tree value, unsigned HOST_WIDE_INT factor);
 
 /* Build an atomic load for the underlying atomic object in SRC.  SYNC is
    true if the load requires synchronization.  */
Index: ada/gcc-interface/utils.c
===================================================================
--- ada/gcc-interface/utils.c	(revision 272633)
+++ ada/gcc-interface/utils.c	(working copy)
@@ -984,10 +984,45 @@  make_aligning_type (tree type, unsigned
   return record_type;
 }
 
+/* TYPE is an ARRAY_TYPE that is being used as the type of a field in a packed
+   record.  See if we can rewrite it as a type that has non-BLKmode, which we
+   can pack tighter in the packed record.  If so, return the new type; if not,
+   return the original type.  */
+
+static tree
+make_packable_array_type (tree type)
+{
+  const unsigned HOST_WIDE_INT size = tree_to_uhwi (TYPE_SIZE (type));
+  unsigned HOST_WIDE_INT new_size;
+  unsigned int new_align;
+
+  /* No point in doing anything if the size is either zero or too large for an
+     integral mode, or if the type already has non-BLKmode.  */
+  if (size == 0 || size > MAX_FIXED_MODE_SIZE || TYPE_MODE (type) != BLKmode)
+    return type;
+
+  /* Punt if the component type is an aggregate type for now.  */
+  if (AGGREGATE_TYPE_P (TREE_TYPE (type)))
+    return type;
+
+  tree new_type = copy_type (type);
+
+  new_size = ceil_pow2 (size);
+  new_align = MIN (new_size, BIGGEST_ALIGNMENT);
+  SET_TYPE_ALIGN (new_type, new_align);
+
+  TYPE_SIZE (new_type) = bitsize_int (new_size);
+  TYPE_SIZE_UNIT (new_type) = size_int (new_size / BITS_PER_UNIT);
+
+  SET_TYPE_MODE (new_type, mode_for_size (new_size, MODE_INT, 1).else_blk ());
+
+  return new_type;
+}
+
 /* TYPE is a RECORD_TYPE, UNION_TYPE or QUAL_UNION_TYPE that is being used
-   as the field type of a packed record if IN_RECORD is true, or as the
-   component type of a packed array if IN_RECORD is false.  See if we can
-   rewrite it either as a type that has non-BLKmode, which we can pack
+   as the type of a field in a packed record if IN_RECORD is true, or as
+   the component type of a packed array if IN_RECORD is false.  See if we
+   can rewrite it either as a type that has non-BLKmode, which we can pack
    tighter in the packed record case, or as a smaller type with at most
    MAX_ALIGN alignment if the value is non-zero.  If so, return the new
    type; if not, return the original type.  */
@@ -995,9 +1030,9 @@  make_aligning_type (tree type, unsigned
 tree
 make_packable_type (tree type, bool in_record, unsigned int max_align)
 {
-  unsigned HOST_WIDE_INT size = tree_to_uhwi (TYPE_SIZE (type));
+  const unsigned HOST_WIDE_INT size = tree_to_uhwi (TYPE_SIZE (type));
+  const unsigned int align = TYPE_ALIGN (type);
   unsigned HOST_WIDE_INT new_size;
-  unsigned int align = TYPE_ALIGN (type);
   unsigned int new_align;
 
   /* No point in doing anything if the size is zero.  */
@@ -1058,10 +1093,19 @@  make_packable_type (tree type, bool in_r
       tree new_field_type = TREE_TYPE (field);
       tree new_field, new_field_size;
 
-      if (RECORD_OR_UNION_TYPE_P (new_field_type)
-	  && !TYPE_FAT_POINTER_P (new_field_type)
+      if (AGGREGATE_TYPE_P (new_field_type)
 	  && tree_fits_uhwi_p (TYPE_SIZE (new_field_type)))
-	new_field_type = make_packable_type (new_field_type, true, max_align);
+	{
+	  if (RECORD_OR_UNION_TYPE_P (new_field_type)
+	      && !TYPE_FAT_POINTER_P (new_field_type))
+	    new_field_type
+	      = make_packable_type (new_field_type, true, max_align);
+	  else if (in_record
+		   && max_align > 0
+		   && max_align < BITS_PER_UNIT
+		   && TREE_CODE (new_field_type) == ARRAY_TYPE)
+	    new_field_type = make_packable_array_type (new_field_type);
+	}
 
       /* However, for the last field in a not already packed record type
 	 that is of an aggregate type, we need to use the RM size in the
@@ -1411,7 +1455,7 @@  maybe_pad_type (tree type, tree size, un
      different modes, a VIEW_CONVERT_EXPR will be required for converting
      between them and it might be hard to overcome afterwards, including
      at the RTL level when the stand-alone object is accessed as a whole.  */
-  if (align != 0
+  if (align > 0
       && RECORD_OR_UNION_TYPE_P (type)
       && TYPE_MODE (type) == BLKmode
       && !TYPE_BY_REFERENCE_P (type)
@@ -1422,7 +1466,7 @@  maybe_pad_type (tree type, tree size, un
 	  || (TREE_CODE (size) == INTEGER_CST
 	      && compare_tree_int (size, MAX_FIXED_MODE_SIZE) <= 0)))
     {
-      tree packable_type = make_packable_type (type, true);
+      tree packable_type = make_packable_type (type, true, align);
       if (TYPE_MODE (packable_type) != BLKmode
 	  && align >= TYPE_ALIGN (packable_type))
         type = packable_type;
@@ -2790,10 +2834,9 @@  create_field_decl (tree name, tree type,
 	size = round_up (size, BITS_PER_UNIT);
     }
 
-  /* If we may, according to ADDRESSABLE, make a bitfield when the size is
-     specified for two reasons: first if the size differs from the natural
-     size; second, if the alignment is insufficient.  There are a number of
-     ways the latter can be true.
+  /* If we may, according to ADDRESSABLE, then make a bitfield when the size
+     is specified for two reasons: first, when it differs from the natural
+     size; second, when the alignment is insufficient.
 
      We never make a bitfield if the type of the field has a nonconstant size,
      because no such entity requiring bitfield operations should reach here.
@@ -2809,17 +2852,17 @@  create_field_decl (tree name, tree type,
       && size
       && TREE_CODE (size) == INTEGER_CST
       && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST
-      && (!tree_int_cst_equal (size, TYPE_SIZE (type))
+      && (packed
+	  || !tree_int_cst_equal (size, TYPE_SIZE (type))
 	  || (pos && !value_factor_p (pos, TYPE_ALIGN (type)))
-	  || packed
-	  || (TYPE_ALIGN (record_type) != 0
+	  || (TYPE_ALIGN (record_type)
 	      && TYPE_ALIGN (record_type) < TYPE_ALIGN (type))))
     {
       DECL_BIT_FIELD (field_decl) = 1;
       DECL_SIZE (field_decl) = size;
       if (!packed && !pos)
 	{
-	  if (TYPE_ALIGN (record_type) != 0
+	  if (TYPE_ALIGN (record_type)
 	      && TYPE_ALIGN (record_type) < TYPE_ALIGN (type))
 	    SET_DECL_ALIGN (field_decl, TYPE_ALIGN (record_type));
 	  else
@@ -3001,10 +3044,12 @@  process_attributes (tree *node, struct a
    a power of 2. */
 
 bool
-value_factor_p (tree value, HOST_WIDE_INT factor)
+value_factor_p (tree value, unsigned HOST_WIDE_INT factor)
 {
+  gcc_checking_assert (pow2p_hwi (factor));
+
   if (tree_fits_uhwi_p (value))
-    return tree_to_uhwi (value) % factor == 0;
+    return (tree_to_uhwi (value) & (factor - 1)) == 0;
 
   if (TREE_CODE (value) == MULT_EXPR)
     return (value_factor_p (TREE_OPERAND (value, 0), factor)