[PATCH] PR target/65248: Copy relocation against protected symbol doesn't work

[PATCH] PR target/65248: Copy relocation against protected symbol doesn't work

[H.J. Lu]

Protected data symbol means that it can't be pre-emptied.  It doesn't mean
its address won't be external.  This is true for pointer to protected
function.  With copy relocation, address of protected data defined in the
shared library may also be external.  We only know that for sure at
run-time.  TARGET_BINDS_LOCAL_P should return false on protected data
symbol.  OK for trunk?

Thanks.

H.J.
---
	PR target/65248
	* output.h (default_binds_local_p_2): New.
	* varasm.c (default_binds_local_p_2): Renamed to ...
	(default_binds_local_p_3): This.  Don't return true on protected
	data symbol if protected data may be external.
	(default_binds_local_p): Use default_binds_local_p_3.
	(default_binds_local_p_1): Likewise.
	(default_binds_local_p_2): New.
	* config/i386/i386.c (TARGET_BINDS_LOCAL_P): Replace
	darwin_binds_local_p with default_binds_local_p_2.
---
 gcc/config/i386/i386.c |  3 +++
 gcc/output.h           |  1 +
 gcc/varasm.c           | 18 +++++++++++++++---
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index ab8f03a..41a487a 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -51878,6 +51878,9 @@ ix86_initialize_bounds (tree var, tree lb, tree ub, tree *stmts)
 #if TARGET_MACHO
 #undef TARGET_BINDS_LOCAL_P
 #define TARGET_BINDS_LOCAL_P darwin_binds_local_p
+#else
+#undef TARGET_BINDS_LOCAL_P
+#define TARGET_BINDS_LOCAL_P default_binds_local_p_2
 #endif
 #if TARGET_DLLIMPORT_DECL_ATTRIBUTES
 #undef TARGET_BINDS_LOCAL_P
diff --git a/gcc/output.h b/gcc/output.h
index 217d979..53e47d0 100644
--- a/gcc/output.h
+++ b/gcc/output.h
@@ -586,6 +586,7 @@ extern void default_asm_output_anchor (rtx);
 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_local_p_2 (const_tree);
 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);
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 8173207..87ac646 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6803,7 +6803,8 @@ resolution_local_p (enum ld_plugin_symbol_resolution resolution)
 }
 
 static bool
-default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
+default_binds_local_p_3 (const_tree exp, bool shlib, bool weak_dominate,
+			 bool extern_protected_data)
 {
   /* A non-decl is an entry in the constant pool.  */
   if (!DECL_P (exp))
@@ -6849,6 +6850,9 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
      or if we have a definition for the symbol.  We cannot infer visibility
      for undefined symbols.  */
   if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT
+      && (TREE_CODE (exp) == FUNCTION_DECL
+	  || !extern_protected_data
+	  || DECL_VISIBILITY (exp) != VISIBILITY_PROTECTED)
       && (DECL_VISIBILITY_SPECIFIED (exp) || defined_locally))
     return true;
 
