diff mbox

PR rtl-optimization/32219: optimizer causes wrong code in pic/hidden/weak symbol checking

Message ID 54DA75D2.40402@redhat.com
State New
Headers show

Commit Message

Richard Henderson Feb. 10, 2015, 9:19 p.m. UTC
On 02/07/2015 08:45 AM, H.J. Lu wrote:
> Here is an updated patch.

This is an annoying comment to differentiate 3 different versions, FYI.

> @@ -6826,11 +6817,18 @@ default_binds_local_p_1 (const_tree exp, int shlib)
>        && (TREE_STATIC (exp) || DECL_EXTERNAL (exp)))
>      {
>        varpool_node *vnode = varpool_node::get (exp);
> -      if (vnode && (resolution_local_p (vnode->resolution) || vnode->in_other_partition))
> -	resolved_locally = true;
> -      if (vnode
> -	  && resolution_to_local_definition_p (vnode->resolution))
> -	resolved_to_local_def = true;
> +      /* If not building shared library, common or initialized symbols
> +	 are also resolved locally, regardless they are weak or not if
> +	 weak_dominate is true.  */
> +      if (vnode)
> +	{
> +	  if ((weak_dominate && !shlib && vnode->definition)
> +	      || vnode->in_other_partition
> +	      || resolution_local_p (vnode->resolution))
> +	    resolved_locally = true;
> +	  if (resolution_to_local_definition_p (vnode->resolution))
> +	    resolved_to_local_def = true;
> +	}
>      }
>    else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp))
>      {

Not the same test for a function_decl?  That's certainly a mistake.

Indeed, I believe we can now unify these two cases by using the base
symtab_node instead of varpool_node and cgraph_node.

I'm a bit confused about why we have both resolved_to_local_def vs
resolved_locally.  At least within this function, which only cares about module
locality rather than object file locality.  Honza?

> @@ -6859,9 +6857,15 @@ default_binds_local_p_1 (const_tree exp, int shlib)
>    else if (! TREE_PUBLIC (exp))
>      local_p = true;
>    /* A variable is local if the user has said explicitly that it will
> -     be.  */
> +     be and it is resolved or defined locally, not compiling for PIC,
> +     not weak or weak_dominate is false.  */
>    else if ((DECL_VISIBILITY_SPECIFIED (exp)
>  	    || resolved_to_local_def)
> +	   && (!weak_dominate
> +	       || resolved_locally
> +	       || !flag_pic
> +	       || !DECL_EXTERNAL (exp)
> +	       || !DECL_WEAK (exp))
>  	   && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
>      local_p = true;

I think this test is conflating several issues, and as such it's very confusing.

As an existing issue, I'm not sure why "specified" visibility is any different
from unspecified visibility.  As far as I'm aware, the "specified" bit simply
means that the decl doesn't inherit inherit visibility from the class, or from
the command-line.  But once we're this far, the visibility actually applied to
the symbol should be all that matters.

Unfortunately, this test is 11 years old, from r85167.  It came in as part of
the implementation of the visibility attribute for the class.

I believe the next test should be

  else if (DECL_WEAK (exp) && !resolved_locally)
    local_p = false;

Given the change above, resolved_locally will now be true for (weak && defined
&& weak_dominate), and therefore we need not reiterate that test.

I believe the next test should be dictated by normal non-lto usage like

  extern void bar(void) __attribute__((visibility("hidden")));
  void foo(void) { ... bar(); ...)

where the user is expecting that the function call make use of a simpler
instruction sequence, probably avoiding an PLT.  Therefore

  else if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
    local_p = true;

Since we have already excluded undef-weak, we know that any symbol here is
either defined or strong, and non-default visibility must resolve it locally.

>    /* Variables defined outside this object might not be local.  */
> @@ -6881,8 +6885,9 @@ default_binds_local_p_1 (const_tree exp, int shlib)
>    else if (shlib)
>      local_p = false;
>    /* Uninitialized COMMON variable may be unified with symbols
> -     resolved from other modules.  */
> +     resolved from other modules unless weak_dominate is true.  */
>    else if (DECL_COMMON (exp)
> +	   && !weak_dominate
>  	   && !resolved_locally

Like the weak test, surely weak_dominate has already been accounted for.

> @@ -7445,9 +7465,10 @@ default_elf_asm_output_external (FILE *file ATTRIBUTE_UNUSED,
>  {
>    /* We output the name if and only if TREE_SYMBOL_REFERENCED is
>       set in order to avoid putting out names that are never really
> -     used. */
> +     used.   Always output visibility specified in the source.  */
>    if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl))
> -      && targetm.binds_local_p (decl))
> +      && (DECL_VISIBILITY_SPECIFIED (decl)
> +	  || targetm.binds_local_p (decl)))
>      maybe_assemble_visibility (decl);

Explain?

I'm testing the following in the meantime, in order to validate my hypotheses
above.


r~

Comments

H.J. Lu Feb. 10, 2015, 9:25 p.m. UTC | #1
On Tue, Feb 10, 2015 at 1:19 PM, Richard Henderson <rth@redhat.com> wrote:
>> @@ -7445,9 +7465,10 @@ default_elf_asm_output_external (FILE *file ATTRIBUTE_UNUSED,
>>  {
>>    /* We output the name if and only if TREE_SYMBOL_REFERENCED is
>>       set in order to avoid putting out names that are never really
>> -     used. */
>> +     used.   Always output visibility specified in the source.  */
>>    if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl))
>> -      && targetm.binds_local_p (decl))
>> +      && (DECL_VISIBILITY_SPECIFIED (decl)
>> +       || targetm.binds_local_p (decl)))
>>      maybe_assemble_visibility (decl);
>
> Explain?
>

