[PATCH] diagnose attribute conflicts on conversion operators (PR 83394)

[Jason Merrill]

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