[PATCH] Fix up decl_binds_to_current_def_p (PR target/56564)

[PATCH] Fix up decl_binds_to_current_def_p (PR target/56564)

[Jakub Jelinek]

Hi!

As discussed on IRC, decl_binds_to_current_def_p doesn't handle
some cases properly, e.g. if a DECL_COMMON decl has hidden visibility,
then it returns true even without consulting linker plugin resolution,
even when the common might bind to a real definition of the var in some
other TU of the same module.

Fixed by adding new target hook for this.  Bootstrapped/regtested on
x86_64-linux and i686-linux, ok for trunk?

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

	PR target/56564
	* output.h (default_binds_to_current_def_p,
	default_binds_to_current_def_p_1): New prototypes.
	* varasm.c (default_binds_to_current_def_p,
	default_binds_to_current_def_p_1): New functions.
	(decl_binds_to_current_def): Use binds_to_current_def_p
	target hook.
	* target.def (binds_to_current_def_p): New hook.
	* doc/tm.texi.in (TARGET_BINDS_TO_CURRENT_DEF_P): Document.
	* doc/tm.texi: Regenerated.
	* config/i386/winnt.c (i386_pe_binds_to_current_def_p): New function.
	* config/i386/i386-protos.h (i386_pe_binds_to_current_def_p): New
	prototype.
	* config/i386/i386.c (TARGET_BINDS_TO_CURRENT_DEF_P): Redefine for
	PE and darwin.
	* config/darwin-protos.h (darwin_binds_to_current_def_p): New
	prototype.
	* config/darwin.c (darwin_binds_to_current_def_p): New function.
	* config/rs6000/rs6000.c (TARGET_BINDS_TO_CURRENT_DEF_P): Redefine for
	darwin.

--- gcc/output.h.jj	2013-01-11 09:03:01.000000000 +0100
+++ gcc/output.h	2013-06-11 08:40:22.247597454 +0200
@@ -583,6 +583,8 @@ extern void default_asm_output_anchor (r
 extern bool default_use_anchors_for_symbol_p (const_rtx);
 extern bool default_binds_local_p (const_tree);
 extern bool default_binds_local_p_1 (const_tree, int);
+extern bool default_binds_to_current_def_p (const_tree);
+extern bool default_binds_to_current_def_p_1 (const_tree, int);
 extern void default_globalize_label (FILE *, const char *);
 extern void default_globalize_decl_name (FILE *, tree);
 extern void default_emit_unwind_label (FILE *, tree, int, int);
--- gcc/varasm.c.jj1	2013-06-11 09:33:52.367820102 +0200
+++ gcc/varasm.c	2013-06-11 11:17:53.162448015 +0200
@@ -6766,46 +6766,80 @@ default_binds_local_p_1 (const_tree exp,
   return local_p;
 }
 
-/* Return true when references to DECL must bind to current definition in
-   final executable.
+/* Assume ELF-ish defaults, since that's pretty much the most liberal
+   wrt cross-module name binding.  */
 
-   The condition is usually equivalent to whether the function binds to the
-   current module (shared library or executable), that is to binds_local_p.
-   We use this fact to avoid need for another target hook and implement
-   the logic using binds_local_p and just special cases where
-   decl_binds_to_current_def_p is stronger than binds_local_p.  In particular
-   the weak definitions (that can be overwritten at linktime by other
-   definition from different object file) and when resolution info is available
-   we simply use the knowledge passed to us by linker plugin.  */
 bool
-decl_binds_to_current_def_p (tree decl)
+default_binds_to_current_def_p (const_tree exp)
 {
-  gcc_assert (DECL_P (decl));
-  if (!TREE_PUBLIC (decl))
+  return default_binds_to_current_def_p_1 (exp, flag_shlib);
+}
+
+bool
+default_binds_to_current_def_p_1 (const_tree exp, int shlib)
+{
+  /* Weakrefs may not bind to the current def, even though the weakref itself is
+     always static and therefore local.  Similarly, the resolver for ifunc functions
+     might resolve to a non-local function.
+     FIXME: We can resolve the weakref case more curefuly by looking at the
+     weakref alias.  */
+  if (lookup_attribute ("weakref", DECL_ATTRIBUTES (exp))
+      || (TREE_CODE (exp) == FUNCTION_DECL
+	  && lookup_attribute ("ifunc", DECL_ATTRIBUTES (exp))))
+    return false;
+  /* Static variables are always local.  */
+  if (! TREE_PUBLIC (exp))
     return true;
-  if (!targetm.binds_local_p (decl))
+  /* If PIC, then assume that any global name can be overridden by
+     symbols resolved from other modules, unless it has non-default
+     visibility.  */
+  if (shlib && DECL_VISIBILITY (exp) == VISIBILITY_DEFAULT)
     return false;
-  /* When resolution is available, just use it.  */
-  if (TREE_CODE (decl) == VAR_DECL
-      && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
+  /* With resolution file in hands, take look into resolutions.  */
+  if (TREE_CODE (exp) == VAR_DECL && TREE_PUBLIC (exp)
+      && (TREE_STATIC (exp) || DECL_EXTERNAL (exp)))
     {
-      struct varpool_node *vnode = varpool_get_node (decl);
+      struct varpool_node *vnode = varpool_get_node (exp);
       if (vnode
-	  && vnode->symbol.resolution != LDPR_UNKNOWN)
-	return resolution_to_local_definition_p (vnode->symbol.resolution);
+	  && resolution_to_local_definition_p (vnode->symbol.resolution))
+	return true;
     }
-  else if (TREE_CODE (decl) == FUNCTION_DECL)
+  else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp))
     {
-      struct cgraph_node *node = cgraph_get_node (decl);
+      struct cgraph_node *node = cgraph_get_node (exp);
       if (node
-	  && node->symbol.resolution != LDPR_UNKNOWN)
-	return resolution_to_local_definition_p (node->symbol.resolution);
+	  && resolution_to_local_definition_p (node->symbol.resolution))
+	return true;
     }
