[PATCH] Fix default_binds_local_p_2 for extern protected data

[PATCH] Fix default_binds_local_p_2 for extern protected data

[Szabolcs Nagy]

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

The commit
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=222184
changed a true to false in varasm.c:

 bool
 default_binds_local_p_2 (const_tree exp)
 {
-  return default_binds_local_p_3 (exp, flag_shlib != 0, true, true);
+  return default_binds_local_p_3 (exp, flag_shlib != 0, true, false,
+				  !flag_pic);
 }

where

 default_binds_local_p_3 (const_tree exp, bool shlib, bool weak_dominate,
-			 bool extern_protected_data)
+			 bool extern_protected_data, bool common_local_p)
 {

false means that extern protected data binds locally,
which is wrong if the target can have copy relocations
against it (then the address must be loaded from GOT
otherwise the main executable will see different address).

Currently S/390, ARM and AArch64 targets use this predicate
and the current default is wrong for all of them (they can
have copy relocs) so I changed the default instead of doing
it in a target specific way.

The equivalent x86_64 bug was
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248
the default was changed for
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65780
now i opened
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66912
for arm and aarch64.

Needs a further binutils patch too to emit R_*_GLOB_DAT
instead of R_*_RELATIVE relocs for protected data.
The glibc elf/tst-protected1a and elf/tst-protected1b
tests depend on this.

Tested ARM and AArch64 targets.

gcc/ChangeLog:

2015-07-22  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	PR target/66912
	* varasm.c (default_binds_local_p_2): Turn on extern_protected_data.

gcc/testsuite/ChangeLog:

2015-07-22  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	PR target/66912
	* gcc.target/aarch64/pr66912.c: New.
	* gcc.target/arm/pr66912.c: New.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-2.diff --]
[-- Type: text/x-patch; name=gcc-2.diff, Size: 2810 bytes --]

