diagnose attribute conflicts on conversion operators (PR 83394)

In bug 83394 - always_inline vs. noinline no longer diagnosed,
Jakub provided a test case where the recent enhancement to detect
nonsensical attribute combinations fails to detect a pair of
mutually exclusive attributes on separate declarations of
a conversion member operator (see bug 83322 for the origin of
the test case).  This case was previously diagnosed so this is
a regression introduced by the enhancement.

The attached patch restores this diagnostic.  I have very little
experience with lookup and scoping in the C++ front end so if
this isn't the right approach I'd be grateful for suggestions
for what API to use.

In a private conversation Jason mentioned there may be cases
involving templates where the current approach won't have access
to the "last declaration" and so won't be able to detect a mismatch.
I am yet to come up with an example where this happens.  If/when
I do I'll look into enhancing or modifying the current solution
to detect those as well.  But until then I'd like to submit this
as an incremental step in that direction.

The attached patch passes regression testing on x86_64-linux.

Thanks
Martin

The attached update also fixes both instances of the ICE
reported in bug 83322 and supersedes Jakub's patch for that
bug (https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00765.html).
This passes bootstrap on x86_64 with no new regressions (there
are an increasing number of failures on trunk at the moment
but, AFAICS, none caused by this patch).

Jason, I'm still trying to come up with a test case for templates
that would illustrate the issue you're concerned about.  If you
have one that would be great (preferably one showing a regression).

Martin

On 12/12/2017 06:25 PM, Martin Sebor wrote:
> In bug 83394 - always_inline vs. noinline no longer diagnosed,
> Jakub provided a test case where the recent enhancement to detect
> nonsensical attribute combinations fails to detect a pair of
> mutually exclusive attributes on separate declarations of
> a conversion member operator (see bug 83322 for the origin of
> the test case).  This case was previously diagnosed so this is
> a regression introduced by the enhancement.
>
> The attached patch restores this diagnostic.  I have very little
> experience with lookup and scoping in the C++ front end so if
> this isn't the right approach I'd be grateful for suggestions
> for what API to use.
>
> In a private conversation Jason mentioned there may be cases
> involving templates where the current approach won't have access
> to the "last declaration" and so won't be able to detect a mismatch.
> I am yet to come up with an example where this happens.  If/when
> I do I'll look into enhancing or modifying the current solution
> to detect those as well.  But until then I'd like to submit this
> as an incremental step in that direction.
>
> The attached patch passes regression testing on x86_64-linux.
>
> Thanks
> Martin
PR c++/83394 - [8 Regression] always_inline vs. noinline no longer diagnosed
PR c++/83322 - ICE: tree check: expected class ‘type’, have ‘exceptional’

gcc/cp/ChangeLog:

	PR c++/83394
	PR c++/83322
	* decl2.c (cplus_decl_attributes): Look up member functions
	in the scope of their class.

gcc/testsuite/ChangeLog:

	PR c++/83394
	* g++.dg/Wattributes-3.C: New test.
	* g++.dg/Wattributes-4.C: New test.

diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
index 5d30369..ae5dbab 100644
--- a/gcc/cp/decl2.c
+++ b/gcc/cp/decl2.c
@@ -1432,6 +1432,71 @@ cp_omp_mappable_type (tree type)
   return true;
 }
 