This is for the new testcase in my patch:

[hjl@gnu-mic-2 testsuite]$ cat gcc.dg/visibility-23.c
/* PR target/32219 */
/* { dg-do compile } */
/* { dg-require-visibility "" } */
/* { dg-final { scan-hidden "foo" } } */
/* { dg-options "-O2 -fPIC" { target fpic } } */
/* { dg-skip-if "" { "hppa*-*-hpux*" "*-*-aix*" "*-*-darwin*" } "*" { "" } }  */

extern void foo () __attribute__((weak,visibility("hidden")));
int
main()
{
  if (foo)
    foo ();
  return 0;
}
[hjl@gnu-mic-2 testsuite]$

Here targetm.binds_local_p now returns false.  But we must
emit ".hidden foo" even if foo isn't defined in the TU.  Otherwise,
foo wont be hidden if it isn't defined in another TU.
Jack Howarth Feb. 11, 2015, 2:35 p.m. UTC | #2
Richard,
      Your proposed patch fails to bootstrap here on
x86_64-apple-darwin14 against the system clang compilers...

g++ -c   -g  -DIN_GCC    -fno-exceptions -fno-rtti
-fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings
-Wcast-qual -Wno-format -Wmissing-format-attribute
-Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros
-Wno-overlength-strings -fno-common  -DHAVE_CONFIG_H -I. -I.
-I../../gcc-5-20150211/gcc -I../../gcc-5-20150211/gcc/.
-I../../gcc-5-20150211/gcc/../include
-I../../gcc-5-20150211/gcc/../libcpp/include -I/sw/include
-I/sw/include  -I../../gcc-5-20150211/gcc/../libdecnumber
-I../../gcc-5-20150211/gcc/../libdecnumber/dpd -I../libdecnumber
-I../../gcc-5-20150211/gcc/../libbacktrace -I/sw/include -I/sw/include
-o varasm.o -MT varasm.o -MMD -MP -MF ./.deps/varasm.TPo
../../gcc-5-20150211/gcc/varasm.c
...
../../gcc-5-20150211/gcc/varasm.c:6899:9: error: use of undeclared
identifier 'resolution_to_local_definition_p'
        return resolution_to_local_definition_p (vnode->resolution);
               ^
