[Arm] Account for C++17 artificial field determining Homogeneous Aggregates

Matthew Malcomson matthew.malcomson@arm.com
Mon Apr 27 15:31:54 GMT 2020


NOTE:
  There is another patch in the making to handle the APCS abi (selected with
  `-mabi=apcs-gnu`).  That patch has a very different change in behaviour and
  is in a different part of the code so I'm keeping it in a different patch.

In C++14, an empty class deriving from an empty base is not an
aggregate, while in C++17 it is.  In order to implement this, GCC adds
an artificial field to such classes.

This artificial field has no mapping to Fundamental Data Types in the
Arm PCS ABI and hence should not count towards determining whether an
object can be passed using the vector registers as per section
"7.1.2 Procedure Calling" in the arm PCS
https://developer.arm.com/docs/ihi0042/latest?_ga=2.60211309.1506853196.1533541889-405231439.1528186050

This patch avoids counting this artificial field in
aapcs_vfp_sub_candidate, and hence calculates whether such objects
should be passed in vector registers in the same manner as C++14 (where
the artificial field does not exist).

Before this change, the test below would pass the arguments to `f` in
general registers.  After this change, the test passes the arguments to
`f` using the vector registers.

The new behaviour matches the behaviour of `armclang`, and also matches
the GCC behaviour when run with `-std=gnu++14`.

> gcc -std=gnu++17 -march=armv8-a+simd -mfloat-abi=hard test.cpp

``` test.cpp
struct base {};

struct pair : base
{
  float first;
  float second;
  pair (float f, float s) : first(f), second(s) {}
};

void f (pair);
int main()
{
  f({3.14, 666});
  return 1;
}
```

We add a `-Wpsabi` warning to catch cases where this fix has changed the ABI for
some functions.  Unfortunately this warning is not emitted twice for multiple
calls to the same function, but I feel this is not much of a problem and can be
fixed later if needs be.

(i.e. if `main` called `f` twice in a row we only emit a diagnostic for the
first).

Testing:
    Bootstrapped and regression tested on arm-linux.
    This change fixes the struct-layout-1 tests Jakub added
    https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544204.html
    Regression tested on arm-none-eabi.

gcc/ChangeLog:

2020-04-27  Matthew Malcomson  <matthew.malcomson@arm.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR target/94383
	* config/arm/arm.c (aapcs_vfp_sub_candidate): Account for C++17 empty
	base class artificial fields.
	(aapcs_vfp_is_call_or_return_candidate): Warn when PCS ABI
	decision is different after this fix.



###############     Attachment also inlined for ease of reply    ###############


diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 0151bda90d961ae1a001c61cd5e94d6ec67e3aea..0db444c3fdb2ba6fb7ebad410310fb5b1bc0e304 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6139,9 +6139,19 @@ aapcs_vfp_cum_init (CUMULATIVE_ARGS *pcum  ATTRIBUTE_UNUSED,
    If *MODEP is VOIDmode, then set it to the first valid floating point
    type.  If a non-floating point type is found, or if a floating point
    type that doesn't match a non-VOIDmode *MODEP is found, then return -1,
-   otherwise return the count in the sub-tree.  */
+   otherwise return the count in the sub-tree.
+
+   The AVOID_CXX17_EMPTY_BASE argument is to allow the caller to check whether
+   this function has changed its behavior after the fix for PR94384 -- this fix
+   is to avoid artificial fields in empty base classes.
+   When called with this argument as a NULL pointer this function does not
+   avoid the artificial fields -- this is useful to check whether the function
+   returns something different after the fix.
+   When called pointing at a value, this function avoids such artificial fields
+   and sets the value to TRUE when one of these fields has been set.  */
 static int
-aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep)
+aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep,
+			 bool *avoid_cxx17_empty_base)
 {
   machine_mode mode;
   HOST_WIDE_INT size;
@@ -6213,7 +6223,8 @@ aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep)
 	    || TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST)
 	  return -1;
 
-	count = aapcs_vfp_sub_candidate (TREE_TYPE (type), modep);
+	count = aapcs_vfp_sub_candidate (TREE_TYPE (type), modep,
+					 avoid_cxx17_empty_base);
 	if (count == -1
 	    || !index
 	    || !TYPE_MAX_VALUE (index)
@@ -6251,7 +6262,18 @@ aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep)
 	    if (TREE_CODE (field) != FIELD_DECL)
 	      continue;
 
-	    sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep);
+	    /* Ignore C++17 empty base fields, while their type indicates
+	       they do contain padding, they have zero size and thus don't
+	       contain any padding.  */
+	    if (cxx17_empty_base_field_p (field)
+		&& avoid_cxx17_empty_base)
+	      {
+		*avoid_cxx17_empty_base = true;
+		continue;
+	      }
+
+	    sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep,
+						 avoid_cxx17_empty_base);
 	    if (sub_count < 0)
 	      return -1;
 	    count += sub_count;
@@ -6284,7 +6306,8 @@ aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep)
 	    if (TREE_CODE (field) != FIELD_DECL)
 	      continue;
 
-	    sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep);
+	    sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep,
+						 avoid_cxx17_empty_base);
 	    if (sub_count < 0)
 	      return -1;
 	    count = count > sub_count ? count : sub_count;