+/* Return the last pushed declaration for the symbol DECL or NULL
+   when no such declaration exists.  */
+
+static tree
+find_last_decl (tree decl)
+{
+  tree last_decl = NULL_TREE;
+
+  if (tree name = DECL_P (decl) ? DECL_NAME (decl) : NULL_TREE)
+    {
+      /* Look up the declaration in its scope.  */
+      tree pushed_scope = NULL_TREE;
+      if (tree ctype = DECL_CONTEXT (decl))
+	pushed_scope = push_scope (ctype);
+
+      last_decl = lookup_name (name);
+
+      if (pushed_scope)
+	pop_scope (pushed_scope);
+
+      /* The declaration may be a member conversion operator
+	 or a bunch of overfloads (handle the latter below).  */
+      if (last_decl && BASELINK_P (last_decl))
+	last_decl = BASELINK_FUNCTIONS (last_decl);
+    }
+
+  if (!last_decl)
+    return NULL_TREE;
+
+  if (DECL_P (last_decl))
+    return last_decl;
+
+  if (TREE_CODE (last_decl) == OVERLOAD)
+    {
+      /* A set of overloads of the same function.  */
+      for (ovl_iterator iter (last_decl, true); iter; ++iter)
+	{
+	  if (TREE_CODE (*iter) == OVERLOAD)
+	    continue;
+
+	  if (decls_match (decl, *iter, /*record_decls=*/false))
+	    return *iter;
+	}
+
+      return NULL_TREE;
+    }
+
+  if (TREE_CODE (last_decl) == TREE_LIST)
+    {
+      /* The list contains a mix of symbols with the same name
+	 (e.g., functions and data members defined in different
+	 base classes).  */
+      do
+	{
+	  if (decls_match (decl, TREE_VALUE (last_decl)))
+	    return TREE_VALUE (last_decl);
+
+	  last_decl = TREE_CHAIN (last_decl);
+	}
+      while (last_decl);
+    }
+
+  return NULL_TREE;
+}
+
 /* Like decl_attributes, but handle C++ complexity.  */
 
 void
@@ -1483,28 +1548,7 @@ cplus_decl_attributes (tree *decl, tree attributes, int flags)
     }
   else
     {
-      tree last_decl = (DECL_P (*decl) && DECL_NAME (*decl)
-			? lookup_name (DECL_NAME (*decl)) : NULL_TREE);
-
-      if (last_decl && TREE_CODE (last_decl) == OVERLOAD)
-	for (ovl_iterator iter (last_decl, true); ; ++iter)
-	  {
-	    if (!iter)
-	      {
-		last_decl = NULL_TREE;
-		break;
-	      }
-
-	    if (TREE_CODE (*iter) == OVERLOAD)
-	      continue;
-
-	    if (decls_match (*decl, *iter, /*record_decls=*/false))
-	      {
-		last_decl = *iter;
-		break;
-	      }
-	  }
-
+      tree last_decl = find_last_decl (*decl);
       decl_attributes (decl, attributes, flags, last_decl);
     }
 