../../gcc-5-20150211/gcc/varasm.c:6906:9: error: use of undeclared
identifier 'resolution_to_local_definition_p'
        return resolution_to_local_definition_p (node->resolution);
               ^
        Jack


On Tue, Feb 10, 2015 at 4:19 PM, Richard Henderson <rth@redhat.com> wrote:
> On 02/07/2015 08:45 AM, H.J. Lu wrote:
>> Here is an updated patch.
>
> This is an annoying comment to differentiate 3 different versions, FYI.
>
>> @@ -6826,11 +6817,18 @@ default_binds_local_p_1 (const_tree exp, int shlib)
>>        && (TREE_STATIC (exp) || DECL_EXTERNAL (exp)))
>>      {
>>        varpool_node *vnode = varpool_node::get (exp);
>> -      if (vnode && (resolution_local_p (vnode->resolution) || vnode->in_other_partition))
>> -     resolved_locally = true;
>> -      if (vnode
>> -       && resolution_to_local_definition_p (vnode->resolution))
>> -     resolved_to_local_def = true;
>> +      /* If not building shared library, common or initialized symbols
>> +      are also resolved locally, regardless they are weak or not if
>> +      weak_dominate is true.  */
>> +      if (vnode)
>> +     {
>> +       if ((weak_dominate && !shlib && vnode->definition)
>> +           || vnode->in_other_partition
>> +           || resolution_local_p (vnode->resolution))
>> +         resolved_locally = true;
>> +       if (resolution_to_local_definition_p (vnode->resolution))
>> +         resolved_to_local_def = true;
>> +     }
>>      }
>>    else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp))
>>      {
>
> Not the same test for a function_decl?  That's certainly a mistake.
>
> Indeed, I believe we can now unify these two cases by using the base
> symtab_node instead of varpool_node and cgraph_node.
>
> I'm a bit confused about why we have both resolved_to_local_def vs
> resolved_locally.  At least within this function, which only cares about module
> locality rather than object file locality.  Honza?
>
>> @@ -6859,9 +6857,15 @@ default_binds_local_p_1 (const_tree exp, int shlib)
>>    else if (! TREE_PUBLIC (exp))
>>      local_p = true;
>>    /* A variable is local if the user has said explicitly that it will
>> -     be.  */
>> +     be and it is resolved or defined locally, not compiling for PIC,
>> +     not weak or weak_dominate is false.  */
>>    else if ((DECL_VISIBILITY_SPECIFIED (exp)
>>           || resolved_to_local_def)
>> +        && (!weak_dominate
>> +            || resolved_locally
>> +            || !flag_pic
>> +            || !DECL_EXTERNAL (exp)
>> +            || !DECL_WEAK (exp))
>>          && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
>>      local_p = true;
>
> I think this test is conflating several issues, and as such it's very confusing.
>
> As an existing issue, I'm not sure why "specified" visibility is any different
> from unspecified visibility.  As far as I'm aware, the "specified" bit simply
> means that the decl doesn't inherit inherit visibility from the class, or from
> the command-line.  But once we're this far, the visibility actually applied to
> the symbol should be all that matters.
>
> Unfortunately, this test is 11 years old, from r85167.  It came in as part of
> the implementation of the visibility attribute for the class.
>
> I believe the next test should be
>
>   else if (DECL_WEAK (exp) && !resolved_locally)
>     local_p = false;
>
> Given the change above, resolved_locally will now be true for (weak && defined
> && weak_dominate), and therefore we need not reiterate that test.
>
> I believe the next test should be dictated by normal non-lto usage like
>
>   extern void bar(void) __attribute__((visibility("hidden")));
>   void foo(void) { ... bar(); ...)
>
> where the user is expecting that the function call make use of a simpler
> instruction sequence, probably avoiding an PLT.  Therefore
>
>   else if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
>     local_p = true;
>
> Since we have already excluded undef-weak, we know that any symbol here is
> either defined or strong, and non-default visibility must resolve it locally.
>
>>    /* Variables defined outside this object might not be local.  */
>> @@ -6881,8 +6885,9 @@ default_binds_local_p_1 (const_tree exp, int shlib)
>>    else if (shlib)
>>      local_p = false;
>>    /* Uninitialized COMMON variable may be unified with symbols
>> -     resolved from other modules.  */
>> +     resolved from other modules unless weak_dominate is true.  */
>>    else if (DECL_COMMON (exp)
>> +        && !weak_dominate
>>          && !resolved_locally
>
> Like the weak test, surely weak_dominate has already been accounted for.
>
>> @@ -7445,9 +7465,10 @@ default_elf_asm_output_external (FILE *file ATTRIBUTE_UNUSED,
>>  {
>>    /* We output the name if and only if TREE_SYMBOL_REFERENCED is
>>       set in order to avoid putting out names that are never really
>> -     used. */
>> +     used.   Always output visibility specified in the source.  */
>>    if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl))
>> -      && targetm.binds_local_p (decl))
>> +      && (DECL_VISIBILITY_SPECIFIED (decl)
>> +       || targetm.binds_local_p (decl)))
>>      maybe_assemble_visibility (decl);
>
> Explain?
>
> I'm testing the following in the meantime, in order to validate my hypotheses
> above.
>
>
> r~
>
diff mbox

Patch

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 48a4b35..a1ed927 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -803,8 +803,10 @@  varpool_node::finalize_decl (tree decl)
 
   if (node->definition)
     return;
-  notice_global_symbol (decl);
+  /* Set definition first before calling notice_global_symbol so that
+     it is available to notice_global_symbol.  */
   node->definition = true;
+  notice_global_symbol (decl);
   if (TREE_THIS_VOLATILE (decl) || DECL_PRESERVE_P (decl)
       /* Traditionally we do not eliminate static variables when not
 	 optimizing and when not doing toplevel reoder.  */
diff --git a/gcc/testsuite/gcc.target/i386/pr64317.c b/gcc/testsuite/gcc.target/i386/pr64317.c
index 33f5b5d..32969fc 100644
--- a/gcc/testsuite/gcc.target/i386/pr64317.c
+++ b/gcc/testsuite/gcc.target/i386/pr64317.c
@@ -1,7 +1,7 @@ 
 /* { dg-do compile { target { *-*-linux* && ia32 } } } */
 /* { dg-options "-O2 -fpie" } */
 /* { dg-final { scan-assembler "addl\[ \\t\]+\[$\]_GLOBAL_OFFSET_TABLE_, %ebx" } } */
-/* { dg-final { scan-assembler "movl\[ \\t\]+c@GOT\[(\]%ebx\[)\]" } } */
+/* { dg-final { scan-assembler "movl\[ \\t\]+c@GOTOFF\[(\]%ebx\[)\]" } } */
 /* { dg-final { scan-assembler-not "movl\[ \\t\]+\[0-9]+\[(\]%esp\[)\], %ebx" } } */
 long c;
 
diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
index 159fa2b..3c45f37 100644
--- a/gcc/tree-eh.c
+++ b/gcc/tree-eh.c
@@ -3859,6 +3859,17 @@  mark_reachable_handlers (sbitmap *r_reachablep, sbitmap *lp_reachablep)
 			      gimple_eh_dispatch_region (
                                 as_a <geh_dispatch *> (stmt)));
 	      break;
+	    case GIMPLE_CALL:
+	      if (gimple_call_builtin_p (stmt, BUILT_IN_EH_COPY_VALUES))
+		for (int i = 0; i < 2; ++i)
+		  {
+		    tree rt = gimple_call_arg (stmt, i);
+		    HOST_WIDE_INT ri = tree_to_shwi (rt);
+
+		    gcc_assert (ri = (int)ri);
+		    bitmap_set_bit (r_reachable, ri);
+		  }
+	      break;
 	    default:
 	      break;
 	    }
