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

[Martin Sebor <msebor at gmail dot com>]

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