@@ -6884,13 +6888,21 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
 bool
 default_binds_local_p (const_tree exp)
 {
-  return default_binds_local_p_2 (exp, flag_shlib != 0, true);
+  return default_binds_local_p_3 (exp, flag_shlib != 0, true, false);
+}
+
+/* Similar to default_binds_local_p, but protected data may be
+   external.  */
+bool
+default_binds_local_p_2 (const_tree exp)
+{
+  return default_binds_local_p_3 (exp, flag_shlib != 0, true, true);
 }
 
 bool
 default_binds_local_p_1 (const_tree exp, int shlib)
 {
-  return default_binds_local_p_2 (exp, shlib != 0, false);
+  return default_binds_local_p_3 (exp, shlib != 0, false, false);
 }
 
 /* Return true when references to DECL must bind to current definition in
-- 
1.9.3

Re: [PATCH] PR target/65248: Copy relocation against protected symbol doesn't work

[H.J. Lu]

On Thu, Mar 05, 2015 at 05:31:51PM -0800, H.J. Lu wrote:
> Protected data symbol means that it can't be pre-emptied.  It doesn't mean
> its address won't be external.  This is true for pointer to protected
> function.  With copy relocation, address of protected data defined in the
> shared library may also be external.  We only know that for sure at
> run-time.  TARGET_BINDS_LOCAL_P should return false on protected data
> symbol.  OK for trunk?
> 
> Thanks.
> 
> H.J.
> ---
> 	PR target/65248
> 	* output.h (default_binds_local_p_2): New.
> 	* varasm.c (default_binds_local_p_2): Renamed to ...
> 	(default_binds_local_p_3): This.  Don't return true on protected
> 	data symbol if protected data may be external.
> 	(default_binds_local_p): Use default_binds_local_p_3.
> 	(default_binds_local_p_1): Likewise.
> 	(default_binds_local_p_2): New.
> 	* config/i386/i386.c (TARGET_BINDS_LOCAL_P): Replace
> 	darwin_binds_local_p with default_binds_local_p_2.

Here is the updated patch with testcases.  Tested on Linux/x86.  OK
for trunk?

Thanks.


H.J.
----
gcc/

	PR target/65248
	* output.h (default_binds_local_p_2): New.
	* varasm.c (default_binds_local_p_2): Renamed to ...
	(default_binds_local_p_3): This.  Don't return true on protected
	data symbol if protected data may be external.
	(default_binds_local_p): Use default_binds_local_p_3.
	(default_binds_local_p_1): Likewise.
	(default_binds_local_p_2): New.
	* config/i386/i386.c (TARGET_BINDS_LOCAL_P): Replace
	darwin_binds_local_p with default_binds_local_p_2.

gcc/testsuite/

	PR target/65248
	* gcc.target/i386/pr65248-1.c: New file.
	* gcc.target/i386/pr65248-2.c: Likewise.
	* gcc.target/i386/pr65248-3.c: Likewise.
	* gcc.target/i386/pr65248-4.c: Likewise.
---
 gcc/config/i386/i386.c                    |  3 +++
 gcc/output.h                              |  1 +
 gcc/testsuite/gcc.target/i386/pr65248-1.c | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr65248-2.c | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr65248-3.c | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr65248-4.c | 17 +++++++++++++++++
 gcc/varasm.c                              | 18 +++++++++++++++---
 7 files changed, 87 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr65248-1.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr65248-2.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr65248-3.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr65248-4.c

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index ab8f03a..41a487a 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -51878,6 +51878,9 @@ ix86_initialize_bounds (tree var, tree lb, tree ub, tree *stmts)
 #if TARGET_MACHO
 #undef TARGET_BINDS_LOCAL_P
 #define TARGET_BINDS_LOCAL_P darwin_binds_local_p
+#else
+#undef TARGET_BINDS_LOCAL_P
+#define TARGET_BINDS_LOCAL_P default_binds_local_p_2
 #endif
 #if TARGET_DLLIMPORT_DECL_ATTRIBUTES
 #undef TARGET_BINDS_LOCAL_P
diff --git a/gcc/output.h b/gcc/output.h
index 217d979..53e47d0 100644
--- a/gcc/output.h
+++ b/gcc/output.h
@@ -586,6 +586,7 @@ extern void default_asm_output_anchor (rtx);
 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_local_p_2 (const_tree);
 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);
diff --git a/gcc/testsuite/gcc.target/i386/pr65248-1.c b/gcc/testsuite/gcc.target/i386/pr65248-1.c
new file mode 100644
index 0000000..7b0d2af
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr65248-1.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Common symbol with -fpic.  */
+__attribute__((visibility("protected")))
+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/pr65248-2.c b/gcc/testsuite/gcc.target/i386/pr65248-2.c
new file mode 100644
index 0000000..78efa34
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr65248-2.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Weak common symbol with -fpic.  */
+__attribute__((weak, visibility("protected")))
+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/pr65248-3.c b/gcc/testsuite/gcc.target/i386/pr65248-3.c
new file mode 100644
index 0000000..a126a26
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr65248-3.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Initialized symbol with -fpic.  */
+__attribute__((visibility("protected")))
+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/pr65248-4.c b/gcc/testsuite/gcc.target/i386/pr65248-4.c
new file mode 100644
index 0000000..ab730f7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr65248-4.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Weak initialized symbol with -fpic.  */
+__attribute__((weak, visibility("protected")))
+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/varasm.c b/gcc/varasm.c
index 8173207..87ac646 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6803,7 +6803,8 @@ resolution_local_p (enum ld_plugin_symbol_resolution resolution)
 }
 
 static bool