diff --git a/gcc/varasm.c b/gcc/varasm.c
index eb65b1f..9d041c7 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6776,123 +6776,98 @@  default_use_anchors_for_symbol_p (const_rtx symbol)
   return true;
 }
 
-/* Return true when RESOLUTION indicate that symbol will be bound to the
-   definition provided by current .o file.  */
-
 static bool
-resolution_to_local_definition_p (enum ld_plugin_symbol_resolution resolution)
+default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
 {
-  return (resolution == LDPR_PREVAILING_DEF
-	  || resolution == LDPR_PREVAILING_DEF_IRONLY_EXP
-	  || resolution == LDPR_PREVAILING_DEF_IRONLY);
-}
-
-/* Return true when RESOLUTION indicate that symbol will be bound locally
-   within current executable or DSO.  */
-
-static bool
-resolution_local_p (enum ld_plugin_symbol_resolution resolution)
-{
-  return (resolution == LDPR_PREVAILING_DEF
-	  || resolution == LDPR_PREVAILING_DEF_IRONLY
-	  || resolution == LDPR_PREVAILING_DEF_IRONLY_EXP
-	  || resolution == LDPR_PREEMPTED_REG
-	  || resolution == LDPR_PREEMPTED_IR
-	  || resolution == LDPR_RESOLVED_IR
-	  || resolution == LDPR_RESOLVED_EXEC);
-}
-
-/* Assume ELF-ish defaults, since that's pretty much the most liberal
-   wrt cross-module name binding.  */
+  /* A non-decl is an entry in the constant pool.  */
+  if (!DECL_P (exp))
+    return true;
 
