[v2] c++: fn redecl in fn scope wrongly accepted [PR116239]

On Wed, Sep 04, 2024 at 12:28:49PM -0400, Jason Merrill wrote:
> On 8/30/24 3:40 PM, Marek Polacek wrote:
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > 
> > -- >8 --
> > Redeclaration such as
> > 
> >    void f(void);
> >    consteval void f(void);
> > 
> > is invalid.  In a namespace scope, we detect the collision in
> > validate_constexpr_redeclaration, but not when one declaration is
> > at block scope.
> > 
> > When we have
> > 
> >    void f(void);
> >    void g() { consteval void f(void); }
> > 
> > we call pushdecl on the second f and call push_local_extern_decl_alias.
> > It finds the namespace-scope f:
> > 
> >          for (ovl_iterator iter (binding); iter; ++iter)
> >            if (decls_match (decl, *iter, /*record_versions*/false))
> >              {
> >                alias = *iter;
> >                break;
> >              }
> > 
> > but decls_match says they match so we just set DECL_LOCAL_DECL_ALIAS
> > (and do not call another pushdecl leading to duplicate_decls which
> > would detect mismatching return types, for example).  I don't think
> > we want to change decls_match, so a simple fix is to detect the
> > problem in push_local_extern_decl_alias.
> > 
> > 	PR c++/116239
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* cp-tree.h (validate_constexpr_redeclaration): Declare.
> > 	* decl.cc (validate_constexpr_redeclaration): No longer static.
> > 	* name-lookup.cc (push_local_extern_decl_alias): Call
> > 	validate_constexpr_redeclaration.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/diagnostic/redeclaration-6.C: New test.
> > ---
> >   gcc/cp/cp-tree.h                              |  1 +
> >   gcc/cp/decl.cc                                |  2 +-
> >   gcc/cp/name-lookup.cc                         |  3 ++
> >   .../g++.dg/diagnostic/redeclaration-6.C       | 34 +++++++++++++++++++
> >   4 files changed, 39 insertions(+), 1 deletion(-)
> >   create mode 100644 gcc/testsuite/g++.dg/diagnostic/redeclaration-6.C
> > 
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index 2eeb5e3e8b1..1a763b683de 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -6992,6 +6992,7 @@ extern bool member_like_constrained_friend_p	(tree);
> >   extern bool fns_correspond			(tree, tree);
> >   extern int decls_match				(tree, tree, bool = true);
> >   extern bool maybe_version_functions		(tree, tree, bool);
> > +extern bool validate_constexpr_redeclaration	(tree, tree);
> >   extern bool merge_default_template_args		(tree, tree, bool);
> >   extern tree duplicate_decls			(tree, tree,
> >   						 bool hiding = false,
> > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> > index 6458e96bded..4ad68d609d7 100644
> > --- a/gcc/cp/decl.cc
> > +++ b/gcc/cp/decl.cc
> > @@ -1412,7 +1412,7 @@ check_redeclaration_exception_specification (tree new_decl,
> >   /* Return true if OLD_DECL and NEW_DECL agree on constexprness.
> >      Otherwise issue diagnostics.  */
> > -static bool
> > +bool
> >   validate_constexpr_redeclaration (tree old_decl, tree new_decl)
> >   {
> >     old_decl = STRIP_TEMPLATE (old_decl);
> > diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> > index 70ad4cbf3b5..6777d97ac2e 100644
> > --- a/gcc/cp/name-lookup.cc
> > +++ b/gcc/cp/name-lookup.cc
> > @@ -3708,6 +3708,9 @@ push_local_extern_decl_alias (tree decl)
> >   	}
> >       }
> > +  if (!validate_constexpr_redeclaration (alias, decl))
> > +    return;
> > +
> >     retrofit_lang_decl (decl);
> >     DECL_LOCAL_DECL_ALIAS (decl) = alias;
> >   }
> 
> I don't think we need this in the case that we built a new alias and pushed
> it; in that case alias is built from decl, and should certainly match
> already.  So let's put this call before we decide to build a new alias,
> either in the loop or just after it.

That's right.  How about this?

dg.exp passed.

-- >8 --
Redeclaration such as

  void f(void);
  consteval void f(void);

is invalid.  In a namespace scope, we detect the collision in
validate_constexpr_redeclaration, but not when one declaration is
at block scope.

When we have

  void f(void);
  void g() { consteval void f(void); }

we call pushdecl on the second f and call push_local_extern_decl_alias.
It finds the namespace-scope f:

        for (ovl_iterator iter (binding); iter; ++iter)
          if (decls_match (decl, *iter, /*record_versions*/false))
            {
              alias = *iter;
              break;
            }

but decls_match says they match so we just set DECL_LOCAL_DECL_ALIAS
(and do not call another pushdecl leading to duplicate_decls which
would detect mismatching return types, for example).  I don't think
we want to change decls_match, so a simple fix is to detect the
problem in push_local_extern_decl_alias.

	PR c++/116239

gcc/cp/ChangeLog:

	* cp-tree.h (validate_constexpr_redeclaration): Declare.
	* decl.cc (validate_constexpr_redeclaration): No longer static.
	* name-lookup.cc (push_local_extern_decl_alias): Call
	validate_constexpr_redeclaration.

gcc/testsuite/ChangeLog:

	* g++.dg/diagnostic/redeclaration-6.C: New test.
---
 gcc/cp/cp-tree.h                              |  1 +
 gcc/cp/decl.cc                                |  2 +-
 gcc/cp/name-lookup.cc                         |  2 ++
 .../g++.dg/diagnostic/redeclaration-6.C       | 34 +++++++++++++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/diagnostic/redeclaration-6.C