diff --git a/gcc/testsuite/g++.dg/Wattributes-3.C b/gcc/testsuite/g++.dg/Wattributes-3.C
new file mode 100644
index 0000000..2479998
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wattributes-3.C
@@ -0,0 +1,89 @@
+// PR c++/83394 - always_inline vs. noinline no longer diagnosed
+// { dg-do compile }
+// { dg-options "-Wattributes" }
+
+#define ATTR(list) __attribute__ (list)
+
+struct A
+{
+  ATTR ((__noinline__)) operator int ();
+};
+
+ATTR ((__always_inline__))
+A::operator int ()            // { dg-warning "ignoring attribute .always_inline. because it conflicts with attribute .noinline." }
+{
+  return 0;
+}
+
+
+struct B
+{
+  operator char () const;
+  ATTR ((__always_inline__)) operator int () const;
+};
+
+B::operator char () const { return 0; }
+
+ATTR ((__noinline__))
+B::operator int () const      // { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." }
+{
+  return 0;
+}
+
+
+struct C
+{
+  operator char ();
+  ATTR ((__always_inline__)) operator short ();
+  operator int ();
+  ATTR ((__noinline__)) operator long ();
+};
+
+C::operator char () { return 0; }
+
+ATTR ((__noinline__))
+C::operator short ()           // { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." }
+{ return 0; }
+
+inline ATTR ((__noinline__))
+C::operator int () { return 0; }
+
+
+ATTR ((__always_inline__))
+C::operator long ()           // { dg-warning "ignoring attribute .always_inline. because it conflicts with attribute .noinline." }
+{ return 0; }
+
+
+struct D
+{
+  int foo ();
+  int foo (int);
+  int ATTR ((const)) foo (int, int);
+  int ATTR ((pure)) foo (int, int, int);
+
+  int ATTR ((const)) foo (int, int, int, int);
+
+  int foo (int, int, int, int, int);
+};
+
+int ATTR ((const))
+D::foo ()
+{ return 0; }
+
+int ATTR ((pure))
+D::foo (int)
+{ return 0; }
+
+int ATTR ((pure))
+D::foo (int, int)             // { dg-warning "ignoring attribute .pure. because it conflicts with attribute .const." }
+{ return 0; }
+
+int ATTR ((const))
+D::foo (int, int, int)        // { dg-warning "ignoring attribute .const. because it conflicts with attribute .pure." }
+{ return 0; }
+
+int
+D::foo (int, int, int, int) { return 0; }
+
+int ATTR ((const))
+D::foo (int, int, int, int, int) { return 0; }
diff --git a/gcc/testsuite/g++.dg/Wattributes-4.C b/gcc/testsuite/g++.dg/Wattributes-4.C
new file mode 100644
index 0000000..c925225
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wattributes-4.C
@@ -0,0 +1,29 @@
+// PR c++/83322 - ICE: tree check: expected class ‘type’, have ‘exceptional’
+// (baselink) in diag_attr_exclusions, at attribs.c:393
+// { dg-do compile }
+// { dg-options "-Wattributes" }
+
+#define ATTR(list) __attribute__ (list)
+
+// Test case from comment #0.
+struct A0
+{
+  template <class T> operator T();
+  ATTR ((always_inline)) operator int();
+};
+
+// Test case from comment #4.
+struct A1
+{
+  void foo();
+};
+
+struct B
+{
+  bool foo;
+};
+
+struct C: A1, B
+{
+  ATTR ((warn_unused_result)) int foo ();
+};

On Wed, Dec 13, 2017 at 12:54 PM, Martin Sebor <msebor@gmail.com> wrote:
> The attached update also fixes both instances of the ICE
> reported in bug 83322 and supersedes Jakub's patch for that
> bug (https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00765.html).
> This passes bootstrap on x86_64 with no new regressions (there
> are an increasing number of failures on trunk at the moment
> but, AFAICS, none caused by this patch).
>
> Jason, I'm still trying to come up with a test case for templates
> that would illustrate the issue you're concerned about.  If you
> have one that would be great (preferably one showing a regression).

I looked at the case I was concerned about, and found that it isn't an
issue because in that case we call duplicate_decls before applying
attributes.

But it looks like we'll still get this testcase wrong, because the
code assumes that if the old decl is a single _DECL, it must match.

[[gnu::noinline]] void f() { }
[[gnu::always_inline]] void f(int) { }  // OK, not the same function

I think the answer is to use Nathan's new iterators unconditionally,
probably lkp_iterator.

Jason

On 12/13/2017 02:38 PM, Jason Merrill wrote:
> On Wed, Dec 13, 2017 at 12:54 PM, Martin Sebor <msebor@gmail.com> wrote:
>> The attached update also fixes both instances of the ICE
>> reported in bug 83322 and supersedes Jakub's patch for that
>> bug (https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00765.html).
>> This passes bootstrap on x86_64 with no new regressions (there
>> are an increasing number of failures on trunk at the moment
>> but, AFAICS, none caused by this patch).
>>
>> Jason, I'm still trying to come up with a test case for templates
>> that would illustrate the issue you're concerned about.  If you
>> have one that would be great (preferably one showing a regression).
>
> I looked at the case I was concerned about, and found that it isn't an
> issue because in that case we call duplicate_decls before applying
> attributes.
>
> But it looks like we'll still get this testcase wrong, because the
> code assumes that if the old decl is a single _DECL, it must match.
>
> [[gnu::noinline]] void f() { }
> [[gnu::always_inline]] void f(int) { }  // OK, not the same function
>
> I think the answer is to use Nathan's new iterators unconditionally,
> probably lkp_iterator.