-  /* Otherwise we have to assume the worst for DECL_WEAK (hidden weaks
-     binds locally but still can be overwritten).
-     This rely on fact that binds_local_p behave as decl_replaceable_p
-     for all other declaration types.  */
-  return !DECL_WEAK (decl);
+  /* Variables defined outside this object might not be local.  */
+  if (DECL_EXTERNAL (exp))
+    return false;
+  /* Weak data can be overridden by a strong symbol in another TU
+     and so might not bind to current def.  */
+  if (DECL_WEAK (exp))
+    return false;
+  /* Uninitialized COMMON variable may be unified with symbols
+     resolved from other TUs.  */
+  if (DECL_COMMON (exp)
+      && (DECL_INITIAL (exp) == NULL
+	  || DECL_INITIAL (exp) == error_mark_node))
+    return false;
+  /* Otherwise we're left with initialized (or non-common) global data
+     which is of necessity bound to the current def.  */
+  return true;
+}
+
+/* Return true when references to DECL must bind to current definition in
+   final executable.
+
+   The condition is often equivalent to whether the function binds to the
+   current module (shared library or executable), that is to binds_local_p.  */
+
+bool
+decl_binds_to_current_def_p (tree decl)
+{
+  gcc_assert (DECL_P (decl));
+  return targetm.binds_to_current_def_p (decl);
 }
 
 /* A replaceable function or variable is one which may be replaced
--- gcc/target.def.jj	2013-06-01 14:47:19.000000000 +0200
+++ gcc/target.def	2013-06-11 08:41:33.901416274 +0200
@@ -1569,6 +1569,14 @@ DEFHOOK
  bool, (const_tree exp),
  default_binds_local_p)
 
+/* True if EXP names an object for which name resolution must resolve
+   to the definition in the current translation unit.  */
+DEFHOOK
+(binds_to_current_def_p,
+ "",
+ bool, (const_tree exp),
+ default_binds_to_current_def_p)
+
 /* Check if profiling code is before or after prologue.  */
 DEFHOOK
 (profile_before_prologue,
--- gcc/doc/tm.texi.in.jj	2013-06-10 18:16:38.000000000 +0200
+++ gcc/doc/tm.texi.in	2013-06-11 08:49:39.208401706 +0200
@@ -7082,6 +7082,15 @@ for ELF, which has a looser model of glo
 currently supported object file formats.
 @end deftypefn
 
+@hook TARGET_BINDS_TO_CURRENT_DEF_P
+Returns true if @var{exp} names an object for which name resolution
+rules must resolve to the definition in the current translation unit.
+
+The default version of this hook implements the name resolution rules
+for ELF, which has a looser model of global name binding than other
+currently supported object file formats.
+@end deftypefn
+
 @hook TARGET_HAVE_TLS
 Contains the value true if the target supports thread-local storage.
 The default value is false.
--- gcc/doc/tm.texi.jj	2013-06-10 18:16:38.000000000 +0200
+++ gcc/doc/tm.texi	2013-06-11 08:50:16.933780317 +0200
@@ -7200,6 +7200,15 @@ for ELF, which has a looser model of glo
 currently supported object file formats.
 @end deftypefn
 
+@deftypefn {Target Hook} bool TARGET_BINDS_TO_CURRENT_DEF_P (const_tree @var{exp})
+Returns true if @var{exp} names an object for which name resolution
+rules must resolve to the definition in the current translation unit.
+
+The default version of this hook implements the name resolution rules
+for ELF, which has a looser model of global name binding than other
+currently supported object file formats.
+@end deftypefn
+
 @deftypevr {Target Hook} bool TARGET_HAVE_TLS
 Contains the value true if the target supports thread-local storage.
 The default value is false.
--- gcc/config/i386/winnt.c.jj	2013-06-01 14:47:25.000000000 +0200
+++ gcc/config/i386/winnt.c	2013-06-11 08:47:30.710520722 +0200
@@ -368,6 +368,16 @@ i386_pe_binds_local_p (const_tree exp)
   return default_binds_local_p_1 (exp, 0);
 }
 