-default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
+default_binds_local_p_3 (const_tree exp, bool shlib, bool weak_dominate,
+			 bool extern_protected_data)
 {
   /* A non-decl is an entry in the constant pool.  */
   if (!DECL_P (exp))
@@ -6849,6 +6850,9 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
      or if we have a definition for the symbol.  We cannot infer visibility
      for undefined symbols.  */
   if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT
+      && (TREE_CODE (exp) == FUNCTION_DECL
+	  || !extern_protected_data
+	  || DECL_VISIBILITY (exp) != VISIBILITY_PROTECTED)
       && (DECL_VISIBILITY_SPECIFIED (exp) || defined_locally))
     return true;
 
@@ -6884,13 +6888,21 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
 bool
 default_binds_local_p (const_tree exp)
 {
-  return default_binds_local_p_2 (exp, flag_shlib != 0, true);
+  return default_binds_local_p_3 (exp, flag_shlib != 0, true, false);
+}
+
+/* Similar to default_binds_local_p, but protected data may be
+   external.  */
+bool
+default_binds_local_p_2 (const_tree exp)
+{
+  return default_binds_local_p_3 (exp, flag_shlib != 0, true, true);
 }
 
 bool
 default_binds_local_p_1 (const_tree exp, int shlib)
 {
-  return default_binds_local_p_2 (exp, shlib != 0, false);
+  return default_binds_local_p_3 (exp, shlib != 0, false, false);
 }
 
 /* Return true when references to DECL must bind to current definition in
-- 
1.9.3

Re: [PATCH] PR target/65248: Copy relocation against protected symbol doesn't work

[Uros Bizjak]

On Fri, Mar 6, 2015 at 2:42 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> On Thu, Mar 05, 2015 at 05:31:51PM -0800, H.J. Lu wrote:
>> Protected data symbol means that it can't be pre-emptied.  It doesn't mean
>> its address won't be external.  This is true for pointer to protected
>> function.  With copy relocation, address of protected data defined in the
>> shared library may also be external.  We only know that for sure at
>> run-time.  TARGET_BINDS_LOCAL_P should return false on protected data
>> symbol.  OK for trunk?
>>
>> Thanks.
>>
>> H.J.
>> ---
>>       PR target/65248
>>       * output.h (default_binds_local_p_2): New.
>>       * varasm.c (default_binds_local_p_2): Renamed to ...
>>       (default_binds_local_p_3): This.  Don't return true on protected
>>       data symbol if protected data may be external.
>>       (default_binds_local_p): Use default_binds_local_p_3.
>>       (default_binds_local_p_1): Likewise.
>>       (default_binds_local_p_2): New.
>>       * config/i386/i386.c (TARGET_BINDS_LOCAL_P): Replace
>>       darwin_binds_local_p with default_binds_local_p_2.
>
> Here is the updated patch with testcases.  Tested on Linux/x86.  OK
> for trunk?
>
> Thanks.
>
>
> H.J.
> ----
> gcc/
>
>         PR target/65248
>         * output.h (default_binds_local_p_2): New.
>         * varasm.c (default_binds_local_p_2): Renamed to ...
>         (default_binds_local_p_3): This.  Don't return true on protected
>         data symbol if protected data may be external.
>         (default_binds_local_p): Use default_binds_local_p_3.
>         (default_binds_local_p_1): Likewise.
>         (default_binds_local_p_2): New.
>         * config/i386/i386.c (TARGET_BINDS_LOCAL_P): Replace
>         darwin_binds_local_p with default_binds_local_p_2.
>
> gcc/testsuite/
>
>         PR target/65248
>         * gcc.target/i386/pr65248-1.c: New file.
>         * gcc.target/i386/pr65248-2.c: Likewise.
>         * gcc.target/i386/pr65248-3.c: Likewise.
>         * gcc.target/i386/pr65248-4.c: Likewise.

This patch needs global reviewer approval (I have added Jakub to CC)
and Darwin maintainer approval.

Uros.

Re: [PATCH] PR target/65248: Copy relocation against protected symbol doesn't work

[Mike Stump]

On Mar 18, 2015, at 2:55 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> 
>> Here is the updated patch with testcases.  Tested on Linux/x86.  OK
>> for trunk?

> This patch needs global reviewer approval (I have added Jakub to CC)
> and Darwin maintainer approval.

So, my concern would be this, does the bug also impact darwin, and does the bug fix also fix darwin?

If no and the change doesn’t change code-gen for darwin (which I think it does not), then the darwin bits are ok.

I did a quick check of the test case on darwin, no protected variables (we ignore the protected request).  When compiled, the program works (returns 0).

Re: [PATCH] PR target/65248: Copy relocation against protected symbol doesn't work

[H.J. Lu]

On Wed, Mar 18, 2015 at 11:58 AM, Mike Stump <mikestump@comcast.net> wrote:
> On Mar 18, 2015, at 2:55 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>
>>> Here is the updated patch with testcases.  Tested on Linux/x86.  OK
>>> for trunk?
>
>> This patch needs global reviewer approval (I have added Jakub to CC)
>> and Darwin maintainer approval.
>
> So, my concern would be this, does the bug also impact darwin, and does the bug fix also fix darwin?

This bug doesn't impact darwin.

> If no and the change doesn’t change code-gen for darwin (which I think it does not), then the darwin bits are ok.
>
> I did a quick check of the test case on darwin, no protected variables (we ignore the protected request).  When compiled, the program works (returns 0).

It is expected.  This bug only affects targets which support protected
visibility and use copy relocation.

-- 
H.J.

Re: [PATCH] PR target/65248: Copy relocation against protected symbol doesn't work

[H.J. Lu]

On Wed, Mar 18, 2015 at 12:11 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Mar 18, 2015 at 11:58 AM, Mike Stump <mikestump@comcast.net> wrote:
>> On Mar 18, 2015, at 2:55 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>>
>>>> Here is the updated patch with testcases.  Tested on Linux/x86.  OK
>>>> for trunk?
>>
>>> This patch needs global reviewer approval (I have added Jakub to CC)
>>> and Darwin maintainer approval.
>>
>> So, my concern would be this, does the bug also impact darwin, and does the bug fix also fix darwin?
>
> This bug doesn't impact darwin.
>
>> If no and the change doesn’t change code-gen for darwin (which I think it does not), then the darwin bits are ok.
>>
>> I did a quick check of the test case on darwin, no protected variables (we ignore the protected request).  When compiled, the program works (returns 0).
>
> It is expected.  This bug only affects targets which support protected
> visibility and use copy relocation.
>

Hi Jakub,

I'd like to fix this bug for GCC 5.  Is that OK for trunk:

https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00325.html

It only impacts Linux/x86.

Thanks.

-- 
H.J.

Re: [PATCH] PR target/65248: Copy relocation against protected symbol doesn't work

[Uros Bizjak]

On Fri, Mar 27, 2015 at 5:52 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Mar 18, 2015 at 12:11 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Wed, Mar 18, 2015 at 11:58 AM, Mike Stump <mikestump@comcast.net> wrote:
>>> On Mar 18, 2015, at 2:55 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>>>
>>>>> Here is the updated patch with testcases.  Tested on Linux/x86.  OK
>>>>> for trunk?
>>>
>>>> This patch needs global reviewer approval (I have added Jakub to CC)
>>>> and Darwin maintainer approval.
>>>
>>> So, my concern would be this, does the bug also impact darwin, and does the bug fix also fix darwin?
>>
>> This bug doesn't impact darwin.
>>
>>> If no and the change doesn’t change code-gen for darwin (which I think it does not), then the darwin bits are ok.
>>>
>>> I did a quick check of the test case on darwin, no protected variables (we ignore the protected request).  When compiled, the program works (returns 0).
>>
>> It is expected.  This bug only affects targets which support protected
>> visibility and use copy relocation.
>>
>
> Hi Jakub,
>
> I'd like to fix this bug for GCC 5.  Is that OK for trunk:
>
> https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00325.html
>
> It only impacts Linux/x86.

Jakub, does the patch look OK to you? I am not that familiar with this
part of the code ...

Uros.

Re: [PATCH] PR target/65248: Copy relocation against protected symbol doesn't work

[Richard Henderson]

On 03/06/2015 05:42 AM, H.J. Lu wrote:
> gcc/
> 
> 	PR target/65248
> 	* output.h (default_binds_local_p_2): New.
> 	* varasm.c (default_binds_local_p_2): Renamed to ...
> 	(default_binds_local_p_3): This.  Don't return true on protected
> 	data symbol if protected data may be external.
> 	(default_binds_local_p): Use default_binds_local_p_3.
> 	(default_binds_local_p_1): Likewise.
> 	(default_binds_local_p_2): New.
> 	* config/i386/i386.c (TARGET_BINDS_LOCAL_P): Replace
> 	darwin_binds_local_p with default_binds_local_p_2.

Ok.

> 	PR target/65248
> 	* gcc.target/i386/pr65248-1.c: New file.
> 	* gcc.target/i386/pr65248-2.c: Likewise.
> 	* gcc.target/i386/pr65248-3.c: Likewise.
> 	* gcc.target/i386/pr65248-4.c: Likewise.

These need some tidying, I think.

> +/* { 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 } } } */

