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

["H.J. Lu" <hongjiu dot lu at intel dot com>]

If a hidden weak symbol isn't defined in the TU, we can't assume it will
be defined in another TU at link time.  It makes a difference in code
generation when compiling for PIC. If we assume that a hidden weak
undefined symbol is local, the address checking may be optimized out and
leads to the wrong code.  This means that a symbol with user specified
visibility is local only if it is locally resolved or defined, not weak
or not compiling for PIC.  When symbol visibility is specified in the
source, we should always output symbol visibility even if symbol isn't
local to the TU.

If a global data symbol is defined in the TU, it is always local to the
executable, regardless if it is a common symbol or not.  If we aren't
compiling for shared library, locally defined global data symbol binds
locally.

Tested on Linux/x86-64.  OK for trunk?

Thanks.


H.J.
---
gcc/

	PR rtl-optimization/32219
	* cgraphunit.c (varpool_node::finalize_decl): Set definition
	first before calling notice_global_symbol so that it is
	available to notice_global_symbol.
	* varasm.c (default_binds_local_p_1): Resolve defined data
	symbol locally if not building shared library.  Resolve symbol
	with user specified visibility locally only if it is locally
	resolved or defined, not weak or not compiling for PIC.
	(default_elf_asm_output_external): Always output visibility
	specified in the source.

gcc/testsuite/

	PR rtl-optimization/32219
	* gcc.dg/visibility-22.c: New test.
	* gcc.dg/visibility-23.c: Likewise.
	* gcc.target/i386/pr32219-1.c: Likewise.
	* gcc.target/i386/pr32219-2.c: Likewise.
	* gcc.target/i386/pr32219-3.c: Likewise.
	* gcc.target/i386/pr32219-4.c: Likewise.
	* gcc.target/i386/pr32219-5.c: Likewise.
	* gcc.target/i386/pr32219-6.c: Likewise.
	* gcc.target/i386/pr32219-7.c: Likewise.
	* gcc.target/i386/pr32219-8.c: Likewise.
	* gcc.target/i386/pr64317.c: Expect GOTOFF relocation instead
	of GOT relocation.