Thanks for the test case.  You're right that this problem still
exists.  I thought a complete fix for it would be simple enough
to include in this patch but after running into issues with
assumptions about how inline/noinline conflicts are resolved
I think it's best to fix the ICE alone in this patch and deal
with the pre-existing bug above in a follow up.  Apparently
(according to comment #6 on pr83322) the ICE is causing some
anxiety about the timely availability of a fix, so I'd like
to avoid spending more time on it than is necessary.

Attached is an updated patch.  It handles the overloads above
correctly but it doesn't fix the latent problem and so they
are still diagnosed, same as in GCC 7.

Martin
PR c++/83394 - always_inline vs. noinline no longer diagnosed
PR c++/83322 - ICE: tree check: expected class ‘type’, have ‘exceptional’

gcc/cp/ChangeLog:

	PR c++/83394
	PR c++/83322
	* decl2.c (cplus_decl_attributes): Look up member functions
	in the scope of their class.

gcc/testsuite/ChangeLog:

	PR c++/83394
	* g++.dg/Wattributes-3.C: New test.
	* g++.dg/Wattributes-4.C: New test.
	* g++.dg/Wattributes-5.C: New test.

Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 255779)
+++ gcc/cp/decl2.c	(working copy)
@@ -1432,6 +1432,67 @@ cp_omp_mappable_type (tree type)
   return true;
 }
 
+/* Return the last pushed declaration for the symbol DECL or NULL
+   when no such declaration exists.  */
+
+static tree
+find_last_decl (tree decl)
+{
+  tree last_decl = NULL_TREE;
+
+  if (tree name = DECL_P (decl) ? DECL_NAME (decl) : NULL_TREE)
+    {
+      /* Look up the declaration in its scope.  */
+      tree pushed_scope = NULL_TREE;
+      if (tree ctype = DECL_CONTEXT (decl))
+	pushed_scope = push_scope (ctype);
+
+      last_decl = lookup_name (name);
+
+      if (pushed_scope)
+	pop_scope (pushed_scope);
+
+      /* The declaration may be a member conversion operator
+	 or a bunch of overfloads (handle the latter below).  */
+      if (last_decl && BASELINK_P (last_decl))
+	last_decl = BASELINK_FUNCTIONS (last_decl);
+    }
+
+  if (!last_decl)
+    return NULL_TREE;
+
+  if (DECL_P (last_decl) || TREE_CODE (last_decl) == OVERLOAD)
+    {
+      /* A set of overloads of the same function.  */
+      for (lkp_iterator iter (last_decl); iter; ++iter)
+	{
+	  if (TREE_CODE (*iter) == OVERLOAD)
+	    continue;
+
+	  if (decls_match (decl, *iter, /*record_decls=*/false))
+	    return *iter;
+	}
+      return NULL_TREE;
+    }
+
+  if (TREE_CODE (last_decl) == TREE_LIST)
+    {
+      /* The list contains a mix of symbols with the same name
+	 (e.g., functions and data members defined in different
+	 base classes).  */
+      do
+	{
+	  if (decls_match (decl, TREE_VALUE (last_decl)))
+	    return TREE_VALUE (last_decl);
+
+	  last_decl = TREE_CHAIN (last_decl);
+	}
+      while (last_decl);
+    }
+
+  return NULL_TREE;
+}
+
 /* Like decl_attributes, but handle C++ complexity.  */
 
 void
@@ -1483,28 +1544,7 @@ cplus_decl_attributes (tree *decl, tree attributes
     }
   else
     {
-      tree last_decl = (DECL_P (*decl) && DECL_NAME (*decl)
-			? lookup_name (DECL_NAME (*decl)) : NULL_TREE);
-
-      if (last_decl && TREE_CODE (last_decl) == OVERLOAD)
-	for (ovl_iterator iter (last_decl, true); ; ++iter)
-	  {
-	    if (!iter)
-	      {
-		last_decl = NULL_TREE;
-		break;
-	      }
-
-	    if (TREE_CODE (*iter) == OVERLOAD)
-	      continue;
-
-	    if (decls_match (*decl, *iter, /*record_decls=*/false))
-	      {
-		last_decl = *iter;
-		break;
-	      }
-	  }
-
+      tree last_decl = find_last_decl (*decl);
       decl_attributes (decl, attributes, flags, last_decl);
     }
 