The second line is what the rest of these should look like.  You don't need to
scan for movl or %eax.  The relocation used is the only thing that's relevant.
 Thus

	xxx\\(%rip\\)
	xxx@GOTPCREL
	xxx@GOTOFF
	xxx@GOT\\(

respectively.


r~

Re: [PATCH] PR target/65248: Copy relocation against protected symbol doesn't work

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]

On Fri, Mar 27, 2015 at 10:19 AM, Richard Henderson <rth@redhat.com> wrote:
> On 03/06/2015 05:42 AM, H.J. Lu wrote:
>> gcc/
>>
>>       PR target/65248
>>       * output.h (default_binds_local_p_2): New.
>>       * varasm.c (default_binds_local_p_2): Renamed to ...
>>       (default_binds_local_p_3): This.  Don't return true on protected
>>       data symbol if protected data may be external.
>>       (default_binds_local_p): Use default_binds_local_p_3.
>>       (default_binds_local_p_1): Likewise.
>>       (default_binds_local_p_2): New.
>>       * config/i386/i386.c (TARGET_BINDS_LOCAL_P): Replace
>>       darwin_binds_local_p with default_binds_local_p_2.
>
> Ok.
>
>>       PR target/65248
>>       * gcc.target/i386/pr65248-1.c: New file.
>>       * gcc.target/i386/pr65248-2.c: Likewise.
>>       * gcc.target/i386/pr65248-3.c: Likewise.
>>       * gcc.target/i386/pr65248-4.c: Likewise.
>
> These need some tidying, I think.
>
>> +/* { 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 } } } */
>
> The second line is what the rest of these should look like.  You don't need to
> scan for movl or %eax.  The relocation used is the only thing that's relevant.
>  Thus
>
>         xxx\\(%rip\\)
>         xxx@GOTPCREL
>         xxx@GOTOFF
>         xxx@GOT\\(
>
> respectively.
>

This is the patch I am checking in.

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-Add-default_binds_local_p_2-and-use-it-for-x86.patch --]
[-- Type: text/x-patch, Size: 7720 bytes --]