-bool
-default_binds_local_p (const_tree exp)
-{
-  return default_binds_local_p_1 (exp, flag_shlib);
-}
+  /* Weakrefs may not bind locally, 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;
 
-bool
-default_binds_local_p_1 (const_tree exp, int shlib)
-{
-  bool local_p;
-  bool resolved_locally = false;
-  bool resolved_to_local_def = false;
+  /* Static variables are always local.  */
+  if (! TREE_PUBLIC (exp))
+    return true;
 
   /* With resolution file in hands, take look into resolutions.
      We can't just return true for resolved_locally symbols,
      because dynamic linking might overwrite symbols
      in shared libraries.  */
+  symtab_node *node = NULL;
   if (TREE_CODE (exp) == VAR_DECL && TREE_PUBLIC (exp)
       && (TREE_STATIC (exp) || DECL_EXTERNAL (exp)))
-    {
-      varpool_node *vnode = varpool_node::get (exp);
-      if (vnode && (resolution_local_p (vnode->resolution) || vnode->in_other_partition))
-	resolved_locally = true;
-      if (vnode
-	  && resolution_to_local_definition_p (vnode->resolution))
-	resolved_to_local_def = true;
-    }
+    node = varpool_node::get (exp);
   else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp))
+    node = cgraph_node::get (exp);
+
+  bool resolved_locally = false;
+  if (node)
     {
-      struct cgraph_node *node = cgraph_node::get (exp);
-      if (node
-	  && (resolution_local_p (node->resolution) || node->in_other_partition))
+      /* When not building shared library and weak_dominate is true:
+         weak, common or initialized symbols are resolved locally.  */
+      if ((weak_dominate && !shlib && node->definition)
+	  || node->in_other_partition
+	  || node->resolution == LDPR_PREVAILING_DEF
+	  || node->resolution == LDPR_PREVAILING_DEF_IRONLY
+	  || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP
+	  || node->resolution == LDPR_PREEMPTED_REG
+	  || node->resolution == LDPR_PREEMPTED_IR
+	  || node->resolution == LDPR_RESOLVED_IR
+	  || node->resolution == LDPR_RESOLVED_EXEC)
 	resolved_locally = true;
-      if (node
-	  && resolution_to_local_definition_p (node->resolution))
-	resolved_to_local_def = true;
     }
 