---
 gcc/cgraphunit.c                          |  4 +++-
 gcc/testsuite/gcc.dg/visibility-22.c      | 14 +++++++++++++
 gcc/testsuite/gcc.dg/visibility-23.c      | 15 +++++++++++++
 gcc/testsuite/gcc.target/i386/pr32219-1.c | 16 ++++++++++++++
 gcc/testsuite/gcc.target/i386/pr32219-2.c | 16 ++++++++++++++
 gcc/testsuite/gcc.target/i386/pr32219-3.c | 17 +++++++++++++++
 gcc/testsuite/gcc.target/i386/pr32219-4.c | 17 +++++++++++++++
 gcc/testsuite/gcc.target/i386/pr32219-5.c | 16 ++++++++++++++
 gcc/testsuite/gcc.target/i386/pr32219-6.c | 16 ++++++++++++++
 gcc/testsuite/gcc.target/i386/pr32219-7.c | 17 +++++++++++++++
 gcc/testsuite/gcc.target/i386/pr32219-8.c | 17 +++++++++++++++
 gcc/testsuite/gcc.target/i386/pr64317.c   |  2 +-
 gcc/varasm.c                              | 35 ++++++++++++++++++-------------
 13 files changed, 185 insertions(+), 17 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/visibility-22.c
 create mode 100644 gcc/testsuite/gcc.dg/visibility-23.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr32219-1.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr32219-2.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr32219-3.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr32219-4.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr32219-5.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr32219-6.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr32219-7.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr32219-8.c

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index a2650f7..e1a6e41 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -792,8 +792,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.dg/visibility-22.c b/gcc/testsuite/gcc.dg/visibility-22.c
new file mode 100644
index 0000000..30087de
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/visibility-22.c
@@ -0,0 +1,14 @@
+/* PR target/32219 */
+/* { dg-do run } */
+/* { dg-require-visibility "" } */
+/* { dg-options "-fPIC" { target fpic } } */
+
+extern void foo () __attribute__((weak,visibility("hidden")));
+int
+main()
+{
+  if (foo)
+    foo ();
+  return 0;
+}
+
diff --git a/gcc/testsuite/gcc.dg/visibility-23.c b/gcc/testsuite/gcc.dg/visibility-23.c
new file mode 100644
index 0000000..070493f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/visibility-23.c
@@ -0,0 +1,15 @@
+/* PR target/32219 */
+/* { dg-do compile } */
+/* { dg-require-visibility "" } */
+/* { dg-final { scan-hidden "foo" } } */
+/* { dg-options "-fPIC" { target fpic } } */
+
+extern void foo () __attribute__((weak,visibility("hidden")));
+int
+main()
+{
+  if (foo)
+    foo ();
+  return 0;
+}
+
diff --git a/gcc/testsuite/gcc.target/i386/pr32219-1.c b/gcc/testsuite/gcc.target/i386/pr32219-1.c
new file mode 100644
index 0000000..5bd80a0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr32219-1.c
@@ -0,0 +1,16 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpie" } */
+
+/* Common symbol with -fpie.  */
+int xxx;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler "movl\[ \t\]xxx\\(%rip\\), %eax" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "movl\[ \t\]xxx@GOTOFF\\(%\[^,\]*\\), %eax" { target ia32 } } } */
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx@GOT\\(%\[^,\]*\\), %eax" { target ia32 } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr32219-2.c b/gcc/testsuite/gcc.target/i386/pr32219-2.c
new file mode 100644
index 0000000..0cf2eb5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr32219-2.c
@@ -0,0 +1,16 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Common symbol with -fpic.  */
+int xxx;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx\\(%rip\\), %eax" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx@GOTOFF\\(%\[^,\]*\\), %eax" { target ia32 } } } */
+/* { dg-final { scan-assembler "movl\[ \t\]xxx@GOT\\(%\[^,\]*\\), %eax" { target ia32 } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr32219-3.c b/gcc/testsuite/gcc.target/i386/pr32219-3.c
new file mode 100644
index 0000000..911f2a5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr32219-3.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpie" } */
+
+/* Weak common symbol with -fpie.  */
+__attribute__((weak))
+int xxx;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler "movl\[ \t\]xxx\\(%rip\\), %eax" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "movl\[ \t\]xxx@GOTOFF\\(%\[^,\]*\\), %eax" { target ia32 } } } */
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx@GOT\\(%\[^,\]*\\), %eax" { target ia32 } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr32219-4.c b/gcc/testsuite/gcc.target/i386/pr32219-4.c
new file mode 100644
index 0000000..3d43439
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr32219-4.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Weak common symbol with -fpic.  */
+__attribute__((weak))
+int xxx;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx\\(%rip\\), %eax" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx@GOTOFF\\(%\[^,\]*\\), %eax" { target ia32 } } } */
+/* { dg-final { scan-assembler "movl\[ \t\]xxx@GOT\\(%\[^,\]*\\), %eax" { target ia32 } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr32219-5.c b/gcc/testsuite/gcc.target/i386/pr32219-5.c
new file mode 100644
index 0000000..ee7442e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr32219-5.c
@@ -0,0 +1,16 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpie" } */
+
+/* Initialized symbol with -fpie.  */
+int xxx = -1;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler "movl\[ \t\]xxx\\(%rip\\), %eax" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "movl\[ \t\]xxx@GOTOFF\\(%\[^,\]*\\), %eax" { target ia32 } } } */
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx@GOT\\(%\[^,\]*\\), %eax" { target ia32 } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr32219-6.c b/gcc/testsuite/gcc.target/i386/pr32219-6.c
new file mode 100644
index 0000000..f261433
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr32219-6.c
@@ -0,0 +1,16 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Initialized symbol with -fpic.  */
+int xxx = -1;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx\\(%rip\\), %eax" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx@GOTOFF\\(%\[^,\]*\\), %eax" { target ia32 } } } */
+/* { dg-final { scan-assembler "movl\[ \t\]xxx@GOT\\(%\[^,\]*\\), %eax" { target ia32 } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr32219-7.c b/gcc/testsuite/gcc.target/i386/pr32219-7.c
new file mode 100644
index 0000000..12aaf72
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr32219-7.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpie" } */
+
+/* Weak initialized symbol with -fpie.  */
+__attribute__((weak))
+int xxx = -1;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler "movl\[ \t\]xxx\\(%rip\\), %eax" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "movl\[ \t\]xxx@GOTOFF\\(%\[^,\]*\\), %eax" { target ia32 } } } */
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx@GOT\\(%\[^,\]*\\), %eax" { target ia32 } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr32219-8.c b/gcc/testsuite/gcc.target/i386/pr32219-8.c
new file mode 100644
index 0000000..2e4fba0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr32219-8.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Weak initialized symbol with -fpic.  */
+__attribute__((weak))
+int xxx = -1;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx\\(%rip\\), %eax" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "movl\[ \t\]xxx@GOTOFF\\(%\[^,\]*\\), %eax" { target ia32 } } } */
+/* { dg-final { scan-assembler "movl\[ \t\]xxx@GOT\\(%\[^,\]*\\), %eax" { target ia32 } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr64317.c b/gcc/testsuite/gcc.target/i386/pr64317.c
index 46c3c6f..a23e996 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 { ia32 } } } */
 /* { dg-options "-O2 -fPIE -pie" } */
 /* { 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/varasm.c b/gcc/varasm.c
index eb65b1f..f7c13af 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6826,11 +6826,17 @@ 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 (vnode)
+	{
+	  if ((!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))
     {
@@ -6859,9 +6865,14 @@ 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 or
+     not weak.  */
   else if ((DECL_VISIBILITY_SPECIFIED (exp)
 	    || resolved_to_local_def)
+	   && (resolved_locally
+	       || !flag_pic
+	       || !DECL_EXTERNAL (exp)
+	       || !DECL_WEAK (exp))
 	   && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
     local_p = true;
   /* Variables defined outside this object might not be local.  */
@@ -6880,13 +6891,6 @@ default_binds_local_p_1 (const_tree exp, int shlib)
      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;
   /* Otherwise we're left with initialized (or non-common) global data
      which is of necessity defined locally.  */
   else
@@ -7445,9 +7449,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);
 }
 
-- 
1.9.3