diff mbox

Fix up decl_binds_to_current_def_p (PR target/56564)

Message ID 20130611182048.GI2336@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek June 11, 2013, 6:20 p.m. UTC 
On Tue, Jun 11, 2013 at 10:18:17AM -0700, Richard Henderson wrote:
> > 2) DECL_COMMON with non-default visibility - the old version would return
> >    true, now it returns false unless linker plugin tells us the current
> >    common was used
> > 
> > Or do you think we should just do what we did before and just
> > handle the 1) and 2) cases explicitly in the decl_binds_to_current_def_p
> > function?
> 
> This (2) sounds like something that we could handle in d_b_t_c_d_p without
> having to duplicate all of binds_local_p.

Ok, here are all the changes to d_b_t_c_d_p.  Any holes in this?  Untested
so far.

2013-06-11  Jakub Jelinek  <jakub@redhat.com>

	PR target/56564
	* varasm.c (decl_binds_to_current_def_p): Call binds_local_p
	target hook even for !TREE_PUBLIC decls.  If no resolution info
	is available, return false for common and external decls.



	Jakub

Comments

Richard Henderson June 11, 2013, 6:29 p.m. UTC | #1 
On 06/11/2013 11:20 AM, Jakub Jelinek wrote:
> 2013-06-11  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/56564
> 	* varasm.c (decl_binds_to_current_def_p): Call binds_local_p
> 	target hook even for !TREE_PUBLIC decls.  If no resolution info
> 	is available, return false for common and external decls.

Looks good.  Ok if it passes testing.


r~
Jan Hubicka June 11, 2013, 6:39 p.m. UTC | #2 
> 2013-06-11  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/56564
> 	* varasm.c (decl_binds_to_current_def_p): Call binds_local_p
> 	target hook even for !TREE_PUBLIC decls.  If no resolution info
> 	is available, return false for common and external decls.
> 
> --- gcc/varasm.c.jj	2013-06-11 20:11:34.000000000 +0200
> +++ gcc/varasm.c	2013-06-11 20:15:15.729363893 +0200
> @@ -6781,10 +6781,10 @@ bool
>  decl_binds_to_current_def_p (tree decl)
>  {
>    gcc_assert (DECL_P (decl));
> -  if (!TREE_PUBLIC (decl))
> -    return true;
>    if (!targetm.binds_local_p (decl))
>      return false;
> +  if (!TREE_PUBLIC (decl))
> +    return true;
>    /* When resolution is available, just use it.  */
>    if (TREE_CODE (decl) == VAR_DECL
>        && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
> @@ -6802,10 +6802,20 @@ decl_binds_to_current_def_p (tree decl)
>  	return resolution_to_local_definition_p (node->symbol.resolution);
>      }
>    /* Otherwise we have to assume the worst for DECL_WEAK (hidden weaks
> -     binds locally but still can be overwritten).
> +     binds locally but still can be overwritten), DECL_COMMON (can be merged
> +     with a non-common definition somewhere in the same module) or
> +     DECL_EXTERNAL.
>       This rely on fact that binds_local_p behave as decl_replaceable_p
>       for all other declaration types.  */
> -  return !DECL_WEAK (decl);
> +  if (DECL_WEAK (decl))
> +    return false;
> +  if (DECL_COMMON (decl)
> +      && (DECL_INITIAL (decl) == NULL
> +	  || DECL_INITIAL (decl) == error_mark_node))
> +    return false;

As discussed on IRC, this will return unnecesarily conservative answer for
HIDDEN visibility and (resolution == LDPR_PREVAILING_DEF_IRONLY or resolution == LDPR_PREVAILING_DEF) symbols.

I belive in these cases for both returns above can be true.  here we know
symbol binds locally from linker.

We however can not trust resolution == LDPR_PREVAILING_DEF completely because
DEFAULT visibility symbols should still return false for shlib.

One case to handle it would bo whole_program_visibility to look for this case and remove
DECL_WEAK, DECL_COMMON and COMDAT flags.  It already does that for LDPR_PREVAILING_DEF_IRONLY
via make_decl_local, but I am not quite convinced we can safely do it for COMDATs that are
resolution == LDPR_PREVAILING_DEF, since it will make the symbol to change its type for
linker. Linker API specification do not speak about possibility of such changes.

While looking into the details, I also convinced myself that the following test:
  /* Variables defined outside this object might not be local.  */
  else if (DECL_EXTERNAL (exp) && !resolved_locally)
    local_p = false;
  /* If defined in this object and visibility is not default, must be
     local.  */
  else if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
    local_p = true;

should really stand
  /* Variables defined outside this object might not be local.  */
  else if (DECL_EXTERNAL (exp) && !resolved_locally)
    local_p = false;
  /* If defined in this object and visibility is not default, must be
     local.  */
  else if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT && !DECL_EXTERNAL (exp))
    local_p = true;

We do not know if the defining unit is defining the symbol with hidden visibility of not.
Perhaps linker can tell us via the resolution info, too?