+bool
+i386_pe_binds_to_current_def_p (const_tree exp)
+{
+  if ((TREE_CODE (exp) == VAR_DECL || TREE_CODE (exp) == FUNCTION_DECL)
+      && DECL_DLLIMPORT_P (exp))
+    return false;
+
+  return default_binds_to_current_def_p_1 (exp, 0);
+}
+
 /* Also strip the fastcall prefix and stdcall suffix.  */
 
 const char *
--- gcc/config/i386/i386-protos.h.jj	2013-06-10 18:16:38.000000000 +0200
+++ gcc/config/i386/i386-protos.h	2013-06-11 08:47:54.823121655 +0200
@@ -246,6 +246,7 @@ extern void i386_pe_record_external_func
 extern void i386_pe_maybe_record_exported_symbol (tree, const char *, int);
 extern void i386_pe_encode_section_info (tree, rtx, int);
 extern bool i386_pe_binds_local_p (const_tree);
+extern bool i386_pe_binds_to_current_def_p (const_tree);
 extern const char *i386_pe_strip_name_encoding_full (const char *);
 extern bool i386_pe_valid_dllimport_attribute_p (const_tree);
 extern unsigned int i386_pe_section_type_flags (tree, const char *, int);
--- gcc/config/i386/i386.c.jj	2013-06-10 18:16:38.000000000 +0200
+++ gcc/config/i386/i386.c	2013-06-11 08:46:21.326665064 +0200
@@ -42814,10 +42814,14 @@ ix86_memmodel_check (unsigned HOST_WIDE_
 #if TARGET_MACHO
 #undef TARGET_BINDS_LOCAL_P
 #define TARGET_BINDS_LOCAL_P darwin_binds_local_p
+#undef TARGET_BINDS_TO_CURRENT_DEF_P
+#define TARGET_BINDS_TO_CURRENT_DEF_P darwin_binds_to_current_def_p
 #endif
 #if TARGET_DLLIMPORT_DECL_ATTRIBUTES
 #undef TARGET_BINDS_LOCAL_P
 #define TARGET_BINDS_LOCAL_P i386_pe_binds_local_p
+#undef TARGET_BINDS_TO_CURRENT_DEF_P
+#define TARGET_BINDS_TO_CURRENT_DEF_P i386_pe_binds_to_current_def_p
 #endif
 
 #undef TARGET_ASM_OUTPUT_MI_THUNK
--- gcc/config/darwin-protos.h.jj	2013-01-11 09:03:07.000000000 +0100
+++ gcc/config/darwin-protos.h	2013-06-11 08:45:23.621620232 +0200
@@ -108,6 +108,7 @@ extern void darwin_asm_output_aligned_de
 						   unsigned int);
 
 extern bool darwin_binds_local_p (const_tree);
+extern bool darwin_binds_to_current_def_p (const_tree);
 extern void darwin_cpp_builtins (struct cpp_reader *);
 
 extern tree darwin_init_cfstring_builtins (unsigned);
--- gcc/config/darwin.c.jj	2013-02-12 11:23:35.000000000 +0100
+++ gcc/config/darwin.c	2013-06-11 08:44:42.406298454 +0200
@@ -2933,6 +2933,17 @@ darwin_binds_local_p (const_tree decl)
 				  TARGET_KEXTABI && DARWIN_VTABLE_P (decl));
 }
 
