[PATCH] c++: Fix up attribute handling in methods in templates [PR100872]

[PATCH] c++: Fix up attribute handling in methods in templates [PR100872]

[Jakub Jelinek]

Hi!

The following testcase FAILs because a dependent (late) attribute is never
tsubsted.  While the testcase is OpenMP, I think it is a generic C++ FE problem
that could affect any other dependent attribute.

apply_late_template_attributes documents that it relies on
  /* save_template_attributes puts the dependent attributes at the beginning of
     the list; find the non-dependent ones.  */
The "operator binding" attributes that are sometimes added are added to the
head of DECL_ATTRIBUTES list though and because it doesn't have
ATTR_IS_DEPENDENT set it violates this requirement.

The following patch fixes it by adding that attribute after all
ATTR_IS_DEPENDENT attributes.  I'm not 100% sure if DECL_ATTRIBUTES can't be
shared by multiple functions (e.g. the cdtor clones), but the code uses
later remove_attribute which could break that too.  In any case it passed
bootstrap/regtest on x86_64-linux and i686-linux.

Other option would be to copy_list the ATTR_IS_DEPENDENT portion of the
DECL_ATTRIBUTES list if we need to do this, that would be the same as this
patch but replace that *ap = op_attr; at the end with
      *ap = NULL_TREE;
      DECL_ATTRIBUTES (cfn) = chainon (copy_list (DECL_ATTRIBUTES (cfn)),
				       op_attr);
Or perhaps set ATTR_IS_DEPENDENT on the "operator bindings" attribute,
though it would need to be studied what would it try to do with the
attribute during tsubst.

2021-06-03  Jakub Jelinek  <jakub@redhat.com>

	PR c++/100872
	* name-lookup.c (maybe_save_operator_binding): Add op_attr after all
	ATTR_IS_DEPENDENT attributes in the DECL_ATTRIBUTES list rather than
	to the start.

	* g++.dg/gomp/declare-simd-8.C: New test.

--- gcc/cp/name-lookup.c.jj	2021-05-11 09:06:24.281997782 +0200
+++ gcc/cp/name-lookup.c	2021-06-02 15:50:52.042521824 +0200
@@ -9136,9 +9136,12 @@ maybe_save_operator_binding (tree e)
   tree op_attr = lookup_attribute (op_bind_attrname, attributes);
   if (!op_attr)
     {
+      tree *ap = &DECL_ATTRIBUTES (cfn);
+      while (*ap && ATTR_IS_DEPENDENT (*ap))
+	ap = &TREE_CHAIN (*ap);
       op_attr = tree_cons (get_identifier (op_bind_attrname),
-			   NULL_TREE, attributes);
-      DECL_ATTRIBUTES (cfn) = op_attr;
+			   NULL_TREE, *ap);
+      *ap = op_attr;
     }
 
   tree op_bind = purpose_member (fnname, TREE_VALUE (op_attr));
--- gcc/testsuite/g++.dg/gomp/declare-simd-8.C.jj	2021-06-02 16:02:32.792681922 +0200
+++ gcc/testsuite/g++.dg/gomp/declare-simd-8.C	2021-06-02 16:02:09.849004442 +0200
@@ -0,0 +1,15 @@
+// PR c++/100872
+
+template <int N, typename T>
+struct S {
+  #pragma omp declare simd aligned(a : N * 2) aligned(b) linear(ref(b): N)
+  float foo (float *a, T *&b) { return *a + *b; }
+};
+
+S<16, float> s;
+
+float
+bar (float *a, float *p)
+{
+  return s.foo (a, p);
+}

	Jakub

Re: [PATCH] c++: Fix up attribute handling in methods in templates [PR100872]

[Jason Merrill]

On 6/3/21 5:00 AM, Jakub Jelinek wrote:
> Hi!
> 
> The following testcase FAILs because a dependent (late) attribute is never
> tsubsted.  While the testcase is OpenMP, I think it is a generic C++ FE problem
> that could affect any other dependent attribute.
> 
> apply_late_template_attributes documents that it relies on
>    /* save_template_attributes puts the dependent attributes at the beginning of
>       the list; find the non-dependent ones.  */
> The "operator binding" attributes that are sometimes added are added to the
> head of DECL_ATTRIBUTES list though and because it doesn't have
> ATTR_IS_DEPENDENT set it violates this requirement.
> 
> The following patch fixes it by adding that attribute after all
> ATTR_IS_DEPENDENT attributes.  I'm not 100% sure if DECL_ATTRIBUTES can't be
> shared by multiple functions (e.g. the cdtor clones), but the code uses
> later remove_attribute which could break that too.  In any case it passed
> bootstrap/regtest on x86_64-linux and i686-linux.

OK.

> Other option would be to copy_list the ATTR_IS_DEPENDENT portion of the
> DECL_ATTRIBUTES list if we need to do this, that would be the same as this
> patch but replace that *ap = op_attr; at the end with
>        *ap = NULL_TREE;
>        DECL_ATTRIBUTES (cfn) = chainon (copy_list (DECL_ATTRIBUTES (cfn)),
> 				       op_attr);
> Or perhaps set ATTR_IS_DEPENDENT on the "operator bindings" attribute,
> though it would need to be studied what would it try to do with the
> attribute during tsubst.
> 2021-06-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/100872
> 	* name-lookup.c (maybe_save_operator_binding): Add op_attr after all
> 	ATTR_IS_DEPENDENT attributes in the DECL_ATTRIBUTES list rather than
> 	to the start.
> 
> 	* g++.dg/gomp/declare-simd-8.C: New test.
> 
> --- gcc/cp/name-lookup.c.jj	2021-05-11 09:06:24.281997782 +0200
> +++ gcc/cp/name-lookup.c	2021-06-02 15:50:52.042521824 +0200
> @@ -9136,9 +9136,12 @@ maybe_save_operator_binding (tree e)
>     tree op_attr = lookup_attribute (op_bind_attrname, attributes);
>     if (!op_attr)
>       {
> +      tree *ap = &DECL_ATTRIBUTES (cfn);
> +      while (*ap && ATTR_IS_DEPENDENT (*ap))
> +	ap = &TREE_CHAIN (*ap);
>         op_attr = tree_cons (get_identifier (op_bind_attrname),
> -			   NULL_TREE, attributes);
> -      DECL_ATTRIBUTES (cfn) = op_attr;
> +			   NULL_TREE, *ap);
> +      *ap = op_attr;
>       }
>   
>     tree op_bind = purpose_member (fnname, TREE_VALUE (op_attr));
> --- gcc/testsuite/g++.dg/gomp/declare-simd-8.C.jj	2021-06-02 16:02:32.792681922 +0200
> +++ gcc/testsuite/g++.dg/gomp/declare-simd-8.C	2021-06-02 16:02:09.849004442 +0200
> @@ -0,0 +1,15 @@
> +// PR c++/100872
> +
> +template <int N, typename T>
> +struct S {
> +  #pragma omp declare simd aligned(a : N * 2) aligned(b) linear(ref(b): N)
> +  float foo (float *a, T *&b) { return *a + *b; }
> +};
> +
> +S<16, float> s;
> +
> +float
> +bar (float *a, float *p)
> +{
> +  return s.foo (a, p);
> +}
> 
> 	Jakub
>