From ee047bc7f6f7cdaf955cc018ddd746b09b7708e2 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 27 Mar 2015 11:05:13 -0700
Subject: [PATCH] Add default_binds_local_p_2 and use it for x86

Protected data symbol means that it can't be pre-emptied.  It doesn't mean
its address won't be external.  This is true for pointer to protected
function.  With copy relocation, address of protected data defined in the
shared library may also be external.  We only know that for sure at
run-time.  TARGET_BINDS_LOCAL_P should return false on protected data
symbol.

gcc/

	PR target/65248
	* output.h (default_binds_local_p_2): New.
	* varasm.c (default_binds_local_p_2): Renamed to ...
	(default_binds_local_p_3): This.  Don't return true on protected
	data symbol if protected data may be external.
	(default_binds_local_p): Use default_binds_local_p_3.
	(default_binds_local_p_1): Likewise.
	(default_binds_local_p_2): New.
	* config/i386/i386.c (TARGET_BINDS_LOCAL_P): Replace
	darwin_binds_local_p with default_binds_local_p_2.

gcc/testsuite/

	PR target/65248
	* gcc.target/i386/pr65248-1.c: New file.
	* gcc.target/i386/pr65248-2.c: Likewise.
	* gcc.target/i386/pr65248-3.c: Likewise.
	* gcc.target/i386/pr65248-4.c: Likewise.
---
 gcc/config/i386/i386.c                    |  3 +++
 gcc/output.h                              |  1 +
 gcc/testsuite/gcc.target/i386/pr65248-1.c | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr65248-2.c | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr65248-3.c | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr65248-4.c | 17 +++++++++++++++++
 gcc/varasm.c                              | 18 +++++++++++++++---
 7 files changed, 87 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr65248-1.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr65248-2.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr65248-3.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr65248-4.c

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 22bc81f..01eb41d 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -51894,6 +51894,9 @@ ix86_initialize_bounds (tree var, tree lb, tree ub, tree *stmts)
 #if TARGET_MACHO
 #undef TARGET_BINDS_LOCAL_P
 #define TARGET_BINDS_LOCAL_P darwin_binds_local_p