At the moment I can not think of other problems here.
Honza
Jakub Jelinek June 11, 2013, 6:44 p.m. UTC | #3 
On Tue, Jun 11, 2013 at 08:39:07PM +0200, Jan Hubicka wrote:
> > @@ -6802,10 +6802,20 @@ decl_binds_to_current_def_p (tree decl)
> >  	return resolution_to_local_definition_p (node->symbol.resolution);
> >      }
> >    /* Otherwise we have to assume the worst for DECL_WEAK (hidden weaks
> > -     binds locally but still can be overwritten).
> > +     binds locally but still can be overwritten), DECL_COMMON (can be merged
> > +     with a non-common definition somewhere in the same module) or
> > +     DECL_EXTERNAL.
> >       This rely on fact that binds_local_p behave as decl_replaceable_p
> >       for all other declaration types.  */
> > -  return !DECL_WEAK (decl);
> > +  if (DECL_WEAK (decl))
> > +    return false;
> > +  if (DECL_COMMON (decl)
> > +      && (DECL_INITIAL (decl) == NULL
> > +	  || DECL_INITIAL (decl) == error_mark_node))
> > +    return false;
> 
> As discussed on IRC, this will return unnecesarily conservative answer for
> HIDDEN visibility and (resolution == LDPR_PREVAILING_DEF_IRONLY or resolution == LDPR_PREVAILING_DEF) symbols.

If resolution is not LDPR_UNKNOWN, then we don't enter this code at all.
In that case we simply check binds_local_p (it returns true for those),
!TREE_PUBLIC (false), and then just look at at the resolution (so
preexisting code).

	Jakub
Jan Hubicka June 11, 2013, 6:57 p.m. UTC | #4 
> On Tue, Jun 11, 2013 at 08:39:07PM +0200, Jan Hubicka wrote:
> > > @@ -6802,10 +6802,20 @@ decl_binds_to_current_def_p (tree decl)
> > >  	return resolution_to_local_definition_p (node->symbol.resolution);
> > >      }
> > >    /* Otherwise we have to assume the worst for DECL_WEAK (hidden weaks
> > > -     binds locally but still can be overwritten).
> > > +     binds locally but still can be overwritten), DECL_COMMON (can be merged
> > > +     with a non-common definition somewhere in the same module) or
> > > +     DECL_EXTERNAL.
> > >       This rely on fact that binds_local_p behave as decl_replaceable_p
> > >       for all other declaration types.  */
> > > -  return !DECL_WEAK (decl);
> > > +  if (DECL_WEAK (decl))
> > > +    return false;
> > > +  if (DECL_COMMON (decl)
> > > +      && (DECL_INITIAL (decl) == NULL
> > > +	  || DECL_INITIAL (decl) == error_mark_node))
> > > +    return false;
> > 
> > As discussed on IRC, this will return unnecesarily conservative answer for
> > HIDDEN visibility and (resolution == LDPR_PREVAILING_DEF_IRONLY or resolution == LDPR_PREVAILING_DEF) symbols.
> 
> If resolution is not LDPR_UNKNOWN, then we don't enter this code at all.
> In that case we simply check binds_local_p (it returns true for those),
> !TREE_PUBLIC (false), and then just look at at the resolution (so
> preexisting code).

Ah, sorry, you are right.  I overlooked that you kept the existing resolution
code around.

Except for the independent problem with default visibility on external symbols
in default_binds_local_p_1 I do not see any other issue.  I will send separate
patch for that tomorrow.

Thanks,
Honza
> 
> 	Jakub
diff mbox

Patch

--- gcc/varasm.c.jj	2013-06-11 20:11:34.000000000 +0200
+++ gcc/varasm.c	2013-06-11 20:15:15.729363893 +0200
@@ -6781,10 +6781,10 @@  bool
 decl_binds_to_current_def_p (tree decl)
 {
   gcc_assert (DECL_P (decl));
-  if (!TREE_PUBLIC (decl))
-    return true;
   if (!targetm.binds_local_p (decl))
     return false;
+  if (!TREE_PUBLIC (decl))
+    return true;
   /* When resolution is available, just use it.  */
   if (TREE_CODE (decl) == VAR_DECL
       && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
@@ -6802,10 +6802,20 @@  decl_binds_to_current_def_p (tree decl)
 	return resolution_to_local_definition_p (node->symbol.resolution);
     }
   /* Otherwise we have to assume the worst for DECL_WEAK (hidden weaks
-     binds locally but still can be overwritten).
+     binds locally but still can be overwritten), DECL_COMMON (can be merged
+     with a non-common definition somewhere in the same module) or
+     DECL_EXTERNAL.
      This rely on fact that binds_local_p behave as decl_replaceable_p
      for all other declaration types.  */
-  return !DECL_WEAK (decl);
+  if (DECL_WEAK (decl))
+    return false;
+  if (DECL_COMMON (decl)
+      && (DECL_INITIAL (decl) == NULL
+	  || DECL_INITIAL (decl) == error_mark_node))
+    return false;
+  if (DECL_EXTERNAL (decl))
+    return false;
+  return true;
 }
 
 /* A replaceable function or variable is one which may be replaced