diff --git a/gcc/testsuite/gcc.target/aarch64/pr66912.c b/gcc/testsuite/gcc.target/aarch64/pr66912.c
new file mode 100644
index 0000000..b8aabcd
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr66912.c
@@ -0,0 +1,42 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+__attribute__((visibility("protected")))
+int n_common;
+
+__attribute__((weak, visibility("protected")))
+int n_weak_common;
+
+__attribute__((visibility("protected")))
+int n_init = -1;
+
+__attribute__((weak, visibility("protected")))
+int n_weak_init = -1;
+
+int
+f1 ()
+{
+  /* { dg-final { scan-assembler ":got(page_lo15)?:n_common" } } */
+  return n_common;
+}
+
+int
+f2 ()
+{
+  /* { dg-final { scan-assembler ":got(page_lo15)?:n_weak_common" } } */
+  return n_weak_common;
+}
+
+int
+f3 ()
+{
+  /* { dg-final { scan-assembler ":got(page_lo15)?:n_init" } } */
+  return n_init;
+}
+
+int
+f4 ()
+{
+  /* { dg-final { scan-assembler ":got(page_lo15)?:n_weak_init" } } */
+  return n_weak_init;
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr66912.c b/gcc/testsuite/gcc.target/arm/pr66912.c
new file mode 100644
index 0000000..27e4c45
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr66912.c
@@ -0,0 +1,42 @@
+/* { dg-do compile { target *-*-linux* } } */
+/* { dg-options "-O2 -fpic" } */
+
+__attribute__((visibility("protected")))
+int n_common;
+
+__attribute__((weak, visibility("protected")))
+int n_weak_common;
+
+__attribute__((visibility("protected")))
+int n_init = -1;
+
+__attribute__((weak, visibility("protected")))
+int n_weak_init = -1;
+
+int
+f1 ()
+{
+  /* { dg-final { scan-assembler "\\.word\\tn_common\\(GOT\\)" } } */
+  return n_common;
+}
+
+int
+f2 ()
+{
+  /* { dg-final { scan-assembler "\\.word\\tn_weak_common\\(GOT\\)" } } */
+  return n_weak_common;
+}
+
+int
+f3 ()
+{
+  /* { dg-final { scan-assembler "\\.word\\tn_init\\(GOT\\)" } } */
+  return n_init;
+}
+
+int
+f4 ()
+{
+  /* { dg-final { scan-assembler "\\.word\\tn_weak_init\\(GOT\\)" } } */
+  return n_weak_init;
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 6a4ba0b..a056792 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6907,12 +6907,13 @@ default_binds_local_p (const_tree exp)
   return default_binds_local_p_3 (exp, flag_shlib != 0, true, false, false);
 }
 
-/* Similar to default_binds_local_p, but common symbol may be local.  */
+/* Similar to default_binds_local_p, but common symbol may be local and
+   extern protected data is non-local.  */
 
 bool
 default_binds_local_p_2 (const_tree exp)
 {
-  return default_binds_local_p_3 (exp, flag_shlib != 0, true, false,
+  return default_binds_local_p_3 (exp, flag_shlib != 0, true, true,
 				  !flag_pic);
 }
 

Re: [PATCH] Fix default_binds_local_p_2 for extern protected data

[Szabolcs Nagy]

ping.

On 22/07/15 18:01, Szabolcs Nagy wrote:
> The commit
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=222184
> changed a true to false in varasm.c:
>
>   bool
>   default_binds_local_p_2 (const_tree exp)
>   {
> -  return default_binds_local_p_3 (exp, flag_shlib != 0, true, true);
> +  return default_binds_local_p_3 (exp, flag_shlib != 0, true, false,
> +				  !flag_pic);
>   }
>
> where
>
>   default_binds_local_p_3 (const_tree exp, bool shlib, bool weak_dominate,
> -			 bool extern_protected_data)
> +			 bool extern_protected_data, bool common_local_p)
>   {
>
> false means that extern protected data binds locally,
> which is wrong if the target can have copy relocations
> against it (then the address must be loaded from GOT
> otherwise the main executable will see different address).
>
> Currently S/390, ARM and AArch64 targets use this predicate
> and the current default is wrong for all of them (they can
> have copy relocs) so I changed the default instead of doing
> it in a target specific way.
>
> The equivalent x86_64 bug was
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248
> the default was changed for
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65780
> now i opened
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66912
> for arm and aarch64.
>
> Needs a further binutils patch too to emit R_*_GLOB_DAT
> instead of R_*_RELATIVE relocs for protected data.
> The glibc elf/tst-protected1a and elf/tst-protected1b
> tests depend on this.
>
> Tested ARM and AArch64 targets.
>
> gcc/ChangeLog:
>
> 2015-07-22  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>
> 	PR target/66912
> 	* varasm.c (default_binds_local_p_2): Turn on extern_protected_data.
>
> gcc/testsuite/ChangeLog:
>
> 2015-07-22  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>
> 	PR target/66912
> 	* gcc.target/aarch64/pr66912.c: New.
> 	* gcc.target/arm/pr66912.c: New.
>

Re: [PATCH] Fix default_binds_local_p_2 for extern protected data

[Szabolcs Nagy]

ping 2.

this patch is needed for working visibility ("protected")
attribute for extern data on targets using default_binds_local_p_2.
https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01871.html

On 10/08/15 12:04, Szabolcs Nagy wrote:
>
> ping.
>
> On 22/07/15 18:01, Szabolcs Nagy wrote:
>> The commit
>> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=222184
>> changed a true to false in varasm.c:
>>
>>    bool
>>    default_binds_local_p_2 (const_tree exp)
>>    {
>> -  return default_binds_local_p_3 (exp, flag_shlib != 0, true, true);
>> +  return default_binds_local_p_3 (exp, flag_shlib != 0, true, false,
>> +				  !flag_pic);
>>    }
>>
>> where
>>
>>    default_binds_local_p_3 (const_tree exp, bool shlib, bool weak_dominate,
>> -			 bool extern_protected_data)
>> +			 bool extern_protected_data, bool common_local_p)
>>    {
>>
>> false means that extern protected data binds locally,
>> which is wrong if the target can have copy relocations
>> against it (then the address must be loaded from GOT
>> otherwise the main executable will see different address).
>>
>> Currently S/390, ARM and AArch64 targets use this predicate
>> and the current default is wrong for all of them (they can
>> have copy relocs) so I changed the default instead of doing
>> it in a target specific way.
>>
>> The equivalent x86_64 bug was
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248
>> the default was changed for
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65780
>> now i opened
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66912
>> for arm and aarch64.
>>
>> Needs a further binutils patch too to emit R_*_GLOB_DAT
>> instead of R_*_RELATIVE relocs for protected data.
>> The glibc elf/tst-protected1a and elf/tst-protected1b
>> tests depend on this.
>>
>> Tested ARM and AArch64 targets.
>>
>> gcc/ChangeLog:
>>
>> 2015-07-22  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>>
>> 	PR target/66912
>> 	* varasm.c (default_binds_local_p_2): Turn on extern_protected_data.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2015-07-22  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>>
>> 	PR target/66912
>> 	* gcc.target/aarch64/pr66912.c: New.
>> 	* gcc.target/arm/pr66912.c: New.
>>
>

Re: [PATCH] Fix default_binds_local_p_2 for extern protected data

[Bernd Schmidt]

On 09/17/2015 11:15 AM, Szabolcs Nagy wrote:
> ping 2.
>
> this patch is needed for working visibility ("protected")
> attribute for extern data on targets using default_binds_local_p_2.
> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01871.html

I hesitate to review this one since I don't think I understand the 
issues on the various affected arches well enough. It looks like Jakub 
had some input on the earlier changes, maybe he could take a look? Or 
maybe rth knows best. Adding Ccs.

It would help to have examples of code generation demonstrating the 
problem and how you would solve it. Input from the s390 maintainers 
whether this is correct for their port would also be appreciated.

>>> Needs a further binutils patch too to emit R_*_GLOB_DAT
>>> instead of R_*_RELATIVE relocs for protected data.
>>> The glibc elf/tst-protected1a and elf/tst-protected1b
>>> tests depend on this.

What is the consequence of not having this binutils patch? Is the gcc 
patch and improvement, a null, or are there situations where it causes 
regressions without the binutils patch?

>>> Tested ARM and AArch64 targets.

Tested how, with or without this binutils patch?


Bernd

Re: [PATCH] Fix default_binds_local_p_2 for extern protected data

[Szabolcs Nagy]

On 30/09/15 14:47, Bernd Schmidt wrote:
> On 09/17/2015 11:15 AM, Szabolcs Nagy wrote:
>> ping 2.
>>
>> this patch is needed for working visibility ("protected")
>> attribute for extern data on targets using default_binds_local_p_2.
>> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01871.html
>
> I hesitate to review this one since I don't think I understand the
> issues on the various affected arches well enough. It looks like Jakub
> had some input on the earlier changes, maybe he could take a look? Or
> maybe rth knows best. Adding Ccs.
>
> It would help to have examples of code generation demonstrating the
> problem and how you would solve it. Input from the s390 maintainers
> whether this is correct for their port would also be appreciated.
>

consider the TU

   __attribute__((visibility("protected"))) int n;

   int f () { return n; }

if n "binds_local" then gcc -O -fpic -S is like

         .text
         .align  2
         .global f
         .arch armv8-a+fp+simd
         .type   f, %function
f:
         adrp    x0, n
         ldr     w0, [x0, #:lo12:n]
         ret
         .size   f, .-f
         .protected      n
         .comm   n,4,4

so 'n' is a direct reference, not accessed through
the GOT ('n' will be in the .bss of the dso).
this is the current behavior.

if i remove the protected visibility attribute
then the access goes through GOT:

         .text
         .align  2
         .global f
         .arch armv8-a+fp+simd
         .type   f, %function
f:
         adrp    x0, _GLOBAL_OFFSET_TABLE_
         ldr     x0, [x0, #:gotpage_lo15:n]
         ldr     w0, [x0]
         ret
         .size   f, .-f
         .comm   n,4,4

protected visibility means the definition cannot
be overridden by another module, but it should
still allow extern references.

if the main module references such an object then
(as an implementation detail) it may use copy
relocation against it, which places 'n' in the
main module and the dynamic linker should make
sure that references to 'n' point there.

this is only possible if references to 'n' go
through the GOT (i.e. it should not be "binds_local").

this got fixed on x86 (there are explanations in
pr65248, pr55012), but the default_binds_local_p
logic is not fixed.

>>>> Needs a further binutils patch too to emit R_*_GLOB_DAT
>>>> instead of R_*_RELATIVE relocs for protected data.
>>>> The glibc elf/tst-protected1a and elf/tst-protected1b
>>>> tests depend on this.
>
> What is the consequence of not having this binutils patch? Is the gcc
> patch and improvement, a null, or are there situations where it causes
> regressions without the binutils patch?
>

does not cause regressions.

binutils also assumed that extern protected data is
local so it did not emit the R_*_GLOB_DAT relocations
for it which means there is no symbolic GOT entry that
the dynamic-linker can update, so the behavior is the
same as before.

glibc dynamic linker also failed to handle this case
with similar effect.

so for correct behavior new gcc, new binutils and new
glibc are needed, if any one is old that results the old
behavior.

binutils trunk and glibc-2.22 have the fix for aarch64
and arm (and x86 and various other arches).

>>>> Tested ARM and AArch64 targets.
>
> Tested how, with or without this binutils patch?
>

with old and new binutils as well.

Re: [PATCH] Fix default_binds_local_p_2 for extern protected data

[Andreas Krebbel]

On 09/30/2015 06:21 PM, Szabolcs Nagy wrote:
> On 30/09/15 14:47, Bernd Schmidt wrote:
>> On 09/17/2015 11:15 AM, Szabolcs Nagy wrote:
>>> ping 2.
>>>
>>> this patch is needed for working visibility ("protected")
>>> attribute for extern data on targets using default_binds_local_p_2.
>>> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01871.html
>>
>> I hesitate to review this one since I don't think I understand the
>> issues on the various affected arches well enough. It looks like Jakub
>> had some input on the earlier changes, maybe he could take a look? Or
>> maybe rth knows best. Adding Ccs.
>>
>> It would help to have examples of code generation demonstrating the
>> problem and how you would solve it. Input from the s390 maintainers
>> whether this is correct for their port would also be appreciated.

We are having the same problem on S/390. I think the GCC change is correct for S/390 as well.

-Andreas-

>>
> 
> consider the TU
> 
>    __attribute__((visibility("protected"))) int n;
> 
>    int f () { return n; }
> 
> if n "binds_local" then gcc -O -fpic -S is like
> 
>          .text
>          .align  2
>          .global f
>          .arch armv8-a+fp+simd
>          .type   f, %function
> f:
>          adrp    x0, n
>          ldr     w0, [x0, #:lo12:n]
>          ret
>          .size   f, .-f
>          .protected      n
>          .comm   n,4,4
> 
> so 'n' is a direct reference, not accessed through
> the GOT ('n' will be in the .bss of the dso).
> this is the current behavior.
> 
> if i remove the protected visibility attribute
> then the access goes through GOT:
> 
>          .text
>          .align  2
>          .global f
>          .arch armv8-a+fp+simd
>          .type   f, %function
> f:
>          adrp    x0, _GLOBAL_OFFSET_TABLE_
>          ldr     x0, [x0, #:gotpage_lo15:n]
>          ldr     w0, [x0]
>          ret
>          .size   f, .-f
>          .comm   n,4,4
> 
> protected visibility means the definition cannot
> be overridden by another module, but it should
> still allow extern references.
> 
> if the main module references such an object then
> (as an implementation detail) it may use copy
> relocation against it, which places 'n' in the
> main module and the dynamic linker should make
> sure that references to 'n' point there.
> 
> this is only possible if references to 'n' go
> through the GOT (i.e. it should not be "binds_local").

Re: [PATCH] Fix default_binds_local_p_2 for extern protected data

[Szabolcs Nagy]

On 30/09/15 20:23, Andreas Krebbel wrote:
> On 09/30/2015 06:21 PM, Szabolcs Nagy wrote:
>> On 30/09/15 14:47, Bernd Schmidt wrote:
>>> On 09/17/2015 11:15 AM, Szabolcs Nagy wrote:
>>>> ping 2.
>>>>
>>>> this patch is needed for working visibility ("protected")
>>>> attribute for extern data on targets using default_binds_local_p_2.
>>>> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01871.html
>>>
>>> I hesitate to review this one since I don't think I understand the
>>> issues on the various affected arches well enough. It looks like Jakub
>>> had some input on the earlier changes, maybe he could take a look? Or
>>> maybe rth knows best. Adding Ccs.
>>>
>>> It would help to have examples of code generation demonstrating the
>>> problem and how you would solve it. Input from the s390 maintainers
>>> whether this is correct for their port would also be appreciated.
>
> We are having the same problem on S/390. I think the GCC change is correct for S/390 as well.
>
> -Andreas-
>

i think the approvals of arm and aarch64 maintainers
are needed to apply this fix for pr target/66912.

(only s390, arm and aarch64 use this predicate.)

>>>
>>
>> consider the TU
>>
>>     __attribute__((visibility("protected"))) int n;
>>
>>     int f () { return n; }
>>
>> if n "binds_local" then gcc -O -fpic -S is like
>>
>>           .text
>>           .align  2
>>           .global f
>>           .arch armv8-a+fp+simd
>>           .type   f, %function
>> f:
>>           adrp    x0, n
>>           ldr     w0, [x0, #:lo12:n]
>>           ret
>>           .size   f, .-f
>>           .protected      n
>>           .comm   n,4,4
>>
>> so 'n' is a direct reference, not accessed through
>> the GOT ('n' will be in the .bss of the dso).
>> this is the current behavior.
>>
>> if i remove the protected visibility attribute
>> then the access goes through GOT:
>>
>>           .text
>>           .align  2
>>           .global f
>>           .arch armv8-a+fp+simd
>>           .type   f, %function
>> f:
>>           adrp    x0, _GLOBAL_OFFSET_TABLE_
>>           ldr     x0, [x0, #:gotpage_lo15:n]
>>           ldr     w0, [x0]
>>           ret
>>           .size   f, .-f
>>           .comm   n,4,4
>>
>> protected visibility means the definition cannot
>> be overridden by another module, but it should
>> still allow extern references.
>>
>> if the main module references such an object then
>> (as an implementation detail) it may use copy
>> relocation against it, which places 'n' in the
>> main module and the dynamic linker should make
>> sure that references to 'n' point there.
>>
>> this is only possible if references to 'n' go
>> through the GOT (i.e. it should not be "binds_local").
>
>
>
>
>

Re: [PATCH] Fix default_binds_local_p_2 for extern protected data

[Szabolcs Nagy]

On 14/10/15 10:55, Szabolcs Nagy wrote:
> On 30/09/15 20:23, Andreas Krebbel wrote:
>> On 09/30/2015 06:21 PM, Szabolcs Nagy wrote:
>>> On 30/09/15 14:47, Bernd Schmidt wrote:
>>>> On 09/17/2015 11:15 AM, Szabolcs Nagy wrote:
>>>>> ping 2.
>>>>>
>>>>> this patch is needed for working visibility ("protected")
>>>>> attribute for extern data on targets using default_binds_local_p_2.
>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01871.html
>>>>
>>>> I hesitate to review this one since I don't think I understand the
>>>> issues on the various affected arches well enough. It looks like Jakub
>>>> had some input on the earlier changes, maybe he could take a look? Or
>>>> maybe rth knows best. Adding Ccs.
>>>>
>>>> It would help to have examples of code generation demonstrating the
>>>> problem and how you would solve it. Input from the s390 maintainers
>>>> whether this is correct for their port would also be appreciated.
>>
>> We are having the same problem on S/390. I think the GCC change is correct for S/390 as well.
>>
>> -Andreas-
>>
>
> i think the approvals of arm and aarch64 maintainers
> are needed to apply this fix for pr target/66912.
>
> (only s390, arm and aarch64 use this predicate.)
>

i was told this needs global maintainer approval.
adding jakub and rth back to cc.

>>>>
>>>
>>> consider the TU
>>>
>>>     __attribute__((visibility("protected"))) int n;
>>>
>>>     int f () { return n; }
>>>
>>> if n "binds_local" then gcc -O -fpic -S is like
>>>
>>>           .text
>>>           .align  2
>>>           .global f
>>>           .arch armv8-a+fp+simd
>>>           .type   f, %function
>>> f:
>>>           adrp    x0, n
>>>           ldr     w0, [x0, #:lo12:n]
>>>           ret
>>>           .size   f, .-f
>>>           .protected      n
>>>           .comm   n,4,4
>>>
>>> so 'n' is a direct reference, not accessed through
>>> the GOT ('n' will be in the .bss of the dso).
>>> this is the current behavior.
>>>
>>> if i remove the protected visibility attribute
>>> then the access goes through GOT:
>>>
>>>           .text
>>>           .align  2
>>>           .global f
>>>           .arch armv8-a+fp+simd
>>>           .type   f, %function
>>> f:
>>>           adrp    x0, _GLOBAL_OFFSET_TABLE_
>>>           ldr     x0, [x0, #:gotpage_lo15:n]
>>>           ldr     w0, [x0]
>>>           ret
>>>           .size   f, .-f
>>>           .comm   n,4,4
>>>
>>> protected visibility means the definition cannot
>>> be overridden by another module, but it should
>>> still allow extern references.
>>>
>>> if the main module references such an object then
>>> (as an implementation detail) it may use copy
>>> relocation against it, which places 'n' in the
>>> main module and the dynamic linker should make
>>> sure that references to 'n' point there.
>>>
>>> this is only possible if references to 'n' go
>>> through the GOT (i.e. it should not be "binds_local").
>>
>>
>>
>>
>>
>

Re: [PATCH] Fix default_binds_local_p_2 for extern protected data

[Richard Henderson]

On 07/22/2015 07:01 AM, Szabolcs Nagy wrote:
> 2015-07-22  Szabolcs Nagy<szabolcs.nagy@arm.com>
>
> 	PR target/66912
> 	* varasm.c (default_binds_local_p_2): Turn on extern_protected_data.
>
> gcc/testsuite/ChangeLog:
>
> 2015-07-22  Szabolcs Nagy<szabolcs.nagy@arm.com>
>
> 	PR target/66912
> 	* gcc.target/aarch64/pr66912.c: New.
> 	* gcc.target/arm/pr66912.c: New.

Ok.


r~