Index: gcc/testsuite/g++.dg/Wattributes-3.C
===================================================================
--- gcc/testsuite/g++.dg/Wattributes-3.C	(nonexistent)
+++ gcc/testsuite/g++.dg/Wattributes-3.C	(working copy)
@@ -0,0 +1,90 @@
+// PR c++/83394 - always_inline vs. noinline no longer diagnosed
+// { dg-do compile }
+// { dg-options "-Wattributes" }
+
+#define ATTR(list) __attribute__ (list)
+
+struct A
+{
+  ATTR ((__noinline__)) operator int ();
+};
+
+ATTR ((__always_inline__))
+A::operator int ()            // { dg-warning "ignoring attribute .always_inline. because it conflicts with attribute .noinline." }
+{
+  return 0;
+}
+
+
+struct B
+{
+  operator char () const;
+  ATTR ((__always_inline__)) operator int () const;
+};
+
+B::operator char () const { return 0; }
+
+ATTR ((__noinline__))
+B::operator int () const      // { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." }
+{
+  return 0;
+}
+
+
+struct C
+{
+  operator char ();
+  ATTR ((__always_inline__)) operator short ();
+  operator int ();
+  ATTR ((__noinline__)) operator long ();
+};
+
+C::operator char () { return 0; }
+
+ATTR ((__noinline__))
+C::operator short ()           // { dg-warning "ignoring attribute .noinline. because it conflicts with attribute .always_inline." }
+{ return 0; }
+
+inline ATTR ((__noinline__))
+C::operator int ()
+{ return 0; }
+
+
+ATTR ((__always_inline__))
+C::operator long ()           // { dg-warning "ignoring attribute .always_inline. because it conflicts with attribute .noinline." }
+{ return 0; }
+
+
+struct D
+{
+  int foo ();
+  int foo (int);
+  int ATTR ((const)) foo (int, int);
+  int ATTR ((pure)) foo (int, int, int);
+
+  int ATTR ((const)) foo (int, int, int, int);
+
+  int foo (int, int, int, int, int);
+};
+
+int ATTR ((const))
+D::foo ()
+{ return 0; }
+
+int ATTR ((pure))
+D::foo (int)
+{ return 0; }
+
+int ATTR ((pure))
+D::foo (int, int)             // { dg-warning "ignoring attribute .pure. because it conflicts with attribute .const." }
+{ return 0; }
+
+int ATTR ((const))
+D::foo (int, int, int)        // { dg-warning "ignoring attribute .const. because it conflicts with attribute .pure." }
+{ return 0; }
+
+int
+D::foo (int, int, int, int) { return 0; }
+
+int ATTR ((const))
+D::foo (int, int, int, int, int) { return 0; }
Index: gcc/testsuite/g++.dg/Wattributes-4.C
===================================================================
--- gcc/testsuite/g++.dg/Wattributes-4.C	(nonexistent)
+++ gcc/testsuite/g++.dg/Wattributes-4.C	(working copy)
@@ -0,0 +1,29 @@
+// PR c++/83322 - ICE: tree check: expected class ‘type’, have ‘exceptional’
+// (baselink) in diag_attr_exclusions, at attribs.c:393
+// { dg-do compile }
+// { dg-options "-Wattributes" }
+
+#define ATTR(list) __attribute__ (list)
+
+// Test case from comment #0.
+struct A0
+{
+  template <class T> operator T();
+  ATTR ((always_inline)) operator int();
+};
+
+// Test case from comment #4.
+struct A1
+{
+  void foo();
+};
+
+struct B
+{
+  bool foo;
+};
+
+struct C: A1, B
+{
+  ATTR ((warn_unused_result)) int foo ();
+};
Index: gcc/testsuite/g++.dg/Wattributes-5.C
===================================================================
--- gcc/testsuite/g++.dg/Wattributes-5.C	(nonexistent)
+++ gcc/testsuite/g++.dg/Wattributes-5.C	(working copy)
@@ -0,0 +1,34 @@
+// { dg-do compile }
+// { dg-options "-Wattributes" }
+
+#define ATTR(list) __attribute__ (list)
+
+template <int>
+struct A
+{
+  int __attribute__ ((noinline))
+  f ();                       // { dg-message "previous declaration here" }
+};
+
+template <int N>
+int __attribute__ ((always_inline))
+A<N>::f ()                    // { dg-warning "ignoring attribute .always_inline. because it conflicts with attribute .noinline." } */
+{ return 0; }
+
+
+template <int>
+struct B
+{
+  int __attribute__ ((always_inline))
+  f ();
+};
+
+template <>
+inline int __attribute__ ((always_inline))
+B<0>::f ()
+{ return 0; }
+
+template <>
+int __attribute__ ((noinline))
+B<1>::f ()
+{ return 1; }