+/* Name binding to current TU.  Darwin does not support overriding
+   functions at dynamic-link time, except for vtables in kexts.  */
+
+bool
+darwin_binds_to_current_def_p (const_tree decl)
+{
+  return default_binds_to_current_def_p_1 (decl,
+					   TARGET_KEXTABI
+					   && DARWIN_VTABLE_P (decl));
+}
+
 /* The Darwin's implementation of TARGET_ASM_OUTPUT_ANCHOR.  Define the
    anchor relative to ".", the current section position.  We cannot use
    the default one because ASM_OUTPUT_DEF is wrong for Darwin.  */
--- gcc/config/rs6000/rs6000.c.jj	2013-06-11 08:01:26.000000000 +0200
+++ gcc/config/rs6000/rs6000.c	2013-06-11 09:10:31.585540013 +0200
@@ -1335,6 +1335,8 @@ static const struct attribute_spec rs600
 #if TARGET_MACHO
 #undef TARGET_BINDS_LOCAL_P
 #define TARGET_BINDS_LOCAL_P darwin_binds_local_p
+#undef TARGET_BINDS_TO_CURRENT_DEF_P
+#define TARGET_BINDS_TO_CURRENT_DEF_P darwin_binds_to_current_def_p
 #endif
 
 #undef TARGET_MS_BITFIELD_LAYOUT_P

	Jakub

Re: [PATCH] Fix up decl_binds_to_current_def_p (PR target/56564)

[Richard Henderson]

On 06/11/2013 04:31 AM, Jakub Jelinek wrote:
> +bool
> +default_binds_to_current_def_p_1 (const_tree exp, int shlib)
> +{
...
> +}

I'm having trouble picking out what's supposed to be different between this
function and default_binds_local_p_1.  It appears to be the same set of tests,
just rearranged into a possibly more efficient form requiring less runtime
evaluation.


r~

Re: [PATCH] Fix up decl_binds_to_current_def_p (PR target/56564)

[Jakub Jelinek]

On Tue, Jun 11, 2013 at 09:56:28AM -0700, Richard Henderson wrote:
> On 06/11/2013 04:31 AM, Jakub Jelinek wrote:
> > +bool
> > +default_binds_to_current_def_p_1 (const_tree exp, int shlib)
> > +{
> ...
> > +}
> 
> I'm having trouble picking out what's supposed to be different between this
> function and default_binds_local_p_1.  It appears to be the same set of tests,
> just rearranged into a possibly more efficient form requiring less runtime
> evaluation.

What is different is
1) weakref/ifunc handling without !TREE_PUBLIC (not sure about that though;
   the old version would return true for !TREE_PUBLIC right away, but
   e.g. default_binds_local_p_1 checks TREE_PUBLIC only after testing
   for weakref)
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?

	Jakub

Re: [PATCH] Fix up decl_binds_to_current_def_p (PR target/56564)

[Richard Henderson]

On 06/11/2013 10:03 AM, Jakub Jelinek wrote:
> On Tue, Jun 11, 2013 at 09:56:28AM -0700, Richard Henderson wrote:
>> I'm having trouble picking out what's supposed to be different between this
>> function and default_binds_local_p_1.  It appears to be the same set of tests,
>> just rearranged into a possibly more efficient form requiring less runtime
>> evaluation.
> 
> What is different is
> 1) weakref/ifunc handling without !TREE_PUBLIC (not sure about that though;
>    the old version would return true for !TREE_PUBLIC right away, but
>    e.g. default_binds_local_p_1 checks TREE_PUBLIC only after testing
>    for weakref)

This sounds wrong even for binds_local_p.

(1a) The documentation says that weakrefs must be static, which implies that
TREE_PUBLIC must be 0.  I guess that assumption could be invalidated by
manipulations in the code, but I don't see them in handle_weakref_attribute.
If someone knows better, please chime in...

(1b) One *can* declare a static ifunc, but that only means that the resolver
isn't reachable from outside the translation unit.  The resolver, being user
code, is free to return absolutely anything, so we can't be sure about whence
it binds.

> 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.


r~

Re: [PATCH] Fix up decl_binds_to_current_def_p (PR target/56564)

[Jakub Jelinek]

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.

--- 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


	Jakub

Re: [PATCH] Fix up decl_binds_to_current_def_p (PR target/56564)

[Richard Henderson]

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~

Re: [PATCH] Fix up decl_binds_to_current_def_p (PR target/56564)

[Jan Hubicka]

> 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

Re: [PATCH] Fix up decl_binds_to_current_def_p (PR target/56564)

[Jakub Jelinek]

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

Re: [PATCH] Fix up decl_binds_to_current_def_p (PR target/56564)

[Jan Hubicka]

> 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