Do not use TYPE_NEED_CONSTRUCTING in may_be_aliased

Hi,
this patch drops test for TYPE_NEEDS_CONSTRUCTING in tree.h and instead
sets TREE_READONLY to 0 for external vars of this type. For vars
declared locally we drop TREE_READONLY while expanding constructor.
Note that I have tried to drop TREE_READONLY always (not only for
DECL_EXTERNAL) and it breaks a testcase where constructor is constexpr.
So perhaps this is unnecesarily conservative for external vars having
constexpr cotr and perhaps it is better done by frontend.

Curiously enough, this does not fix the actual testcase in PR88677.

Bootstrapped/regtested x86_64-linux, makes sense?

	PR lto/88777
	* ipa-visibility.c (function_and_variable_visibility): Drop
	TREE_READONLY flag for variables where type needs constructing.
	* tree.h (may_be_aliased): Do not test TYPE_NEEDS_CONSTRUCTING.

> Hi,
> this patch drops test for TYPE_NEEDS_CONSTRUCTING in tree.h and instead
> sets TREE_READONLY to 0 for external vars of this type. For vars
> declared locally we drop TREE_READONLY while expanding constructor.
> Note that I have tried to drop TREE_READONLY always (not only for
> DECL_EXTERNAL) and it breaks a testcase where constructor is constexpr.
> So perhaps this is unnecesarily conservative for external vars having
> constexpr cotr and perhaps it is better done by frontend.
> 
> Curiously enough, this does not fix the actual testcase in PR88677.
This turned out to be bug in my patch: I cleared the flag too late so
free_lang_data caused very much same effect as the may_be_aliased flag.
Here is updated patch, bootstrapped/regtested x86_64-linux. It also
fixes the testcase though I am not quite sure how to add it to
testsuite.
> 
> Bootstrapped/regtested x86_64-linux, makes sense?
> 
 	PR lto/88777
	* cgraphunit.c (analyze_functions): Clear READONLY flag for external
	types that needs constructiong.
	* tree.h (may_be_aliased): Do not check TYPE_NEEDS_CONSTRUCTING.
Index: cgraphunit.c
===================================================================
--- cgraphunit.c	(revision 268741)
+++ cgraphunit.c	(working copy)
@@ -1226,6 +1226,15 @@ analyze_functions (bool first_time)
        && node != first_handled_var; node = next)
     {
       next = node->next;
+      /* For symbols declared locally we clear TREE_READONLY when emitting
+	 the construtor (if one is needed).  For external declarations we can
+	 not safely assume that the type is readonly because we may be called
+	 during its construction.  */
+      if (TREE_CODE (node->decl) == VAR_DECL
+	  && TYPE_P (TREE_TYPE (node->decl))
+	  && TYPE_NEEDS_CONSTRUCTING (TREE_TYPE (node->decl))
+	  && DECL_EXTERNAL (node->decl))
+	TREE_READONLY (node->decl) = 0;
       if (!node->aux && !node->referred_to_p ())
 	{
 	  if (symtab->dump_file)
Index: tree.h
===================================================================
--- tree.h	(revision 268741)
+++ tree.h	(working copy)
@@ -5371,8 +5371,7 @@ may_be_aliased (const_tree var)
 	      || DECL_EXTERNAL (var)
 	      || TREE_ADDRESSABLE (var))
 	  && !((TREE_STATIC (var) || TREE_PUBLIC (var) || DECL_EXTERNAL (var))
-	       && ((TREE_READONLY (var)
-		    && !TYPE_NEEDS_CONSTRUCTING (TREE_TYPE (var)))
+	       && (TREE_READONLY (var)
 		   || (TREE_CODE (var) == VAR_DECL
 		       && DECL_NONALIASED (var)))));
 }

On Sun, 10 Feb 2019, Jan Hubicka wrote:

> > Hi,
> > this patch drops test for TYPE_NEEDS_CONSTRUCTING in tree.h and instead
> > sets TREE_READONLY to 0 for external vars of this type. For vars
> > declared locally we drop TREE_READONLY while expanding constructor.
> > Note that I have tried to drop TREE_READONLY always (not only for
> > DECL_EXTERNAL) and it breaks a testcase where constructor is constexpr.
> > So perhaps this is unnecesarily conservative for external vars having
> > constexpr cotr and perhaps it is better done by frontend.
> > 
> > Curiously enough, this does not fix the actual testcase in PR88677.
> This turned out to be bug in my patch: I cleared the flag too late so
> free_lang_data caused very much same effect as the may_be_aliased flag.
> Here is updated patch, bootstrapped/regtested x86_64-linux. It also
> fixes the testcase though I am not quite sure how to add it to
> testsuite.

OK.

Richard.

> > 
> > Bootstrapped/regtested x86_64-linux, makes sense?
> > 
>  	PR lto/88777
> 	* cgraphunit.c (analyze_functions): Clear READONLY flag for external
> 	types that needs constructiong.
> 	* tree.h (may_be_aliased): Do not check TYPE_NEEDS_CONSTRUCTING.
> Index: cgraphunit.c
> ===================================================================
> --- cgraphunit.c	(revision 268741)
> +++ cgraphunit.c	(working copy)
> @@ -1226,6 +1226,15 @@ analyze_functions (bool first_time)
>         && node != first_handled_var; node = next)
>      {
>        next = node->next;
> +      /* For symbols declared locally we clear TREE_READONLY when emitting
> +	 the construtor (if one is needed).  For external declarations we can
> +	 not safely assume that the type is readonly because we may be called
> +	 during its construction.  */
> +      if (TREE_CODE (node->decl) == VAR_DECL
> +	  && TYPE_P (TREE_TYPE (node->decl))
> +	  && TYPE_NEEDS_CONSTRUCTING (TREE_TYPE (node->decl))
> +	  && DECL_EXTERNAL (node->decl))
> +	TREE_READONLY (node->decl) = 0;
>        if (!node->aux && !node->referred_to_p ())
>  	{
>  	  if (symtab->dump_file)
> Index: tree.h
> ===================================================================
> --- tree.h	(revision 268741)
> +++ tree.h	(working copy)
> @@ -5371,8 +5371,7 @@ may_be_aliased (const_tree var)
>  	      || DECL_EXTERNAL (var)
>  	      || TREE_ADDRESSABLE (var))
>  	  && !((TREE_STATIC (var) || TREE_PUBLIC (var) || DECL_EXTERNAL (var))
> -	       && ((TREE_READONLY (var)
> -		    && !TYPE_NEEDS_CONSTRUCTING (TREE_TYPE (var)))
> +	       && (TREE_READONLY (var)
>  		   || (TREE_CODE (var) == VAR_DECL
>  		       && DECL_NONALIASED (var)))));
>  }
> 
>