On 12/18/2017 05:25 PM, Martin Sebor wrote:
> On 12/13/2017 02:38 PM, Jason Merrill wrote:
>> On Wed, Dec 13, 2017 at 12:54 PM, Martin Sebor <msebor@gmail.com> wrote:
>>> The attached update also fixes both instances of the ICE
>>> reported in bug 83322 and supersedes Jakub's patch for that
>>> bug (https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00765.html).
>>> This passes bootstrap on x86_64 with no new regressions (there
>>> are an increasing number of failures on trunk at the moment
>>> but, AFAICS, none caused by this patch).
>>>
>>> Jason, I'm still trying to come up with a test case for templates
>>> that would illustrate the issue you're concerned about.  If you
>>> have one that would be great (preferably one showing a regression).
>>
>> I looked at the case I was concerned about, and found that it isn't an
>> issue because in that case we call duplicate_decls before applying
>> attributes.
>>
>> But it looks like we'll still get this testcase wrong, because the
>> code assumes that if the old decl is a single _DECL, it must match.
>>
>> [[gnu::noinline]] void f() { }
>> [[gnu::always_inline]] void f(int) { }  // OK, not the same function
>>
>> I think the answer is to use Nathan's new iterators unconditionally,
>> probably lkp_iterator.
> 
> Thanks for the test case.  You're right that this problem still
> exists.  I thought a complete fix for it would be simple enough
> to include in this patch but after running into issues with
> assumptions about how inline/noinline conflicts are resolved
> I think it's best to fix the ICE alone in this patch and deal
> with the pre-existing bug above in a follow up.  Apparently
> (according to comment #6 on pr83322) the ICE is causing some
> anxiety about the timely availability of a fix, so I'd like
> to avoid spending more time on it than is necessary.
> 
> Attached is an updated patch.  It handles the overloads above
> correctly but it doesn't fix the latent problem and so they
> are still diagnosed, same as in GCC 7.

I still share Jakub's uneasiness with this approach; looking up a 
redeclaration works rather differently from a normal lookup, and we 
already have code for handling that.  I still think that handling this 
stuff by extending diagnose_mismatched_attributes is a better way to go.

That said, it's good to fix the ICE as a stopgap.

> +  if (TREE_CODE (last_decl) == TREE_LIST)
> +    {
> +      /* The list contains a mix of symbols with the same name
> +	 (e.g., functions and data members defined in different
> +	 base classes).  */
> +      do
> +	{
> +	  if (decls_match (decl, TREE_VALUE (last_decl)))
> +	    return TREE_VALUE (last_decl);
> +
> +	  last_decl = TREE_CHAIN (last_decl);
> +	}
> +      while (last_decl);
> +    }

We shouldn't need to handle TREE_LIST at all, as getting that result 
should indicate that there isn't any declaration in the scope we care 
about; decls from base classes will never match.

OK with this block removed.

Jason

On 12/19/2017 10:25 AM, Jason Merrill wrote:
> On 12/18/2017 05:25 PM, Martin Sebor wrote:
>> On 12/13/2017 02:38 PM, Jason Merrill wrote:
>>> On Wed, Dec 13, 2017 at 12:54 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>> The attached update also fixes both instances of the ICE
>>>> reported in bug 83322 and supersedes Jakub's patch for that
>>>> bug (https://gcc.gnu.org/ml/gcc-patches/2017-12/msg00765.html).
>>>> This passes bootstrap on x86_64 with no new regressions (there
>>>> are an increasing number of failures on trunk at the moment
>>>> but, AFAICS, none caused by this patch).
>>>>
>>>> Jason, I'm still trying to come up with a test case for templates
>>>> that would illustrate the issue you're concerned about.  If you
>>>> have one that would be great (preferably one showing a regression).
>>>
>>> I looked at the case I was concerned about, and found that it isn't an
>>> issue because in that case we call duplicate_decls before applying
>>> attributes.
>>>
>>> But it looks like we'll still get this testcase wrong, because the
>>> code assumes that if the old decl is a single _DECL, it must match.
>>>
>>> [[gnu::noinline]] void f() { }
>>> [[gnu::always_inline]] void f(int) { }  // OK, not the same function
>>>
>>> I think the answer is to use Nathan's new iterators unconditionally,
>>> probably lkp_iterator.
>>
>> Thanks for the test case.  You're right that this problem still
>> exists.  I thought a complete fix for it would be simple enough
>> to include in this patch but after running into issues with
>> assumptions about how inline/noinline conflicts are resolved
>> I think it's best to fix the ICE alone in this patch and deal
>> with the pre-existing bug above in a follow up.  Apparently
>> (according to comment #6 on pr83322) the ICE is causing some
>> anxiety about the timely availability of a fix, so I'd like
>> to avoid spending more time on it than is necessary.
>>
>> Attached is an updated patch.  It handles the overloads above
>> correctly but it doesn't fix the latent problem and so they
>> are still diagnosed, same as in GCC 7.
>
> I still share Jakub's uneasiness with this approach; looking up a
> redeclaration works rather differently from a normal lookup, and we
> already have code for handling that.  I still think that handling this
> stuff by extending diagnose_mismatched_attributes is a better way to go.
>
> That said, it's good to fix the ICE as a stopgap.

I committed the patch without the block below in r255844.

I've been continuing to try to find a test case that shows
the problem you and Jakub are concerned about.  I've created
a bunch of passing test cases that I think would be useful to
add to the test suite.  I've also found a few pre-existing bugs
but (AFAICT) nothing that conclusively implicates the new code.

I've opened the following for these problems.  The first one
is the one you pointed out.  The GCC 8 regression (pr83394)
is triggered by my patch.  I haven't debugged it yet but I
wonder if it's due to the same root cause as 83502).

83498 - bogus -Wattributes for always_inline and noinline on
         distinct overloads
83504 - incorrect attribute const interpretation on function
         overloads
83502 - [6/7/8 Regression] bogus -Wattributes for always_inline
         and noinline on function template specialization
83394 - [8 Regression] always_inline vs. noinline no longer
         diagnosed​

Martin

>
>> +  if (TREE_CODE (last_decl) == TREE_LIST)
>> +    {
>> +      /* The list contains a mix of symbols with the same name
>> +     (e.g., functions and data members defined in different
>> +     base classes).  */
>> +      do
>> +    {
>> +      if (decls_match (decl, TREE_VALUE (last_decl)))
>> +        return TREE_VALUE (last_decl);
>> +
>> +      last_decl = TREE_CHAIN (last_decl);
>> +    }
>> +      while (last_decl);
>> +    }
>
> We shouldn't need to handle TREE_LIST at all, as getting that result
> should indicate that there isn't any declaration in the scope we care
> about; decls from base classes will never match.
>
> OK with this block removed.
>
> Jason