diff mbox

Fix up decl_binds_to_current_def_p (PR target/56564)

Message ID 20130611113151.GY1493@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek June 11, 2013, 11:31 a.m. UTC
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.


	Jakub

Comments

Richard Henderson June 11, 2013, 4:56 p.m. UTC | #1
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~
Jakub Jelinek June 11, 2013, 5:03 p.m. UTC | #2
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
Richard Henderson June 11, 2013, 5:18 p.m. UTC | #3
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~
diff mbox

Patch

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