On Wed, Apr 22, 2020 at 05:10:56PM -0500, Segher Boessenkool wrote:
> >     PR target/94707
> >     * config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Add
> >     CXX17_EMPTY_BASE_SEEN argument.  Pass it to recursive calls.
> >     Ignore cxx17_empty_base_field_p fields after setting
> >     *CXX17_EMPTY_BASE_SEEN to true.
> 
> Please use the literal capitalisation of symbol names?  So that grep and
> other search works (the ALL CAPS thing is for the function introductory
> comment (and other function comments) only).

Done.

> > +       if (cxx17_empty_base_field_p (field))
> > +         {
> > +           *cxx17_empty_base_seen = true;
> > +           continue;
> > +         }
> 
> Is there no way to describe this without referring to "c++17" (or even
> "base field")?  It's a pretty gross abstraction violation.

I'm afraid it is desirable to talk about c++17 and base field, otherwise
it won't be clear what we mean.

> > +                 inform (input_location,
> > +                         "prior to GCC 10, parameters of type "
> > +                         "%qT were passed incorrectly for C++17", type);
> 
> This could more explicitly say that makes the compiled code incompatible
> between GCC 10 and older, which is the point of the warning?  It's not
> really necessary to say the old one was bad -- let's hope it was :-)

After many iterations on IRC we came up with the following wording, which is
what I've committed.  Might be nice to also have URL with larger explanation
in changes.html. but the diagnostic framework doesn't support that yet.

2020-04-23  Jakub Jelinek  <ja...@redhat.com>

        PR target/94707
        * config/rs6000/rs6000-call.c (rs6000_aggregate_candidate): Add
        cxx17_empty_base_seen argument.  Pass it to recursive calls.
        Ignore cxx17_empty_base_field_p fields after setting
        *cxx17_empty_base_seen to true.
        (rs6000_discover_homogeneous_aggregate): Adjust
        rs6000_aggregate_candidate caller.  With -Wpsabi, diagnose homogeneous
        aggregates with C++17 empty base fields.

        * g++.dg/tree-ssa/pr27830.C: Use -Wpsabi -w for -std=c++17 and higher.

--- gcc/config/rs6000/rs6000-call.c.jj  2020-03-30 22:53:40.746640328 +0200
+++ gcc/config/rs6000/rs6000-call.c     2020-04-22 13:05:07.947809888 +0200
@@ -5528,7 +5528,8 @@ const struct altivec_builtin_types altiv
    sub-tree.  */
 
 static int
-rs6000_aggregate_candidate (const_tree type, machine_mode *modep)
+rs6000_aggregate_candidate (const_tree type, machine_mode *modep,
+                           bool *cxx17_empty_base_seen)
 {
   machine_mode mode;
   HOST_WIDE_INT size;
@@ -5598,7 +5599,8 @@ rs6000_aggregate_candidate (const_tree t
            || TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST)
          return -1;
 
-       count = rs6000_aggregate_candidate (TREE_TYPE (type), modep);
+       count = rs6000_aggregate_candidate (TREE_TYPE (type), modep,
+                                           cxx17_empty_base_seen);
        if (count == -1
            || !index
            || !TYPE_MAX_VALUE (index)
@@ -5636,7 +5638,14 @@ rs6000_aggregate_candidate (const_tree t
            if (TREE_CODE (field) != FIELD_DECL)
              continue;
 
-           sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep);
+           if (cxx17_empty_base_field_p (field))
+             {
+               *cxx17_empty_base_seen = true;
+               continue;
+             }
+
+           sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep,
+                                                   cxx17_empty_base_seen);
            if (sub_count < 0)
              return -1;
            count += sub_count;
@@ -5669,7 +5678,8 @@ rs6000_aggregate_candidate (const_tree t
            if (TREE_CODE (field) != FIELD_DECL)
              continue;
 
-           sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep);
+           sub_count = rs6000_aggregate_candidate (TREE_TYPE (field), modep,
+                                                   cxx17_empty_base_seen);
            if (sub_count < 0)
              return -1;
            count = count > sub_count ? count : sub_count;
@@ -5710,7 +5720,9 @@ rs6000_discover_homogeneous_aggregate (m
       && AGGREGATE_TYPE_P (type))
     {
       machine_mode field_mode = VOIDmode;
-      int field_count = rs6000_aggregate_candidate (type, &field_mode);
+      bool cxx17_empty_base_seen = false;
+      int field_count = rs6000_aggregate_candidate (type, &field_mode,
+                                                   &cxx17_empty_base_seen);
 
       if (field_count > 0)
        {
@@ -5725,6 +5737,18 @@ rs6000_discover_homogeneous_aggregate (m
                *elt_mode = field_mode;
              if (n_elts)
                *n_elts = field_count;
+             if (cxx17_empty_base_seen && warn_psabi)
+               {
+                 static const_tree last_reported_type;
+                 if (type != last_reported_type)
+                   {
+                     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);
+                     last_reported_type = type;
+                   }
+               }
              return true;
            }
        }
--- gcc/testsuite/g++.dg/tree-ssa/pr27830.C.jj  2020-01-11 16:31:54.844296359 
+0100
+++ gcc/testsuite/g++.dg/tree-ssa/pr27830.C     2020-04-22 15:01:37.949881630 
+0200
@@ -1,5 +1,7 @@
 /* { dg-do compile } */
 /* { dg-options "-O" } */
+/* Ignore ABI warnings for C++17 and later.  */
+/* { dg-additional-options "-Wno-psabi -w" { target c++17 } } */
 
 struct gc{};
 struct transform:public gc


        Jakub

Reply via email to