+#else
+#undef TARGET_BINDS_LOCAL_P
+#define TARGET_BINDS_LOCAL_P default_binds_local_p_2
 #endif
 #if TARGET_DLLIMPORT_DECL_ATTRIBUTES
 #undef TARGET_BINDS_LOCAL_P
diff --git a/gcc/output.h b/gcc/output.h
index 217d979..53e47d0 100644
--- a/gcc/output.h
+++ b/gcc/output.h
@@ -586,6 +586,7 @@ extern void default_asm_output_anchor (rtx);
 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_local_p_2 (const_tree);
 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);
diff --git a/gcc/testsuite/gcc.target/i386/pr65248-1.c b/gcc/testsuite/gcc.target/i386/pr65248-1.c
new file mode 100644
index 0000000..735adde
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr65248-1.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Common symbol with -fpic.  */
+__attribute__((visibility("protected")))
+int xxx;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler-not "xxx\\(%rip\\)" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "xxx@GOTOFF" { target ia32 } } } */
+/* { dg-final { scan-assembler "xxx@GOT\\(" { target ia32 } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr65248-2.c b/gcc/testsuite/gcc.target/i386/pr65248-2.c
new file mode 100644
index 0000000..af264f3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr65248-2.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Weak common symbol with -fpic.  */
+__attribute__((weak, visibility("protected")))
+int xxx;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler-not "xxx\\(%rip\\)" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "xxx@GOTOFF" { target ia32 } } } */
+/* { dg-final { scan-assembler "xxx@GOT\\(" { target ia32 } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr65248-3.c b/gcc/testsuite/gcc.target/i386/pr65248-3.c
new file mode 100644
index 0000000..e7a05ea
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr65248-3.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Initialized symbol with -fpic.  */
+__attribute__((visibility("protected")))
+int xxx = -1;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler-not "xxx\\(%rip\\)" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "xxx@GOTOFF" { target ia32 } } } */
+/* { dg-final { scan-assembler "xxx@GOT\\(" { target ia32 } } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr65248-4.c b/gcc/testsuite/gcc.target/i386/pr65248-4.c
new file mode 100644
index 0000000..db818fc
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr65248-4.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* Weak initialized symbol with -fpic.  */
+__attribute__((weak, visibility("protected")))
+int xxx = -1;
+
+int
+foo ()
+{
+  return xxx;
+}
+
+/* { dg-final { scan-assembler-not "xxx\\(%rip\\)" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler "xxx@GOTPCREL" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-not "xxx@GOTOFF" { target ia32 } } } */
+/* { dg-final { scan-assembler "xxx@GOT\\(" { target ia32 } } } */
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 752dccf..537a64d 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6809,7 +6809,8 @@ resolution_local_p (enum ld_plugin_symbol_resolution resolution)
 }
 
 static bool
-default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
+default_binds_local_p_3 (const_tree exp, bool shlib, bool weak_dominate,
+			 bool extern_protected_data)
 {
   /* A non-decl is an entry in the constant pool.  */
   if (!DECL_P (exp))
@@ -6855,6 +6856,9 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
      or if we have a definition for the symbol.  We cannot infer visibility
      for undefined symbols.  */
   if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT
+      && (TREE_CODE (exp) == FUNCTION_DECL
+	  || !extern_protected_data
+	  || DECL_VISIBILITY (exp) != VISIBILITY_PROTECTED)
       && (DECL_VISIBILITY_SPECIFIED (exp) || defined_locally))
     return true;
 
@@ -6890,13 +6894,21 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
 bool
 default_binds_local_p (const_tree exp)
 {
-  return default_binds_local_p_2 (exp, flag_shlib != 0, true);
+  return default_binds_local_p_3 (exp, flag_shlib != 0, true, false);
+}
+
+/* Similar to default_binds_local_p, but protected data may be
+   external.  */
+bool
+default_binds_local_p_2 (const_tree exp)
+{
+  return default_binds_local_p_3 (exp, flag_shlib != 0, true, true);
 }
 
 bool
 default_binds_local_p_1 (const_tree exp, int shlib)
 {
-  return default_binds_local_p_2 (exp, shlib != 0, false);
+  return default_binds_local_p_3 (exp, shlib != 0, false, false);
 }
 
 /* Return true when references to DECL must bind to current definition in
-- 
2.1.0