c++: Fix up push_local_extern_decl_alias error recovery [PR102642]

  Hi!

My recent push_local_extern_decl_alias change broke error-recovery,
do_pushdecl can return error_mark_node and set_decl_tls_model can't be
called on that.  There are other code paths that store error_mark_node
into DECL_LOCAL_DECL_ALIAS, with the intent to differentiate the cases
where we haven't yet tried to push it into the namespace scope (NULL)
and one where we have tried it but it failed (error_mark_node), but looking
around, there are other spots where we call functions or do processing
which doesn't tolerate error_mark_node.

So, the first hunk with the testcase fixes the testcase, the others
fix what I've spotted and the fix was easy to figure out (there are I think
3 other spots mainly for function multiversioning).

Ok for trunk and 11.3 (where I've backported the tls fix before) if it
passes bootstrap/regtest?

2021-10-11  Jakub Jelinek  <jakub@redhat.com>

	PR c++/102642
	* name-lookup.c (push_local_extern_decl_alias): Don't call
	set_decl_tls_model on error_mark_node.
	* decl.c (make_rtl_for_nonlocal_decl): Don't call
	set_user_assembler_name on error_mark_node.
	* parser.c (cp_parser_oacc_declare): Ignore DECL_LOCAL_DECL_ALIAS
	if it is error_mark_node.
	(cp_parser_omp_declare_target): Likewise.

	* g++.dg/tls/pr102642.C: New test.


	Jakub
  

On 10/11/21 07:18, Jakub Jelinek wrote:
> Hi!
> 
> My recent push_local_extern_decl_alias change broke error-recovery,
> do_pushdecl can return error_mark_node and set_decl_tls_model can't be
> called on that.  There are other code paths that store error_mark_node
> into DECL_LOCAL_DECL_ALIAS, with the intent to differentiate the cases
> where we haven't yet tried to push it into the namespace scope (NULL)
> and one where we have tried it but it failed (error_mark_node), but looking
> around, there are other spots where we call functions or do processing
> which doesn't tolerate error_mark_node.
> 
> So, the first hunk with the testcase fixes the testcase, the others
> fix what I've spotted and the fix was easy to figure out (there are I think
> 3 other spots mainly for function multiversioning).

What if we use NULL_TREE for the error case instead of error_mark_node, i.e.

-  DECL_LOCAL_DECL_ALIAS (decl) = alias;
+  if (alias != error_mark_node)
+    DECL_LOCAL_DECL_ALIAS (decl) = alias;

?  That ought to avoid the need to change other functions, since they 
already check for null.

> Ok for trunk and 11.3 (where I've backported the tls fix before) if it
> passes bootstrap/regtest?
> 
> 2021-10-11  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/102642
> 	* name-lookup.c (push_local_extern_decl_alias): Don't call
> 	set_decl_tls_model on error_mark_node.
> 	* decl.c (make_rtl_for_nonlocal_decl): Don't call
> 	set_user_assembler_name on error_mark_node.
> 	* parser.c (cp_parser_oacc_declare): Ignore DECL_LOCAL_DECL_ALIAS
> 	if it is error_mark_node.
> 	(cp_parser_omp_declare_target): Likewise.
> 
> 	* g++.dg/tls/pr102642.C: New test.
> 
> --- gcc/cp/name-lookup.c.jj	2021-10-01 10:30:07.674588541 +0200
> +++ gcc/cp/name-lookup.c	2021-10-11 12:43:39.261051228 +0200
> @@ -3474,7 +3474,9 @@ push_local_extern_decl_alias (tree decl)
>   	  push_nested_namespace (ns);
>   	  alias = do_pushdecl (alias, /* hiding= */true);
>   	  pop_nested_namespace (ns);
> -	  if (VAR_P (decl) && CP_DECL_THREAD_LOCAL_P (decl))
> +	  if (VAR_P (decl)
> +	      && CP_DECL_THREAD_LOCAL_P (decl)
> +	      && alias != error_mark_node)
>   	    set_decl_tls_model (alias, DECL_TLS_MODEL (decl));
>   	}
>       }
> --- gcc/cp/decl.c.jj	2021-10-09 10:07:51.883704975 +0200
> +++ gcc/cp/decl.c	2021-10-11 12:49:33.810977118 +0200
> @@ -7373,7 +7373,8 @@ make_rtl_for_nonlocal_decl (tree decl, t
>   		 This is horrible, as we're affecting a
>   		 possibly-shared decl.  Again, a one-true-decl
>   		 model breaks down.  */
> -	      set_user_assembler_name (ns_decl, asmspec);
> +	      if (ns_decl != error_mark_node)
> +		set_user_assembler_name (ns_decl, asmspec);
>   	}
>       }
>   
> --- gcc/cp/parser.c.jj	2021-10-09 10:14:24.043098112 +0200
> +++ gcc/cp/parser.c	2021-10-11 12:47:21.220874667 +0200
> @@ -44437,7 +44437,8 @@ cp_parser_oacc_declare (cp_parser *parse
>   	       dependent local extern variable decls are as rare as
>   	       hen's teeth.  */
>   	    if (auto alias = DECL_LOCAL_DECL_ALIAS (decl))
> -	      decl = alias;
> +	      if (alias != error_mark_node)
> +		decl = alias;
>   
>   	  if (OMP_CLAUSE_MAP_KIND (t) == GOMP_MAP_LINK)
>   	    id = get_identifier ("omp declare target link");
> @@ -45665,7 +45666,8 @@ cp_parser_omp_declare_target (cp_parser
>         if (VAR_OR_FUNCTION_DECL_P (t)
>   	  && DECL_LOCAL_DECL_P (t)
>   	  && DECL_LANG_SPECIFIC (t)
> -	  && DECL_LOCAL_DECL_ALIAS (t))
> +	  && DECL_LOCAL_DECL_ALIAS (t)
> +	  && DECL_LOCAL_DECL_ALIAS (t) != error_mark_node)
>   	handle_omp_declare_target_clause (c, DECL_LOCAL_DECL_ALIAS (t),
>   					  device_type);
>       }
> --- gcc/testsuite/g++.dg/tls/pr102642.C.jj	2021-10-11 13:00:35.889503002 +0200
> +++ gcc/testsuite/g++.dg/tls/pr102642.C	2021-10-11 13:00:20.388724721 +0200
> @@ -0,0 +1,10 @@
> +// PR c++/102642
> +// { dg-do compile { target c++11 } }
> +
> +thread_local int *z;		// { dg-message "previous declaration" }
> +
> +void
> +foo ()
> +{
> +  extern thread_local int z;	// { dg-error "conflicting declaration" }
> +}
> 
> 	Jakub
>
  