-  /* A non-decl is an entry in the constant pool.  */
-  if (!DECL_P (exp))
-    local_p = true;
-  /* Weakrefs may not bind locally, 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.  */
-  else if (lookup_attribute ("weakref", DECL_ATTRIBUTES (exp))
-	   || (TREE_CODE (exp) == FUNCTION_DECL
-	       && lookup_attribute ("ifunc", DECL_ATTRIBUTES (exp))))
-    local_p = false;
-  /* Static variables are always local.  */
-  else if (! TREE_PUBLIC (exp))
-    local_p = true;
-  /* A variable is local if the user has said explicitly that it will
-     be.  */
-  else if ((DECL_VISIBILITY_SPECIFIED (exp)
-	    || resolved_to_local_def)
-	   && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
-    local_p = true;
+  /* Undefined (or non-dominant) weak symbols are not defined locally.  */
+  if (DECL_WEAK (exp) && !resolved_locally)
+    return false;
+
+  /* If a strong or defined weak symbol has non-default visibility,
+     it must be defined locally.  */
+  if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
+    return true;
+
   /* 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;
-  /* Default visibility weak data can be overridden by a strong symbol
-     in another module and so are not local.  */
-  else if (DECL_WEAK (exp)
-	   && !resolved_locally)
-    local_p = false;
-  /* If PIC, then assume that any global name can be overridden by
-     symbols resolved from other modules.  */
-  else if (shlib)
-    local_p = false;
-  /* Uninitialized COMMON variable may be unified with symbols
-     resolved from other modules.  */
-  else if (DECL_COMMON (exp)
-	   && !resolved_locally
-	   && (DECL_INITIAL (exp) == NULL
-	       || (!in_lto_p && DECL_INITIAL (exp) == error_mark_node)))
-    local_p = false;
+  if (DECL_EXTERNAL (exp) && !resolved_locally)
+    return false;
+
+  /* When compiling for a shared library, then assume that any global name
+     can be overridden by symbols resolved from other modules.  */
+  if (shlib)
+    return false;
+
+  /* Uninitialized COMMON variable may be unified with symbols.  */
+  if (DECL_COMMON (exp)
+      && !resolved_locally
+      && (DECL_INITIAL (exp) == NULL
+	  || (!in_lto_p && DECL_INITIAL (exp) == error_mark_node)))
+    return false;
+
   /* Otherwise we're left with initialized (or non-common) global data
      which is of necessity defined locally.  */
-  else
-    local_p = true;
+  return true;
+}
+
+/* Assume ELF-ish defaults, since that's pretty much the most liberal
+   wrt cross-module name binding.  */
 
-  return local_p;
+bool
+default_binds_local_p (const_tree exp)
+{
+  return default_binds_local_p_2 (exp, flag_shlib != 0, true);
+}
+
+bool
+default_binds_local_p_1 (const_tree exp, int shlib)
+{
+  return default_binds_local_p_2 (exp, shlib != 0, false);
 }
 
 /* Return true when references to DECL must bind to current definition in
@@ -7445,9 +7420,10 @@  default_elf_asm_output_external (FILE *file ATTRIBUTE_UNUSED,
 {
   /* We output the name if and only if TREE_SYMBOL_REFERENCED is
      set in order to avoid putting out names that are never really
-     used. */
+     used.   Always output visibility specified in the source.  */
   if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl))
-      && targetm.binds_local_p (decl))
+      && (DECL_VISIBILITY_SPECIFIED (decl)
+	  || targetm.binds_local_p (decl)))
     maybe_assemble_visibility (decl);
 }