@@ -6346,10 +6369,27 @@ aapcs_vfp_is_call_or_return_candidate (enum arm_pcs pcs_variant,
      out from the mode.  */
   if (type)
     {
-      int ag_count = aapcs_vfp_sub_candidate (type, &new_mode);
-
+      bool avoided = false;
+      int ag_count = aapcs_vfp_sub_candidate (type, &new_mode, &avoided);
       if (ag_count > 0 && ag_count <= 4)
-	*count = ag_count;
+	{
+	  static unsigned last_reported_type_uid;
+	  unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
+	  int alt;
+	  if (warn_psabi
+	      && avoided
+	      && uid != last_reported_type_uid
+	      && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL))
+		  != ag_count))
+	    {
+	      gcc_assert (alt == -1);
+	      last_reported_type_uid = uid;
+	      inform (input_location, "parameter passing for argument of type "
+		      "%qT when C++17 is enabled changed to match C++14 "
+		      "in GCC 10.1", type);
+	    }
+	  *count = ag_count;
+	}
       else
 	return false;
     }

-------------- next part --------------
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 0151bda90d961ae1a001c61cd5e94d6ec67e3aea..0db444c3fdb2ba6fb7ebad410310fb5b1bc0e304 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6139,9 +6139,19 @@ aapcs_vfp_cum_init (CUMULATIVE_ARGS *pcum  ATTRIBUTE_UNUSED,
    If *MODEP is VOIDmode, then set it to the first valid floating point
    type.  If a non-floating point type is found, or if a floating point
    type that doesn't match a non-VOIDmode *MODEP is found, then return -1,
-   otherwise return the count in the sub-tree.  */
+   otherwise return the count in the sub-tree.
+
+   The AVOID_CXX17_EMPTY_BASE argument is to allow the caller to check whether
+   this function has changed its behavior after the fix for PR94384 -- this fix
+   is to avoid artificial fields in empty base classes.
+   When called with this argument as a NULL pointer this function does not
+   avoid the artificial fields -- this is useful to check whether the function
+   returns something different after the fix.
+   When called pointing at a value, this function avoids such artificial fields
+   and sets the value to TRUE when one of these fields has been set.  */
 static int
-aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep)
+aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep,
+			 bool *avoid_cxx17_empty_base)
 {
   machine_mode mode;
   HOST_WIDE_INT size;
@@ -6213,7 +6223,8 @@ aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep)
 	    || TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST)
 	  return -1;
 
-	count = aapcs_vfp_sub_candidate (TREE_TYPE (type), modep);
+	count = aapcs_vfp_sub_candidate (TREE_TYPE (type), modep,
+					 avoid_cxx17_empty_base);
 	if (count == -1
 	    || !index
 	    || !TYPE_MAX_VALUE (index)
@@ -6251,7 +6262,18 @@ aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep)
 	    if (TREE_CODE (field) != FIELD_DECL)
 	      continue;
 
-	    sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep);
+	    /* Ignore C++17 empty base fields, while their type indicates
+	       they do contain padding, they have zero size and thus don't
+	       contain any padding.  */
+	    if (cxx17_empty_base_field_p (field)
+		&& avoid_cxx17_empty_base)
+	      {
+		*avoid_cxx17_empty_base = true;
+		continue;
+	      }
+
+	    sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep,
+						 avoid_cxx17_empty_base);
 	    if (sub_count < 0)
 	      return -1;
 	    count += sub_count;
@@ -6284,7 +6306,8 @@ aapcs_vfp_sub_candidate (const_tree type, machine_mode *modep)
 	    if (TREE_CODE (field) != FIELD_DECL)
 	      continue;
 
-	    sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep);
+	    sub_count = aapcs_vfp_sub_candidate (TREE_TYPE (field), modep,
+						 avoid_cxx17_empty_base);
 	    if (sub_count < 0)
 	      return -1;
 	    count = count > sub_count ? count : sub_count;
@@ -6346,10 +6369,27 @@ aapcs_vfp_is_call_or_return_candidate (enum arm_pcs pcs_variant,
      out from the mode.  */
   if (type)
     {
-      int ag_count = aapcs_vfp_sub_candidate (type, &new_mode);
-
+      bool avoided = false;
+      int ag_count = aapcs_vfp_sub_candidate (type, &new_mode, &avoided);
       if (ag_count > 0 && ag_count <= 4)
-	*count = ag_count;
+	{
+	  static unsigned last_reported_type_uid;
+	  unsigned uid = TYPE_UID (TYPE_MAIN_VARIANT (type));
+	  int alt;
+	  if (warn_psabi
+	      && avoided
+	      && uid != last_reported_type_uid
+	      && ((alt = aapcs_vfp_sub_candidate (type, &new_mode, NULL))
+		  != ag_count))
+	    {
+	      gcc_assert (alt == -1);
+	      last_reported_type_uid = uid;
+	      inform (input_location, "parameter passing for argument of type "
+		      "%qT when C++17 is enabled changed to match C++14 "
+		      "in GCC 10.1", type);
+	    }
+	  *count = ag_count;
+	}
       else
 	return false;
     }



More information about the Gcc-patches mailing list