base-commit: c755c7a32590e2eef5a8b062b9756c1513910246

On 9/4/24 1:18 PM, Marek Polacek wrote:
> On Wed, Sep 04, 2024 at 12:28:49PM -0400, Jason Merrill wrote:

>>> +  if (!validate_constexpr_redeclaration (alias, decl))
>>> +    return;
>>> +
>>>      retrofit_lang_decl (decl);
>>>      DECL_LOCAL_DECL_ALIAS (decl) = alias;
>>>    }
>>
>> I don't think we need this in the case that we built a new alias and pushed
>> it; in that case alias is built from decl, and should certainly match
>> already.  So let's put this call before we decide to build a new alias,
>> either in the loop or just after it.
> 
> That's right.  How about this?

Hmm, I'd still prefer to have it *before* the !alias case for locality; 
putting it as an else to a 66-line if seems obscure.

But OK however.

> dg.exp passed.
> 
> -- >8 --
> Redeclaration such as
> 
>    void f(void);
>    consteval void f(void);
> 
> is invalid.  In a namespace scope, we detect the collision in
> validate_constexpr_redeclaration, but not when one declaration is
> at block scope.
> 
> When we have
> 
>    void f(void);
>    void g() { consteval void f(void); }
> 
> we call pushdecl on the second f and call push_local_extern_decl_alias.
> It finds the namespace-scope f:
> 
>          for (ovl_iterator iter (binding); iter; ++iter)
>            if (decls_match (decl, *iter, /*record_versions*/false))
>              {
>                alias = *iter;
>                break;
>              }
> 
> but decls_match says they match so we just set DECL_LOCAL_DECL_ALIAS
> (and do not call another pushdecl leading to duplicate_decls which
> would detect mismatching return types, for example).  I don't think
> we want to change decls_match, so a simple fix is to detect the
> problem in push_local_extern_decl_alias.
> 
> 	PR c++/116239
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (validate_constexpr_redeclaration): Declare.
> 	* decl.cc (validate_constexpr_redeclaration): No longer static.
> 	* name-lookup.cc (push_local_extern_decl_alias): Call
> 	validate_constexpr_redeclaration.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/diagnostic/redeclaration-6.C: New test.
> ---
>   gcc/cp/cp-tree.h                              |  1 +
>   gcc/cp/decl.cc                                |  2 +-
>   gcc/cp/name-lookup.cc                         |  2 ++
>   .../g++.dg/diagnostic/redeclaration-6.C       | 34 +++++++++++++++++++
>   4 files changed, 38 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/redeclaration-6.C
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 2eeb5e3e8b1..1a763b683de 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -6992,6 +6992,7 @@ extern bool member_like_constrained_friend_p	(tree);
>   extern bool fns_correspond			(tree, tree);
>   extern int decls_match				(tree, tree, bool = true);
>   extern bool maybe_version_functions		(tree, tree, bool);
> +extern bool validate_constexpr_redeclaration	(tree, tree);
>   extern bool merge_default_template_args		(tree, tree, bool);
>   extern tree duplicate_decls			(tree, tree,
>   						 bool hiding = false,
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index 6458e96bded..4ad68d609d7 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -1412,7 +1412,7 @@ check_redeclaration_exception_specification (tree new_decl,
>   /* Return true if OLD_DECL and NEW_DECL agree on constexprness.
>      Otherwise issue diagnostics.  */
>   
> -static bool
> +bool
>   validate_constexpr_redeclaration (tree old_decl, tree new_decl)
>   {
>     old_decl = STRIP_TEMPLATE (old_decl);
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index 70ad4cbf3b5..98e15878657 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -3706,6 +3706,8 @@ push_local_extern_decl_alias (tree decl)
>   	  /* Adjust visibility.  */
>   	  determine_visibility (alias);
>   	}
> +      else if (!validate_constexpr_redeclaration (alias, decl))
> +	return;
>       }
>   
>     retrofit_lang_decl (decl);
> diff --git a/gcc/testsuite/g++.dg/diagnostic/redeclaration-6.C b/gcc/testsuite/g++.dg/diagnostic/redeclaration-6.C
> new file mode 100644
> index 00000000000..ed8d4af7792
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/diagnostic/redeclaration-6.C
> @@ -0,0 +1,34 @@
> +// PR c++/116239
> +// { dg-do compile { target c++20 } }
> +
> +consteval void f1();
> +void f2();
> +constexpr void f3();
> +void f4();
> +consteval void f5();
> +constexpr void f6();
> +
> +void
> +g ()
> +{
> +  void f1();		// { dg-error "differs in .consteval." }
> +  consteval void f2();	// { dg-error "differs in .consteval." }
> +
> +  void f3();		// { dg-error "differs in .constexpr." }
> +  constexpr void f4();  // { dg-error "differs in .constexpr." }
> +
> +  consteval void f5();
> +  constexpr void f6();
> +
> +  void f7();
> +  consteval void f7();	// { dg-error "differs in .consteval." }
> +
> +  consteval void f8();
> +  void f8();		// { dg-error "differs in .consteval." }
> +
> +  void f9();
> +  constexpr void f9();	// { dg-error "differs in .constexpr." }
> +
> +  constexpr void f10();
> +  void f10();		// { dg-error "differs in .constexpr." }
> +}
> 
> base-commit: c755c7a32590e2eef5a8b062b9756c1513910246