On Tue, Oct 19, 2021 at 10:48:04AM -0400, Jason Merrill wrote:
> What if we use NULL_TREE for the error case instead of error_mark_node, i.e.
> 
> -  DECL_LOCAL_DECL_ALIAS (decl) = alias;
> +  if (alias != error_mark_node)
> +    DECL_LOCAL_DECL_ALIAS (decl) = alias;
> 
> ?  That ought to avoid the need to change other functions, since they
> already check for null.

True, but I'm worried about e.g. maybe_version_functions which does
  if (DECL_LOCAL_DECL_P (olddecl))
    {
      olddecl = DECL_LOCAL_DECL_ALIAS (olddecl);
      maybe_mark_function_versioned (olddecl);
    }
(I think the above will crash of DECL_LOCAL_DECL_ALIAS is NULL) and
  if (DECL_LOCAL_DECL_P (newdecl))
    {
      /* Unfortunately, we can get here before pushdecl naturally calls
         push_local_extern_decl_alias, so we need to call it directly.  */
      if (!DECL_LOCAL_DECL_ALIAS (newdecl))
        push_local_extern_decl_alias (newdecl);
      newdecl = DECL_LOCAL_DECL_ALIAS (newdecl);
      maybe_mark_function_versioned (newdecl);
    }
which means that if there is an error from push_local_extern_decl_alias,
we'd report it twice rather than once (once from here, another time
from do_pushdecl).
I'm afraid maybe_version_functions needs fixing no matter what,
but the NULL vs. error_mark_node decision is probably dependent on whether
it is ok to error twice or not.

	Jakub
  

On 10/19/21 11:21, Jakub Jelinek wrote:
> On Tue, Oct 19, 2021 at 10:48:04AM -0400, Jason Merrill wrote:
>> What if we use NULL_TREE for the error case instead of error_mark_node, i.e.
>>
>> -  DECL_LOCAL_DECL_ALIAS (decl) = alias;
>> +  if (alias != error_mark_node)
>> +    DECL_LOCAL_DECL_ALIAS (decl) = alias;
>>
>> ?  That ought to avoid the need to change other functions, since they
>> already check for null.
> 
> True, but I'm worried about e.g. maybe_version_functions which does
>    if (DECL_LOCAL_DECL_P (olddecl))
>      {
>        olddecl = DECL_LOCAL_DECL_ALIAS (olddecl);
>        maybe_mark_function_versioned (olddecl);
>      }
> (I think the above will crash of DECL_LOCAL_DECL_ALIAS is NULL) and
>    if (DECL_LOCAL_DECL_P (newdecl))
>      {
>        /* Unfortunately, we can get here before pushdecl naturally calls
>           push_local_extern_decl_alias, so we need to call it directly.  */
>        if (!DECL_LOCAL_DECL_ALIAS (newdecl))
>          push_local_extern_decl_alias (newdecl);
>        newdecl = DECL_LOCAL_DECL_ALIAS (newdecl);
>        maybe_mark_function_versioned (newdecl);
>      }
> which means that if there is an error from push_local_extern_decl_alias,
> we'd report it twice rather than once (once from here, another time
> from do_pushdecl).
> I'm afraid maybe_version_functions needs fixing no matter what,
> but the NULL vs. error_mark_node decision is probably dependent on whether
> it is ok to error twice or not.

Ah, true.  Your earlier patch is OK.

Jason