On Fri, Jan 29, 2021 at 10:31:15PM -0500, Jason Merrill via Gcc-patches wrote:
> On 1/29/21 6:28 PM, Marek Polacek wrote:
> > On Fri, Jan 29, 2021 at 06:18:35PM -0500, Jason Merrill via Gcc-patches
> > wrote:
> > > On 1/29/21 5:52 PM, Marek Polacek wrote:
> > > > On Fri, Jan 29, 2021 at 04:23:14PM -0500, Marek Polacek via Gcc-patches
> > > > wrote:
> > > > > On Fri, Jan 29, 2021 at 04:02:51PM -0500, Marek Polacek via
> > > > > Gcc-patches wrote:
> > > > > > __builtin_has_attribute doesn't work in templates yet (bug 92104),
> > > > > > so
> > > > > > in r11-471 I added a sorry. But that only caught type-dependent
> > > > > > expressions and we also want to sorry on value-dependent
> > > > > > expressions.
> > > > > > This patch uses v_d_e_p rather than uses_template_parms because
> > > > > > u_t_p
> > > > > > sets p_t_d and then v_d_e_p considers variables with reference types
> > > > > > value-dependent, which breaks builtin-has-attribute-6.c.
> > > > > >
> > > > > > This is a regression and I also plan to apply this to gcc-10.
> > > > > >
> > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
> > > > > >
> > > > > > gcc/cp/ChangeLog:
> > > > > >
> > > > > > PR c++/98355
> > > > > > * parser.c (cp_parser_has_attribute_expression): Use
> > > > > > value_dependent_expression_p instead of
> > > > > > type_dependent_expression_p.
> > > > > >
> > > > > > gcc/testsuite/ChangeLog:
> > > > > >
> > > > > > PR c++/98355
> > > > > > * g++.dg/ext/builtin-has-attribute2.C: New test.
> > > > > > ---
> > > > > > gcc/cp/parser.c | 2 +-
> > > > > > gcc/testsuite/g++.dg/ext/builtin-has-attribute2.C | 8 ++++++++
> > > > > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > > > > > create mode 100644
> > > > > > gcc/testsuite/g++.dg/ext/builtin-has-attribute2.C
> > > > > >
> > > > > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> > > > > > index 5c1d880c9fc..7b1dc0dc93f 100644
> > > > > > --- a/gcc/cp/parser.c
> > > > > > +++ b/gcc/cp/parser.c
> > > > > > @@ -8934,7 +8934,7 @@ cp_parser_has_attribute_expression (cp_parser
> > > > > > *parser)
> > > > > > {
> > > > > > if (oper == error_mark_node)
> > > > > > /* Nothing. */;
> > > > > > - else if (type_dependent_expression_p (oper))
> > > > > > + else if (value_dependent_expression_p (oper))
> > > > > > sorry_at (atloc, "%<__builtin_has_attribute%> with dependent
> > > > > > argument "
> > > > > > "not supported yet");
> > > > >
> > > > > Actually I don't like this. I think we want
> > > > > processing_template_decl && uses_template_parms ()
> > > > >
> > > > > here.
> > > >
> > > > So here's v2. Sorry for the self-review.
> > > >
> > > > -- >8 --
> > > > __builtin_has_attribute doesn't work in templates yet (bug 92104), so
> > > > in r11-471 I added a sorry. But that only caught type-dependent
> > > > expressions and we also want to sorry on value-dependent expressions.
> > > > This patch uses uses_template_parms, but guarded with p_t_d, because
> > > > u_t_p sets p_t_d and then v_d_e_p considers variables with reference
> > > > types value-dependent, which breaks builtin-has-attribute-6.c.
> > >
> > > Maybe instantiation_dependent_expression_p?
> >
> > That didn't work here because "oper" can be a type, e.g. integer_type,
> > and then potential_constant_expression_1 called from i_d_e_p crashes
> > because it doesn't expect to see a TYPE_P.
>
> Hmm, if we were calling type_dependent_expression_p with a type argument,
> that was a bug, and we probably should have aborted; please add such an
> assert. But then your patch fixes the bug here, so the patch is OK with the
> assert added.
Thanks. Unfortunately adding
gcc_assert (!TYPE_P (expression));
to type_dependent_expression_p causes some amount of ICEs in the testsuite.
Can I go ahead with my patch without the assert and deal with the assert
later?
Marek
