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

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

[Uros Bizjak]

Hello!

> 2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
>             Richard Henderson  <rth@redhat.com>
>
>         PR rtl/32219
>         * cgraphunit.c (cgraph_node::finalize_function): Set definition
>         before notice_global_symbol.
>         (varpool_node::finalize_decl): Likewise.
>         * varasm.c (default_binds_local_p_2): Rename from
>         default_binds_local_p_1, add weak_dominate argument.  Use direct
>         returns instead of assigning to local variable.  Unify varpool and
>         cgraph paths via symtab_node.  Reject undef weak variables before
>         testing visibility.  Reorder tests for simplicity.
>         (default_binds_local_p): Use default_binds_local_p_2.
>         (default_binds_local_p_1): Likewise.
>         (decl_binds_to_current_def_p): Unify varpool and cgraph paths
>         via symtab_node.
>         (default_elf_asm_output_external): Emit visibility when specified.

It looks like this patch broke alphaev68-linux-gnu [1]. There are many
failures of the type:

/tmp/cck7V7MR.o: In function
`__static_initialization_and_destruction_0(int, int)':^M
(.text+0x3ac): relocation truncated to fit: GPRELHIGH against symbol
`std::__cxx11::basic_string<char, std::char_traits<char>,
std::allocator<char> >::~basic_string()@@GLIBCXX_3.4.21' defined in
.text section in
/space/uros/gcc-build/alphaev68-unknown-linux-gnu/./libstdc++-v3/src/.libs/libstdc++.so^M
/space/homedirs/uros/local/bin/ld: /tmp/cck7V7MR.o: gp-relative
relocation against dynamic symbol
_ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEED1Ev@@GLIBCXX_3.4.21^M
/space/homedirs/uros/local/bin/ld: final link failed: Nonrepresentable
section on output^M

An example is g++.dg/torture/pr60750.C :

/space/uros/gcc-build/gcc/testsuite/g++/../../xg++
-B/space/uros/gcc-build/gcc/testsuite/g++/../../
/space/homedirs/uros/gcc-svn/trunk/gcc/testsuite/g++.dg/torture/pr60750.C
-fno-diagnostics-show-caret -fdiagnostics-color=never -nostdinc++
-I/space/homedirs/uros/gcc-build/alphaev68-unknown-linux-gnu/libstdc++-v3/include/alphaev68-unknown-linux-gnu
-I/space/homedirs/uros/gcc-build/alphaev68-unknown-linux-gnu/libstdc++-v3/include
-I/space/homedirs/uros/gcc-svn/trunk/libstdc++-v3/libsupc++
-I/space/homedirs/uros/gcc-svn/trunk/libstdc++-v3/include/backward
-I/space/homedirs/uros/gcc-svn/trunk/libstdc++-v3/testsuite/util
-fmessage-length=0 -O0 -std=c++11
-L/space/uros/gcc-build/alphaev68-unknown-linux-gnu/./libstdc++-v3/src/.libs
-B/space/uros/gcc-build/alphaev68-unknown-linux-gnu/./libstdc++-v3/src/.libs
-L/space/uros/gcc-build/alphaev68-unknown-linux-gnu/./libstdc++-v3/src/.libs
-lm -o ./pr60750.exe^M
/space/homedirs/uros/local/bin/ld: /tmp/cck7V7MR.o: gp-relative
relocation against dynamic symbol
_ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEED1Ev@@GLIBCXX_3.4.21^M
/tmp/cck7V7MR.o: In function
`__static_initialization_and_destruction_0(int, int)':^M
(.text+0x3ac): relocation truncated to fit: GPRELHIGH against symbol
`std::__cxx11::basic_string<char, std::char_traits<char>,
std::allocator<char> >::~basic_string()@@GLIBCXX_3.4.21' defined in
.text section in
/space/uros/gcc-build/alphaev68-unknown-linux-gnu/./libstdc++-v3/src/.libs/libstdc++.so^M
/space/homedirs/uros/local/bin/ld: /tmp/cck7V7MR.o: gp-relative
relocation against dynamic symbol
_ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEED1Ev@@GLIBCXX_3.4.21^M
/space/homedirs/uros/local/bin/ld: final link failed: Nonrepresentable
section on output^M
collect2: error: ld returned 1 exit status^M
compiler exited with status 1

[1] https://gcc.gnu.org/ml/gcc-testresults/2015-02/msg01867.html

Uros.

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

[H.J. Lu]

On Mon, Feb 16, 2015 at 5:25 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Hello!
>
>> 2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
>>             Richard Henderson  <rth@redhat.com>
>>
>>         PR rtl/32219
>>         * cgraphunit.c (cgraph_node::finalize_function): Set definition
>>         before notice_global_symbol.
>>         (varpool_node::finalize_decl): Likewise.
>>         * varasm.c (default_binds_local_p_2): Rename from
>>         default_binds_local_p_1, add weak_dominate argument.  Use direct
>>         returns instead of assigning to local variable.  Unify varpool and
>>         cgraph paths via symtab_node.  Reject undef weak variables before
>>         testing visibility.  Reorder tests for simplicity.
>>         (default_binds_local_p): Use default_binds_local_p_2.
>>         (default_binds_local_p_1): Likewise.
>>         (decl_binds_to_current_def_p): Unify varpool and cgraph paths
>>         via symtab_node.
>>         (default_elf_asm_output_external): Emit visibility when specified.
>
> It looks like this patch broke alphaev68-linux-gnu [1]. There are many
> failures of the type:
>
> /tmp/cck7V7MR.o: In function
> `__static_initialization_and_destruction_0(int, int)':^M
> (.text+0x3ac): relocation truncated to fit: GPRELHIGH against symbol
> `std::__cxx11::basic_string<char, std::char_traits<char>,
> std::allocator<char> >::~basic_string()@@GLIBCXX_3.4.21' defined in
> .text section in
> /space/uros/gcc-build/alphaev68-unknown-linux-gnu/./libstdc++-v3/src/.libs/libstdc++.so^M
> /space/homedirs/uros/local/bin/ld: /tmp/cck7V7MR.o: gp-relative
> relocation against dynamic symbol

It could be related to:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65064

Before this bug fix, all common symbols don't bind locally,
which is one of PR 32219 bugs.  After this fix, common
symbols bind locally.  It may cause problems on targets with
small data sections and common symbols aren't in small
data section:

1. Since common symbols bind locally, backend may assume
they are in small data section and lead to link-time failure.
2. Backend assume common symbols are never in small
data section.  But a definition in small data section may
override a common symbol, which still binds lcoally, and
turn a reference to common symbol to reference to small
data section.  This also may lead to link-time failure.

Those targets can't assume common symbols are in
small data section since it may change at link-time.

The most conservative solution is to make common
symbol doesn't bind locally for targets which defines
TARGET_IN_SMALL_DATA_P.

-- 
H.J.

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

[Richard Henderson]

On 02/16/2015 06:01 AM, H.J. Lu wrote:
> On Mon, Feb 16, 2015 at 5:25 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> Hello!
>>
>>> 2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
>>>             Richard Henderson  <rth@redhat.com>
>>>
>>>         PR rtl/32219
>>>         * cgraphunit.c (cgraph_node::finalize_function): Set definition
>>>         before notice_global_symbol.
>>>         (varpool_node::finalize_decl): Likewise.
>>>         * varasm.c (default_binds_local_p_2): Rename from
>>>         default_binds_local_p_1, add weak_dominate argument.  Use direct
>>>         returns instead of assigning to local variable.  Unify varpool and
>>>         cgraph paths via symtab_node.  Reject undef weak variables before
>>>         testing visibility.  Reorder tests for simplicity.
>>>         (default_binds_local_p): Use default_binds_local_p_2.
>>>         (default_binds_local_p_1): Likewise.
>>>         (decl_binds_to_current_def_p): Unify varpool and cgraph paths
>>>         via symtab_node.
>>>         (default_elf_asm_output_external): Emit visibility when specified.
>>
>> It looks like this patch broke alphaev68-linux-gnu [1]. There are many
>> failures of the type:
>>
>> /tmp/cck7V7MR.o: In function
>> `__static_initialization_and_destruction_0(int, int)':^M
>> (.text+0x3ac): relocation truncated to fit: GPRELHIGH against symbol
>> `std::__cxx11::basic_string<char, std::char_traits<char>,
>> std::allocator<char> >::~basic_string()@@GLIBCXX_3.4.21' defined in
>> .text section in
>> /space/uros/gcc-build/alphaev68-unknown-linux-gnu/./libstdc++-v3/src/.libs/libstdc++.so^M
>> /space/homedirs/uros/local/bin/ld: /tmp/cck7V7MR.o: gp-relative
>> relocation against dynamic symbol
> 
> It could be related to:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65064
> 
> Before this bug fix, all common symbols don't bind locally,
> which is one of PR 32219 bugs.  After this fix, common
> symbols bind locally.  It may cause problems on targets with
> small data sections and common symbols aren't in small
> data section:

This is a destructor, and so obviously not a common symbol.

I'll have a look.


r~

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

[H.J. Lu]

On Mon, Feb 16, 2015 at 8:30 AM, Richard Henderson <rth@redhat.com> wrote:
> On 02/16/2015 06:01 AM, H.J. Lu wrote:
>> On Mon, Feb 16, 2015 at 5:25 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>> Hello!
>>>
>>>> 2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
>>>>             Richard Henderson  <rth@redhat.com>
>>>>
>>>>         PR rtl/32219
>>>>         * cgraphunit.c (cgraph_node::finalize_function): Set definition
>>>>         before notice_global_symbol.
>>>>         (varpool_node::finalize_decl): Likewise.
>>>>         * varasm.c (default_binds_local_p_2): Rename from
>>>>         default_binds_local_p_1, add weak_dominate argument.  Use direct
>>>>         returns instead of assigning to local variable.  Unify varpool and
>>>>         cgraph paths via symtab_node.  Reject undef weak variables before
>>>>         testing visibility.  Reorder tests for simplicity.
>>>>         (default_binds_local_p): Use default_binds_local_p_2.
>>>>         (default_binds_local_p_1): Likewise.
>>>>         (decl_binds_to_current_def_p): Unify varpool and cgraph paths
>>>>         via symtab_node.
>>>>         (default_elf_asm_output_external): Emit visibility when specified.
>>>
>>> It looks like this patch broke alphaev68-linux-gnu [1]. There are many
>>> failures of the type:
>>>
>>> /tmp/cck7V7MR.o: In function
>>> `__static_initialization_and_destruction_0(int, int)':^M
>>> (.text+0x3ac): relocation truncated to fit: GPRELHIGH against symbol
>>> `std::__cxx11::basic_string<char, std::char_traits<char>,
>>> std::allocator<char> >::~basic_string()@@GLIBCXX_3.4.21' defined in
>>> .text section in
>>> /space/uros/gcc-build/alphaev68-unknown-linux-gnu/./libstdc++-v3/src/.libs/libstdc++.so^M
>>> /space/homedirs/uros/local/bin/ld: /tmp/cck7V7MR.o: gp-relative
>>> relocation against dynamic symbol
>>
>> It could be related to:
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65064
>>
>> Before this bug fix, all common symbols don't bind locally,
>> which is one of PR 32219 bugs.  After this fix, common
>> symbols bind locally.  It may cause problems on targets with
>> small data sections and common symbols aren't in small
>> data section:
>
> This is a destructor, and so obviously not a common symbol.

Then it could be:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65074


-- 
H.J.

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

[Uros Bizjak]

On Mon, Feb 16, 2015 at 5:30 PM, Richard Henderson <rth@redhat.com> wrote:

>>>> 2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
>>>>             Richard Henderson  <rth@redhat.com>
>>>>
>>>>         PR rtl/32219
>>>>         * cgraphunit.c (cgraph_node::finalize_function): Set definition
>>>>         before notice_global_symbol.
>>>>         (varpool_node::finalize_decl): Likewise.
>>>>         * varasm.c (default_binds_local_p_2): Rename from
>>>>         default_binds_local_p_1, add weak_dominate argument.  Use direct
>>>>         returns instead of assigning to local variable.  Unify varpool and
>>>>         cgraph paths via symtab_node.  Reject undef weak variables before
>>>>         testing visibility.  Reorder tests for simplicity.
>>>>         (default_binds_local_p): Use default_binds_local_p_2.
>>>>         (default_binds_local_p_1): Likewise.
>>>>         (decl_binds_to_current_def_p): Unify varpool and cgraph paths
>>>>         via symtab_node.
>>>>         (default_elf_asm_output_external): Emit visibility when specified.
>>>
>>> It looks like this patch broke alphaev68-linux-gnu [1]. There are many
>>> failures of the type:
>>>
>>> /tmp/cck7V7MR.o: In function
>>> `__static_initialization_and_destruction_0(int, int)':^M
>>> (.text+0x3ac): relocation truncated to fit: GPRELHIGH against symbol
>>> `std::__cxx11::basic_string<char, std::char_traits<char>,
>>> std::allocator<char> >::~basic_string()@@GLIBCXX_3.4.21' defined in
>>> .text section in
>>> /space/uros/gcc-build/alphaev68-unknown-linux-gnu/./libstdc++-v3/src/.libs/libstdc++.so^M
>>> /space/homedirs/uros/local/bin/ld: /tmp/cck7V7MR.o: gp-relative
>>> relocation against dynamic symbol
>>
>> It could be related to:
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65064
>>
>> Before this bug fix, all common symbols don't bind locally,
>> which is one of PR 32219 bugs.  After this fix, common
>> symbols bind locally.  It may cause problems on targets with
>> small data sections and common symbols aren't in small
>> data section:
>
> This is a destructor, and so obviously not a common symbol.
>
> I'll have a look.

The attached patch fixes all alpha-linux-gnu failures.

2015-02-19  Uros Bizjak  <ubizjak@gmail.com>

    * config/alpha/alpha.c (alpha_in_small_data_p): Reject common symbols.

Patch was bootstrapped and regression tested on alphaev68-linux-gnu.

OK for mainline?

Uros.

Index: config/alpha/alpha.c
===================================================================
--- config/alpha/alpha.c        (revision 220806)
+++ config/alpha/alpha.c        (working copy)
@@ -835,6 +835,10 @@ alpha_in_small_data_p (const_tree exp)
   if (TREE_CODE (exp) == FUNCTION_DECL)
     return false;

+  /* COMMON symbols are never small data.  */
+  if (TREE_CODE (exp) == VAR_DECL && DECL_COMMON (exp))
+    return false;
+
   if (TREE_CODE (exp) == VAR_DECL && DECL_SECTION_NAME (exp))
     {
       const char *section = DECL_SECTION_NAME (exp);

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

[H.J. Lu]

On Thu, Feb 19, 2015 at 1:04 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Mon, Feb 16, 2015 at 5:30 PM, Richard Henderson <rth@redhat.com> wrote:
>
>>>>> 2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
>>>>>             Richard Henderson  <rth@redhat.com>
>>>>>
>>>>>         PR rtl/32219
>>>>>         * cgraphunit.c (cgraph_node::finalize_function): Set definition
>>>>>         before notice_global_symbol.
>>>>>         (varpool_node::finalize_decl): Likewise.
>>>>>         * varasm.c (default_binds_local_p_2): Rename from
>>>>>         default_binds_local_p_1, add weak_dominate argument.  Use direct
>>>>>         returns instead of assigning to local variable.  Unify varpool and
>>>>>         cgraph paths via symtab_node.  Reject undef weak variables before
>>>>>         testing visibility.  Reorder tests for simplicity.
>>>>>         (default_binds_local_p): Use default_binds_local_p_2.
>>>>>         (default_binds_local_p_1): Likewise.
>>>>>         (decl_binds_to_current_def_p): Unify varpool and cgraph paths
>>>>>         via symtab_node.
>>>>>         (default_elf_asm_output_external): Emit visibility when specified.
>>>>
>>>> It looks like this patch broke alphaev68-linux-gnu [1]. There are many
>>>> failures of the type:
>>>>
>>>> /tmp/cck7V7MR.o: In function
>>>> `__static_initialization_and_destruction_0(int, int)':^M
>>>> (.text+0x3ac): relocation truncated to fit: GPRELHIGH against symbol
>>>> `std::__cxx11::basic_string<char, std::char_traits<char>,
>>>> std::allocator<char> >::~basic_string()@@GLIBCXX_3.4.21' defined in
>>>> .text section in
>>>> /space/uros/gcc-build/alphaev68-unknown-linux-gnu/./libstdc++-v3/src/.libs/libstdc++.so^M
>>>> /space/homedirs/uros/local/bin/ld: /tmp/cck7V7MR.o: gp-relative
>>>> relocation against dynamic symbol
>>>
>>> It could be related to:
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65064
>>>
>>> Before this bug fix, all common symbols don't bind locally,
>>> which is one of PR 32219 bugs.  After this fix, common
>>> symbols bind locally.  It may cause problems on targets with
>>> small data sections and common symbols aren't in small
>>> data section:
>>
>> This is a destructor, and so obviously not a common symbol.
>>
>> I'll have a look.
>
> The attached patch fixes all alpha-linux-gnu failures.
>
> 2015-02-19  Uros Bizjak  <ubizjak@gmail.com>
>
>     * config/alpha/alpha.c (alpha_in_small_data_p): Reject common symbols.
>
> Patch was bootstrapped and regression tested on alphaev68-linux-gnu.
>
> OK for mainline?
>

You may want to use something like this:

https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01105.html


-- 
H.J.

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

[Richard Henderson]

On 02/19/2015 01:07 PM, H.J. Lu wrote:
>> > The attached patch fixes all alpha-linux-gnu failures.
>> >
>> > 2015-02-19  Uros Bizjak  <ubizjak@gmail.com>
>> >
>> >     * config/alpha/alpha.c (alpha_in_small_data_p): Reject common symbols.
>> >
>> > Patch was bootstrapped and regression tested on alphaev68-linux-gnu.
>> >
>> > OK for mainline?
>> >
> You may want to use something like this:
> 
> https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01105.html

That's exactly what he's done.


r~

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

[H.J. Lu]

On Thu, Feb 19, 2015 at 1:07 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Feb 19, 2015 at 1:04 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Mon, Feb 16, 2015 at 5:30 PM, Richard Henderson <rth@redhat.com> wrote:
>>
>>>>>> 2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
>>>>>>             Richard Henderson  <rth@redhat.com>
>>>>>>
>>>>>>         PR rtl/32219
>>>>>>         * cgraphunit.c (cgraph_node::finalize_function): Set definition
>>>>>>         before notice_global_symbol.
>>>>>>         (varpool_node::finalize_decl): Likewise.
>>>>>>         * varasm.c (default_binds_local_p_2): Rename from
>>>>>>         default_binds_local_p_1, add weak_dominate argument.  Use direct
>>>>>>         returns instead of assigning to local variable.  Unify varpool and
>>>>>>         cgraph paths via symtab_node.  Reject undef weak variables before
>>>>>>         testing visibility.  Reorder tests for simplicity.
>>>>>>         (default_binds_local_p): Use default_binds_local_p_2.
>>>>>>         (default_binds_local_p_1): Likewise.
>>>>>>         (decl_binds_to_current_def_p): Unify varpool and cgraph paths
>>>>>>         via symtab_node.
>>>>>>         (default_elf_asm_output_external): Emit visibility when specified.
>>>>>
>>>>> It looks like this patch broke alphaev68-linux-gnu [1]. There are many
>>>>> failures of the type:
>>>>>
>>>>> /tmp/cck7V7MR.o: In function
>>>>> `__static_initialization_and_destruction_0(int, int)':^M
>>>>> (.text+0x3ac): relocation truncated to fit: GPRELHIGH against symbol
>>>>> `std::__cxx11::basic_string<char, std::char_traits<char>,
>>>>> std::allocator<char> >::~basic_string()@@GLIBCXX_3.4.21' defined in
>>>>> .text section in
>>>>> /space/uros/gcc-build/alphaev68-unknown-linux-gnu/./libstdc++-v3/src/.libs/libstdc++.so^M
>>>>> /space/homedirs/uros/local/bin/ld: /tmp/cck7V7MR.o: gp-relative
>>>>> relocation against dynamic symbol
>>>>
>>>> It could be related to:
>>>>
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65064
>>>>
>>>> Before this bug fix, all common symbols don't bind locally,
>>>> which is one of PR 32219 bugs.  After this fix, common
>>>> symbols bind locally.  It may cause problems on targets with
>>>> small data sections and common symbols aren't in small
>>>> data section:
>>>
>>> This is a destructor, and so obviously not a common symbol.
>>>
>>> I'll have a look.
>>
>> The attached patch fixes all alpha-linux-gnu failures.
>>
>> 2015-02-19  Uros Bizjak  <ubizjak@gmail.com>
>>
>>     * config/alpha/alpha.c (alpha_in_small_data_p): Reject common symbols.
>>
>> Patch was bootstrapped and regression tested on alphaev68-linux-gnu.
>>
>> OK for mainline?
>>
>
> You may want to use something like this:
>
> https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01105.html
>

See:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65064

for why.

-- 
H.J.

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

[Richard Henderson]

On 02/19/2015 01:04 PM, Uros Bizjak wrote:
> 2015-02-19  Uros Bizjak  <ubizjak@gmail.com>
> 
>     * config/alpha/alpha.c (alpha_in_small_data_p): Reject common symbols.

Ok.  Thanks.


r~

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

[H.J. Lu]

On Thu, Feb 19, 2015 at 1:16 PM, Richard Henderson <rth@redhat.com> wrote:
> On 02/19/2015 01:04 PM, Uros Bizjak wrote:
>> 2015-02-19  Uros Bizjak  <ubizjak@gmail.com>
>>
>>     * config/alpha/alpha.c (alpha_in_small_data_p): Reject common symbols.
>
> Ok.  Thanks.

We tried it on ia64 and failed with Ada build.

-- 
H.J.

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

[Uros Bizjak]

On Thu, Feb 19, 2015 at 10:35 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Feb 19, 2015 at 1:16 PM, Richard Henderson <rth@redhat.com> wrote:
>> On 02/19/2015 01:04 PM, Uros Bizjak wrote:
>>> 2015-02-19  Uros Bizjak  <ubizjak@gmail.com>
>>>
>>>     * config/alpha/alpha.c (alpha_in_small_data_p): Reject common symbols.
>>
>> Ok.  Thanks.
>
> We tried it on ia64 and failed with Ada build.

As I see from the PR, it was the pilot error, please see comment #14 [1].

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65064#c14

Uros.

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

[Alex Velenko]

On 05/03/15 15:28, Ramana Radhakrishnan wrote:
>>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>>> index 7bf5b4d..777230e 100644
>>>> --- a/gcc/config/arm/arm.c
>>>> +++ b/gcc/config/arm/arm.c
>>>> @@ -6392,14 +6392,8 @@ arm_set_default_type_attributes (tree type)
>>>>    static bool
>>>>    arm_function_in_section_p (tree decl, section *section)
>>>>    {
>>>> -  /* We can only be certain about functions defined in the same
>>>> -     compilation unit.  */
>>>> -  if (!TREE_STATIC (decl))
>>>> -    return false;
>>>> -
>>>> -  /* Make sure that SYMBOL always binds to the definition in this
>>>> -     compilation unit.  */
>>>> -  if (!targetm.binds_local_p (decl))
>>>> +  /* We can only be certain about the prevailing symbol definition.  */
>>>> +  if (!decl_binds_to_current_def_p (decl))
>>>>        return false;
>>>>
>>>>      /* If DECL_SECTION_NAME is set, assume it is trustworthy. */
>>>>
>>>>
>
> Sorry to have missed this - I've also been traveling recently which has
> made it harder with patch traffic - this is OK if no regressions.
>
> Please apply with an appropriate Changelog.
>
> regressions
> Ramana
>
>
Hi,
Committed as r221220 and fixed ChangeLog entry in r221234.
Sorry for claiming the patch for myself.
Kind regards,
Alex

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

[Ramana Radhakrishnan]

>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>> index 7bf5b4d..777230e 100644
>>> --- a/gcc/config/arm/arm.c
>>> +++ b/gcc/config/arm/arm.c
>>> @@ -6392,14 +6392,8 @@ arm_set_default_type_attributes (tree type)
>>>   static bool
>>>   arm_function_in_section_p (tree decl, section *section)
>>>   {
>>> -  /* We can only be certain about functions defined in the same
>>> -     compilation unit.  */
>>> -  if (!TREE_STATIC (decl))
>>> -    return false;
>>> -
>>> -  /* Make sure that SYMBOL always binds to the definition in this
>>> -     compilation unit.  */
>>> -  if (!targetm.binds_local_p (decl))
>>> +  /* We can only be certain about the prevailing symbol definition.  */
>>> +  if (!decl_binds_to_current_def_p (decl))
>>>       return false;
>>>
>>>     /* If DECL_SECTION_NAME is set, assume it is trustworthy. */
>>>
>>>


Sorry to have missed this - I've also been traveling recently which has 
made it harder with patch traffic - this is OK if no regressions.

Please apply with an appropriate Changelog.

regressions
Ramana

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

[Alex Velenko]

On 03/03/15 15:58, Alex Velenko wrote:
> On 19/02/15 17:26, Richard Henderson wrote:
>> On 02/19/2015 09:08 AM, Alex Velenko wrote:
>>> Your suggestion seem to fix gcc.target/arm/long-calls-1.c, but has 
>>> to be
>>> thoroughly tested.
>>
>> Before you do complete testing, please also delete the TREE_STATIC test.
>> That bit should never be relevant to functions, as it indicates not that
>> it is in the compilation unit, but that it has static (as opposed to
>> automatic) storage duration.  Thus it is only relevant to variables.
>>
>>
>> r~
>>
>>
>>
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>> index 7bf5b4d..777230e 100644
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -6392,14 +6392,8 @@ arm_set_default_type_attributes (tree type)
>>   static bool
>>   arm_function_in_section_p (tree decl, section *section)
>>   {
>> -  /* We can only be certain about functions defined in the same
>> -     compilation unit.  */
>> -  if (!TREE_STATIC (decl))
>> -    return false;
>> -
>> -  /* Make sure that SYMBOL always binds to the definition in this
>> -     compilation unit.  */
>> -  if (!targetm.binds_local_p (decl))
>> +  /* We can only be certain about the prevailing symbol definition.  */
>> +  if (!decl_binds_to_current_def_p (decl))
>>       return false;
>>
>>     /* If DECL_SECTION_NAME is set, assume it is trustworthy. */
>>
>>
>
> Hi,
>
> Did a bootstrap and a full regression run on arm-none-linux-gnueabihf,
> No new regressions found. Some previously failing tests in libstdc++ 
> started to fail differently, for example:
>
> < ERROR: 22_locale/num_get/get/wchar_t/2.cc: can't read 
> "additional_sources": no such variable for " dg-do 22 run { xfail 
> lax_strtof\
> p } "
> < UNRESOLVED: 22_locale/num_get/get/wchar_t/2.cc: can't read 
> "additional_sources": no such variable for " dg-do 22 run { xfail lax_s\
> trtofp } "
> ---
> > ERROR: 22_locale/num_get/get/wchar_t/2.cc: can't read 
> "et_cache(uclibc,value)": no such element in array for " dg-do 22 run 
> { xfai\
> l lax_strtofp } "
> > UNRESOLVED: 22_locale/num_get/get/wchar_t/2.cc: can't read 
> "et_cache(uclibc,value)": no such element in array for " dg-do 22 run {\
>  xfail lax_strtofp } "
>
>
> But I think it is okay.
>
> Kind regards,
> Alex
>
>

Hi,
Ping. Could someone, please approve Richard's patch?
This issue needs fixing.
Kind regards,
Alex

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

[Alex Velenko]

On 19/02/15 17:26, Richard Henderson wrote:
> On 02/19/2015 09:08 AM, Alex Velenko wrote:
>> Your suggestion seem to fix gcc.target/arm/long-calls-1.c, but has to be
>> thoroughly tested.
>
> Before you do complete testing, please also delete the TREE_STATIC test.
> That bit should never be relevant to functions, as it indicates not that
> it is in the compilation unit, but that it has static (as opposed to
> automatic) storage duration.  Thus it is only relevant to variables.
>
>
> r~
>
>
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 7bf5b4d..777230e 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -6392,14 +6392,8 @@ arm_set_default_type_attributes (tree type)
>   static bool
>   arm_function_in_section_p (tree decl, section *section)
>   {
> -  /* We can only be certain about functions defined in the same
> -     compilation unit.  */
> -  if (!TREE_STATIC (decl))
> -    return false;
> -
> -  /* Make sure that SYMBOL always binds to the definition in this
> -     compilation unit.  */
> -  if (!targetm.binds_local_p (decl))
> +  /* We can only be certain about the prevailing symbol definition.  */
> +  if (!decl_binds_to_current_def_p (decl))
>       return false;
>
>     /* If DECL_SECTION_NAME is set, assume it is trustworthy.  */
>
>

Hi,

Did a bootstrap and a full regression run on arm-none-linux-gnueabihf,
No new regressions found. Some previously failing tests in libstdc++ 
started to fail differently, for example:

< ERROR: 22_locale/num_get/get/wchar_t/2.cc: can't read 
"additional_sources": no such variable for " dg-do 22 run { xfail 
lax_strtof\
p } "
< UNRESOLVED: 22_locale/num_get/get/wchar_t/2.cc: can't read 
"additional_sources": no such variable for " dg-do 22 run { xfail lax_s\
trtofp } "
---
 > ERROR: 22_locale/num_get/get/wchar_t/2.cc: can't read 
"et_cache(uclibc,value)": no such element in array for " dg-do 22 run { 
xfai\
l lax_strtofp } "
 > UNRESOLVED: 22_locale/num_get/get/wchar_t/2.cc: can't read 
"et_cache(uclibc,value)": no such element in array for " dg-do 22 run {\
  xfail lax_strtofp } "


But I think it is okay.

Kind regards,
Alex

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

[Richard Henderson]

On 02/19/2015 09:08 AM, Alex Velenko wrote:
> Your suggestion seem to fix gcc.target/arm/long-calls-1.c, but has to be
> thoroughly tested.

Before you do complete testing, please also delete the TREE_STATIC test.
That bit should never be relevant to functions, as it indicates not that
it is in the compilation unit, but that it has static (as opposed to
automatic) storage duration.  Thus it is only relevant to variables.


r~



diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 7bf5b4d..777230e 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6392,14 +6392,8 @@ arm_set_default_type_attributes (tree type)
 static bool
 arm_function_in_section_p (tree decl, section *section)
 {
-  /* We can only be certain about functions defined in the same
-     compilation unit.  */
-  if (!TREE_STATIC (decl))
-    return false;
-
-  /* Make sure that SYMBOL always binds to the definition in this
-     compilation unit.  */
-  if (!targetm.binds_local_p (decl))
+  /* We can only be certain about the prevailing symbol definition.  */
+  if (!decl_binds_to_current_def_p (decl))
     return false;

   /* If DECL_SECTION_NAME is set, assume it is trustworthy.  */

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

[Alex Velenko]

On 19/02/15 14:16, Richard Henderson wrote:
> On 02/18/2015 06:17 AM, Alex Velenko wrote:
>> By changing behaviour of varasm.c:default_binds_local_p, this patch changes
>> behaviour of gcc/config/arm/arm.c:arm_function_in_section_p and through it
>> breaks gcc/config/arm/arm.c:arm_is_long_call_p for weak symbols.
>>
>> As a result, I get regression for gcc.target/arm/long-calls-1.c on
>> arm-none-eabi:
>> FAIL: gcc.target/arm/long-calls-1.c scan-assembler-not \tbl?\tweak_l1\n
>> FAIL: gcc.target/arm/long-calls-1.c scan-assembler-not \tbl?\tweak_l3\n
>>
>> In https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html there
>> is a description for -mlong-calls.
>>
>> This has to be fixed.
>
> Please file a bug, for tracking purposes.
>
> That said, it looks as if arm_function_in_section_p should be using
> decl_binds_to_current_def_p instead of targetm.binds_local_p.
>
> That will properly reject weak symbols within a given module until we receive
> extra information from LTO indicating when a weak definition turns out to be
> the prevailing definition.
>
>
> r~
>

Hi Richard,
Thank you for your reply.
Here is the bug report.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65121

Your suggestion seem to fix gcc.target/arm/long-calls-1.c, but has to be 
thoroughly tested.

Kind regards,
Alex

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

[Richard Henderson]

On 02/18/2015 06:17 AM, Alex Velenko wrote:
> By changing behaviour of varasm.c:default_binds_local_p, this patch changes
> behaviour of gcc/config/arm/arm.c:arm_function_in_section_p and through it
> breaks gcc/config/arm/arm.c:arm_is_long_call_p for weak symbols.
> 
> As a result, I get regression for gcc.target/arm/long-calls-1.c on
> arm-none-eabi:
> FAIL: gcc.target/arm/long-calls-1.c scan-assembler-not \tbl?\tweak_l1\n
> FAIL: gcc.target/arm/long-calls-1.c scan-assembler-not \tbl?\tweak_l3\n
> 
> In https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html there
> is a description for -mlong-calls.
> 
> This has to be fixed.

Please file a bug, for tracking purposes.

That said, it looks as if arm_function_in_section_p should be using
decl_binds_to_current_def_p instead of targetm.binds_local_p.

That will properly reject weak symbols within a given module until we receive
extra information from LTO indicating when a weak definition turns out to be
the prevailing definition.


r~

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

[H.J. Lu]

On Wed, Feb 18, 2015 at 6:17 AM, Alex Velenko <Alex.Velenko@arm.com> wrote:
> On 13/02/15 05:11, Richard Henderson wrote:
>>
>> On 02/12/2015 08:14 PM, H.J. Lu wrote:
>>>
>>> I tried the second patch.  Results look good on Linux/x86-64.
>>
>>
>> Thanks.  My results concurr.  I went ahead and installed the patch as
>> posted.
>>
>>
>> r~
>>
>>
>> 2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
>>              Richard Henderson  <rth@redhat.com>
>>
>>          PR rtl/32219
>>          * cgraphunit.c (cgraph_node::finalize_function): Set definition
>>          before notice_global_symbol.
>>          (varpool_node::finalize_decl): Likewise.
>>          * varasm.c (default_binds_local_p_2): Rename from
>>          default_binds_local_p_1, add weak_dominate argument.  Use direct
>>          returns instead of assigning to local variable.  Unify varpool
>> and
>>          cgraph paths via symtab_node.  Reject undef weak variables before
>>          testing visibility.  Reorder tests for simplicity.
>>          (default_binds_local_p): Use default_binds_local_p_2.
>>          (default_binds_local_p_1): Likewise.
>>          (decl_binds_to_current_def_p): Unify varpool and cgraph paths
>>          via symtab_node.
>>          (default_elf_asm_output_external): Emit visibility when
>> specified.
>>
>> 2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
>>
>>          PR rtl/32219
>>          * gcc.dg/visibility-22.c: New test.
>>          * gcc.dg/visibility-23.c: New test.
>>          * gcc.target/i386/pr32219-1.c: New test.
>>          * gcc.target/i386/pr32219-2.c: New test.
>>          * gcc.target/i386/pr32219-3.c: New test.
>>          * gcc.target/i386/pr32219-4.c: New test.
>>          * gcc.target/i386/pr32219-5.c: New test.
>>          * gcc.target/i386/pr32219-6.c: New test.
>>          * gcc.target/i386/pr32219-7.c: New test.
>>          * gcc.target/i386/pr32219-8.c: New test.
>>          * gcc.target/i386/pr64317.c: Expect GOTOFF, not GOT.
>>
>
> Hi all,
> By changing behaviour of varasm.c:default_binds_local_p, this patch changes
> behaviour of gcc/config/arm/arm.c:arm_function_in_section_p and through it
> breaks gcc/config/arm/arm.c:arm_is_long_call_p for weak symbols.
>
> As a result, I get regression for gcc.target/arm/long-calls-1.c on
> arm-none-eabi:
> FAIL: gcc.target/arm/long-calls-1.c scan-assembler-not \tbl?\tweak_l1\n
> FAIL: gcc.target/arm/long-calls-1.c scan-assembler-not \tbl?\tweak_l3\n
>
> In https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html there
> is a description for -mlong-calls.

I know nothing about arm.  I built a cross compiler and got:

[hjl@gnu-tools-1 gcc]$ cat /tmp/z.i
const char *
__attribute__((long_call)) __attribute__((noinline))
strong_l1 (void) { return "strong_l1"; }
const char * call_strong_l1 (void) { return strong_l1 () + 1; }
const char * sibcall_strong_l1 (void) { return strong_l1 (); }
const char *
__attribute__((weak)) __attribute__((long_call)) __attribute__((noinline))
weak_l1 (void) { return "weak_l1"; }
const char * call_weak_l1 (void) { return weak_l1 () + 1; }
const char * sibcall_weak_l1 (void) { return weak_l1 (); }
[hjl@gnu-tools-1 gcc]$ ./xgcc -B./ -S -O2 /tmp/z.i
[hjl@gnu-tools-1 gcc]$ grep "b.*        strong_l1" z.s
.global strong_l1
bl strong_l1
b strong_l1
[hjl@gnu-tools-1 gcc]$ grep "b.*        weak_l1" z.s
bl weak_l1
bl weak_l1
[hjl@gnu-tools-1 gcc]$

Can someone tell me what is wrong with the output and why?

-- 
H.J.

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

[Alex Velenko]

On 13/02/15 05:11, Richard Henderson wrote:
> On 02/12/2015 08:14 PM, H.J. Lu wrote:
>> I tried the second patch.  Results look good on Linux/x86-64.
>
> Thanks.  My results concurr.  I went ahead and installed the patch as posted.
>
>
> r~
>
>
> 2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
>              Richard Henderson  <rth@redhat.com>
>
>          PR rtl/32219
>          * cgraphunit.c (cgraph_node::finalize_function): Set definition
>          before notice_global_symbol.
>          (varpool_node::finalize_decl): Likewise.
>          * varasm.c (default_binds_local_p_2): Rename from
>          default_binds_local_p_1, add weak_dominate argument.  Use direct
>          returns instead of assigning to local variable.  Unify varpool and
>          cgraph paths via symtab_node.  Reject undef weak variables before
>          testing visibility.  Reorder tests for simplicity.
>          (default_binds_local_p): Use default_binds_local_p_2.
>          (default_binds_local_p_1): Likewise.
>          (decl_binds_to_current_def_p): Unify varpool and cgraph paths
>          via symtab_node.
>          (default_elf_asm_output_external): Emit visibility when specified.
>
> 2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
>
>          PR rtl/32219
>          * gcc.dg/visibility-22.c: New test.
>          * gcc.dg/visibility-23.c: New test.
>          * gcc.target/i386/pr32219-1.c: New test.
>          * gcc.target/i386/pr32219-2.c: New test.
>          * gcc.target/i386/pr32219-3.c: New test.
>          * gcc.target/i386/pr32219-4.c: New test.
>          * gcc.target/i386/pr32219-5.c: New test.
>          * gcc.target/i386/pr32219-6.c: New test.
>          * gcc.target/i386/pr32219-7.c: New test.
>          * gcc.target/i386/pr32219-8.c: New test.
>          * gcc.target/i386/pr64317.c: Expect GOTOFF, not GOT.
>

Hi all,
By changing behaviour of varasm.c:default_binds_local_p, this patch 
changes behaviour of gcc/config/arm/arm.c:arm_function_in_section_p and 
through it breaks gcc/config/arm/arm.c:arm_is_long_call_p for weak symbols.

As a result, I get regression for gcc.target/arm/long-calls-1.c on
arm-none-eabi:
FAIL: gcc.target/arm/long-calls-1.c scan-assembler-not \tbl?\tweak_l1\n
FAIL: gcc.target/arm/long-calls-1.c scan-assembler-not \tbl?\tweak_l3\n

In https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html there
is a description for -mlong-calls.

This has to be fixed.

Kind regards,
Alex

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

[Richard Henderson]

On 02/12/2015 08:14 PM, H.J. Lu wrote:
> I tried the second patch.  Results look good on Linux/x86-64.

Thanks.  My results concurr.  I went ahead and installed the patch as posted.


r~


2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>
            Richard Henderson  <rth@redhat.com>

        PR rtl/32219
        * cgraphunit.c (cgraph_node::finalize_function): Set definition
        before notice_global_symbol.
        (varpool_node::finalize_decl): Likewise.
        * varasm.c (default_binds_local_p_2): Rename from
        default_binds_local_p_1, add weak_dominate argument.  Use direct
        returns instead of assigning to local variable.  Unify varpool and
        cgraph paths via symtab_node.  Reject undef weak variables before
        testing visibility.  Reorder tests for simplicity.
        (default_binds_local_p): Use default_binds_local_p_2.
        (default_binds_local_p_1): Likewise.
        (decl_binds_to_current_def_p): Unify varpool and cgraph paths
        via symtab_node.
        (default_elf_asm_output_external): Emit visibility when specified.

2015-02-12  H.J. Lu  <hongjiu.lu@intel.com>

        PR rtl/32219
        * gcc.dg/visibility-22.c: New test.
        * gcc.dg/visibility-23.c: New test.
        * gcc.target/i386/pr32219-1.c: New test.
        * gcc.target/i386/pr32219-2.c: New test.
        * gcc.target/i386/pr32219-3.c: New test.
        * gcc.target/i386/pr32219-4.c: New test.
        * gcc.target/i386/pr32219-5.c: New test.
        * gcc.target/i386/pr32219-6.c: New test.
        * gcc.target/i386/pr32219-7.c: New test.
        * gcc.target/i386/pr32219-8.c: New test.
        * gcc.target/i386/pr64317.c: Expect GOTOFF, not GOT.

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

[H.J. Lu]

On Thu, Feb 12, 2015 at 4:04 PM, Richard Henderson <rth@redhat.com> wrote:
> On 02/12/2015 03:05 PM, H.J. Lu wrote:
>> @@ -6830,9 +6830,15 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
>>    bool resolved_locally = false;
>>    if (symtab_node *node = symtab_node::get (exp))
>>      {
>> -      /* When not building shared library and weak_dominate is true:
>> -         weak, common or initialized symbols are resolved locally.  */
>> -      if ((weak_dominate && !shlib && node->definition)
>> +      /* When weak_dominate is true and not building shared library or
>> +      non-default visibility is specified by user: weak, common or
>> +      initialized symbols are resolved locally.
>> +      */
>> +      if (((!shlib
>> +         || (DECL_VISIBILITY_SPECIFIED (exp)
>> +             && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT))
>> +        && weak_dominate
>> +        && node->definition)
>>         || node->in_other_partition
>>         || resolution_local_p (node->resolution))
>>       resolved_locally = true;
>
> Hum.  I don't find that particularly easy to reason with either.
>
> How about this?  I'm about half-way through regression testing on it.
>
> I re-instated the use of resolution_to_local_definition_p, and attempt to infer
> a proper value for that when lto isn't in use.  I use this to eliminate only
> undef-weak early, rather than non-dominate weak.
>
> I re-instated the use of the existence of the local definition in the
> DECL_VISIBILITY test.  But unlike before, I reason that this allows us to
> eliminate the second visibility check.  We either have an assertion from the
> user (SPECIFIED), or we know we have a definition.  We no longer rely on the
> DECL_EXTERNAL test in the middle eliminating symbols without a definition.
>
> I shuffled some of the "return false" tests around among themselves, attempting
> to put the simplest test first.  No change in behavior there.
>
> (First patch is delta from the 5-patch bundle; second patch is the
> composite from trunk, to avoid confusion.)
>
>

I tried the second patch.  Results look good on Linux/x86-64.

Thanks.


-- 
H.J.

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

[Richard Henderson]

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

On 02/12/2015 03:05 PM, H.J. Lu wrote:
> @@ -6830,9 +6830,15 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
>    bool resolved_locally = false;
>    if (symtab_node *node = symtab_node::get (exp))
>      {
> -      /* When not building shared library and weak_dominate is true:
> -         weak, common or initialized symbols are resolved locally.  */
> -      if ((weak_dominate && !shlib && node->definition)
> +      /* When weak_dominate is true and not building shared library or
> +	 non-default visibility is specified by user: weak, common or
> +	 initialized symbols are resolved locally.
> +	 */
> +      if (((!shlib
> +	    || (DECL_VISIBILITY_SPECIFIED (exp)
> +		&& DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT))
> +	   && weak_dominate
> +	   && node->definition)
>  	  || node->in_other_partition
>  	  || resolution_local_p (node->resolution))
>  	resolved_locally = true;

Hum.  I don't find that particularly easy to reason with either.

How about this?  I'm about half-way through regression testing on it.

I re-instated the use of resolution_to_local_definition_p, and attempt to infer
a proper value for that when lto isn't in use.  I use this to eliminate only
undef-weak early, rather than non-dominate weak.

I re-instated the use of the existence of the local definition in the
DECL_VISIBILITY test.  But unlike before, I reason that this allows us to
eliminate the second visibility check.  We either have an assertion from the
user (SPECIFIED), or we know we have a definition.  We no longer rely on the
DECL_EXTERNAL test in the middle eliminating symbols without a definition.

I shuffled some of the "return false" tests around among themselves, attempting
to put the simplest test first.  No change in behavior there.

(First patch is delta from the 5-patch bundle; second patch is the
composite from trunk, to avoid confusion.)


r~

[-- Attachment #2: zz --]
[-- Type: text/plain, Size: 3243 bytes --]

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 057eedb..942826d 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -442,8 +442,10 @@ cgraph_node::finalize_function (tree decl, bool no_collect)
       node->local.redefined_extern_inline = true;
     }
 
-  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);
   node->lowered = DECL_STRUCT_FUNCTION (decl)->cfg != NULL;
 
   /* With -fkeep-inline-functions we are keeping all inline functions except
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 9f79416..0211306 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6828,37 +6828,42 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
      because dynamic linking might overwrite symbols
      in shared libraries.  */
   bool resolved_locally = false;
+  bool defined_locally = false;
   if (symtab_node *node = symtab_node::get (exp))
     {
-      /* 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
-	  || resolution_local_p (node->resolution))
+      if (node->definition || node->in_other_partition)
+	{
+	  defined_locally = true;
+	  resolved_locally = (weak_dominate && !shlib);
+	}
+      if (resolution_to_local_definition_p (node->resolution))
+	defined_locally = resolved_locally = true;
+      else if (resolution_local_p (node->resolution))
 	resolved_locally = true;
     }
 
-  /* Undefined (or non-dominant) weak symbols are not defined locally.  */
-  if (DECL_WEAK (exp) && !resolved_locally)
+  /* Undefined weak symbols are never defined locally.  */
+  if (DECL_WEAK (exp) && !defined_locally)
     return false;
 
-  /* A variable is local if the user has said explicitly that it will be.  */
-  if (DECL_VISIBILITY_SPECIFIED (exp)
-      && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
+  /* A symbol is local if the user has said explicitly that it will be,
+     or if we have a definition for the symbol.  We cannot infer visibility
+     for undefined symbols.  */
+  if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT
+      && (DECL_VISIBILITY_SPECIFIED (exp) || defined_locally))
     return true;
 
+  /* If PIC, then assume that any global name can be overridden by
+     symbols resolved from other modules.  */
+  if (shlib)
+    return false;
+
   /* Variables defined outside this object might not be local.  */
   if (DECL_EXTERNAL (exp) && !resolved_locally)
     return false;
 
-  /* If defined in this object and visibility is not default,
-     must be local.  */
-  if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
-    return true;
-
-  /* If PIC, then assume that any global name can be overridden by
-     symbols resolved from other modules.  */
-  if (shlib)
+  /* Non-dominant weak symbols are not defined locally.  */
+  if (DECL_WEAK (exp) && !resolved_locally)
     return false;
 
   /* Uninitialized COMMON variable may be unified with symbols

[-- Attachment #3: z3 --]
[-- Type: text/plain, Size: 17327 bytes --]

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index f2c40d4..942826d 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -442,8 +442,10 @@ cgraph_node::finalize_function (tree decl, bool no_collect)
       node->local.redefined_extern_inline = true;
     }
 
-  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);
   node->lowered = DECL_STRUCT_FUNCTION (decl)->cfg != NULL;
 
   /* With -fkeep-inline-functions we are keeping all inline functions except
@@ -803,8 +805,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..52f59be
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/visibility-22.c
@@ -0,0 +1,17 @@
+/* PR target/32219 */
+/* { dg-do run } */
+/* { dg-require-visibility "" } */
+/* { dg-options "-O2 -fPIC" { target fpic } } */
+/* This test requires support for undefined weak symbols.  This support
+   is not available on hppa*-*-hpux*.  The test is skipped rather than
+   xfailed to suppress the warning that would otherwise arise.  */
+/* { dg-skip-if "" { "hppa*-*-hpux*" "*-*-aix*" "*-*-darwin*" } "*" { "" } }  */
+
+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..0fa9ef4
--- /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 "-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;
+}
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 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/varasm.c b/gcc/varasm.c
index 3f62fca..0211306 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6802,97 +6802,96 @@ resolution_local_p (enum ld_plugin_symbol_resolution resolution)
 	  || resolution == LDPR_RESOLVED_EXEC);
 }
 
-/* Assume ELF-ish defaults, since that's pretty much the most liberal
-   wrt cross-module name binding.  */
-
-bool
-default_binds_local_p (const_tree exp)
-{
-  return default_binds_local_p_1 (exp, flag_shlib);
-}
-
-bool
-default_binds_local_p_1 (const_tree exp, int shlib)
+static bool
+default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
 {
-  bool local_p;
-  bool resolved_locally = false;
-  bool resolved_to_local_def = false;
-
-  /* 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.  */
-  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;
-    }
-  else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp))
-    {
-      struct cgraph_node *node = cgraph_node::get (exp);
-      if (node
-	  && (resolution_local_p (node->resolution) || node->in_other_partition))
-	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;
+    return 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))
+  if (lookup_attribute ("weakref", DECL_ATTRIBUTES (exp))
 	   || (TREE_CODE (exp) == FUNCTION_DECL
 	       && lookup_attribute ("ifunc", DECL_ATTRIBUTES (exp))))
-    local_p = false;
+    return 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;
-  /* 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 (! TREE_PUBLIC (exp))
+    return true;
+
+  /* With resolution file in hand, take look into resolutions.
+     We can't just return true for resolved_locally symbols,
+     because dynamic linking might overwrite symbols
+     in shared libraries.  */
+  bool resolved_locally = false;
+  bool defined_locally = false;
+  if (symtab_node *node = symtab_node::get (exp))
+    {
+      if (node->definition || node->in_other_partition)
+	{
+	  defined_locally = true;
+	  resolved_locally = (weak_dominate && !shlib);
+	}
+      if (resolution_to_local_definition_p (node->resolution))
+	defined_locally = resolved_locally = true;
+      else if (resolution_local_p (node->resolution))
+	resolved_locally = true;
+    }
+
+  /* Undefined weak symbols are never defined locally.  */
+  if (DECL_WEAK (exp) && !defined_locally)
+    return false;
+
+  /* A symbol is local if the user has said explicitly that it will be,
+     or if we have a definition for the symbol.  We cannot infer visibility
+     for undefined symbols.  */
+  if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT
+      && (DECL_VISIBILITY_SPECIFIED (exp) || defined_locally))
+    return true;
+
   /* If PIC, then assume that any global name can be overridden by
      symbols resolved from other modules.  */
-  else if (shlib)
-    local_p = false;
+  if (shlib)
+    return false;
+
+  /* Variables defined outside this object might not be local.  */
+  if (DECL_EXTERNAL (exp) && !resolved_locally)
+    return false;
+
+  /* Non-dominant weak symbols are not defined locally.  */
+  if (DECL_WEAK (exp) && !resolved_locally)
+    return 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_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.  */
+
+bool
+default_binds_local_p (const_tree exp)
+{
+  return default_binds_local_p_2 (exp, flag_shlib != 0, true);
+}
 
-  return local_p;
+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
@@ -6914,22 +6913,14 @@ decl_binds_to_current_def_p (const_tree decl)
     return false;
   if (!TREE_PUBLIC (decl))
     return true;
+
   /* When resolution is available, just use it.  */
-  if (TREE_CODE (decl) == VAR_DECL
-      && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
+  if (symtab_node *node = symtab_node::get (decl))
     {
-      varpool_node *vnode = varpool_node::get (decl);
-      if (vnode
-	  && vnode->resolution != LDPR_UNKNOWN)
-	return resolution_to_local_definition_p (vnode->resolution);
-    }
-  else if (TREE_CODE (decl) == FUNCTION_DECL)
-    {
-      struct cgraph_node *node = cgraph_node::get (decl);
-      if (node
-	  && node->resolution != LDPR_UNKNOWN)
+      if (node->resolution != LDPR_UNKNOWN)
 	return resolution_to_local_definition_p (node->resolution);
     }
+
   /* Otherwise we have to assume the worst for DECL_WEAK (hidden weaks
      binds locally but still can be overwritten), DECL_COMMON (can be merged
      with a non-common definition somewhere in the same module) or
@@ -7449,9 +7440,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);
 }
 

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

[H.J. Lu]

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

On Thu, Feb 12, 2015 at 3:04 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Feb 12, 2015 at 11:25 AM, Richard Henderson <rth@redhat.com> wrote:
>> On 02/12/2015 10:58 AM, H.J. Lu wrote:
>>>    if (DECL_VISIBILITY_SPECIFIED (exp)
>>> +      && (resolved_locally
>>> +       || !flag_pic
>>
>> Yes, this essentially goes back to your original patch, which I claim still
>> conflates issues.
>>
>> In particular, I believe mentioning flag_pic here is a serious error.
>>
>> There are two problems that I see so far,
>>
>>  (1) node->definition isn't set for this symbol.  This is because you
>>      only fixed varpool_node::finalize_decl, and not
>>      cgraph_node::finalize_function.
>>
>>  (2) The weak test should probably be split into two pieces, like the
>>      visibility test: exclude undefined weak, include specified visibility,
>>      exclude non-dominant weak, exclude external, include implied visibility.
>>
>
> How about this patch?
>

Oops.  Wrong one.


-- 
H.J.

[-- Attachment #2: 0001-non-default-visibility-is-local.patch --]
[-- Type: text/x-patch, Size: 1899 bytes --]

From afbb56ab924d9f419ac4f65f5c535ebdbd22f16e Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 12 Feb 2015 15:03:00 -0800
Subject: [PATCH] non-default visibility is local

---
 gcc/cgraphunit.c |  4 +++-
 gcc/varasm.c     | 12 +++++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 057eedb..942826d 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -442,8 +442,10 @@ cgraph_node::finalize_function (tree decl, bool no_collect)
       node->local.redefined_extern_inline = true;
     }
 
-  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);
   node->lowered = DECL_STRUCT_FUNCTION (decl)->cfg != NULL;
 
   /* With -fkeep-inline-functions we are keeping all inline functions except
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 9f79416..5d7cba1 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6830,9 +6830,15 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
   bool resolved_locally = false;
   if (symtab_node *node = symtab_node::get (exp))
     {
-      /* When not building shared library and weak_dominate is true:
-         weak, common or initialized symbols are resolved locally.  */
-      if ((weak_dominate && !shlib && node->definition)
+      /* When weak_dominate is true and not building shared library or
+	 non-default visibility is specified by user: weak, common or
+	 initialized symbols are resolved locally.
+	 */
+      if (((!shlib
+	    || (DECL_VISIBILITY_SPECIFIED (exp)
+		&& DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT))
+	   && weak_dominate
+	   && node->definition)
 	  || node->in_other_partition
 	  || resolution_local_p (node->resolution))
 	resolved_locally = true;
-- 
1.9.3

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

[H.J. Lu]

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

On Thu, Feb 12, 2015 at 11:25 AM, Richard Henderson <rth@redhat.com> wrote:
> On 02/12/2015 10:58 AM, H.J. Lu wrote:
>>    if (DECL_VISIBILITY_SPECIFIED (exp)
>> +      && (resolved_locally
>> +       || !flag_pic
>
> Yes, this essentially goes back to your original patch, which I claim still
> conflates issues.
>
> In particular, I believe mentioning flag_pic here is a serious error.
>
> There are two problems that I see so far,
>
>  (1) node->definition isn't set for this symbol.  This is because you
>      only fixed varpool_node::finalize_decl, and not
>      cgraph_node::finalize_function.
>
>  (2) The weak test should probably be split into two pieces, like the
>      visibility test: exclude undefined weak, include specified visibility,
>      exclude non-dominant weak, exclude external, include implied visibility.
>

How about this patch?


-- 
H.J.

[-- Attachment #2: 0001-A-variable-is-local-if-specified-by-user.patch --]
[-- Type: text/x-patch, Size: 1499 bytes --]

From 3b516badec25151acd4a96fa4ef07b3f88e3f053 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 12 Feb 2015 10:55:55 -0800
Subject: [PATCH] A variable is local if specified by user

And it is resolved or defined locally, not compiling for PIC or not weak.
---
 gcc/varasm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/gcc/varasm.c b/gcc/varasm.c
index 9f79416..b14f2d3 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6838,15 +6838,21 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
 	resolved_locally = true;
     }
 
-  /* Undefined (or non-dominant) weak symbols are not defined locally.  */
-  if (DECL_WEAK (exp) && !resolved_locally)
-    return false;
-
-  /* A variable is local if the user has said explicitly that it will be.  */
+  /* A variable is local if the user has said explicitly that it will
+     be and it is resolved or defined locally, not compiling for PIC or
+     not weak.  */
   if (DECL_VISIBILITY_SPECIFIED (exp)
+      && (resolved_locally
+	  || !flag_pic
+	  || !DECL_EXTERNAL (exp)
+	  || !DECL_WEAK (exp))
       && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
     return true;
 
+  /* Undefined (or non-dominant) weak symbols are not defined locally.  */
+  if (DECL_WEAK (exp) && !resolved_locally)
+    return false;
+
   /* Variables defined outside this object might not be local.  */
   if (DECL_EXTERNAL (exp) && !resolved_locally)
     return false;
-- 
1.9.3

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

[Jack Howarth]

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

On Thu, Feb 12, 2015 at 2:18 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Feb 12, 2015 at 11:16 AM, Jack Howarth <howarth.at.gcc@gmail.com> wrote:
>> H.J.,
>>    Oddly I saw no regressions in the g++ test suite at -m32/-m64 on
>> x86_64-apple-darwin14.
>>            Jack
>
> They have
>
> // { dg-require-effective-target tls }
>
> Does x86_64-apple-darwin14 support TLS?  If yes, what does the
> generated assembly code look like?

We use emutls on darwin. Those failing test are run at -m32/-m64...

Executing on host:
/sw/src/fink.build/gcc50-5.0.0-1000/darwin_objdir/gcc/testsuite/g++/../../xg++
-B/sw/src/fink.build/gcc50-5.0.0-1000/darwin_objdir/gcc/testsuite/g++/../../
/sw/src/fink.build/gcc50-5.0.0-1000/gcc-5-20150212/gcc/testsuite/g++.dg/gomp/tls-wrap4.C
 -fno-diagnostics-show-caret -fdiagnostics-color=never  -nostdinc++
-I/sw/src/fink.build/gcc50-5.0.0-1000/darwin_objdir/x86_64-apple-darwin13.4.0/libstdc++-v3/include/x86_64-apple-darwin13.4.0
-I/sw/src/fink.build/gcc50-5.0.0-1000/darwin_objdir/x86_64-apple-darwin13.4.0/libstdc++-v3/include
-I/sw/src/fink.build/gcc50-5.0.0-1000/gcc-5-20150212/libstdc++-v3/libsupc++
-I/sw/src/fink.build/gcc50-5.0.0-1000/gcc-5-20150212/libstdc++-v3/include/backward
-I/sw/src/fink.build/gcc50-5.0.0-1000/gcc-5-20150212/libstdc++-v3/testsuite/util
-fmessage-length=0  -std=gnu++11 -fPIC  -S  -m64  -o tls-wrap4.s
(timeout = 300)
spawn -ignore SIGHUP
/sw/src/fink.build/gcc50-5.0.0-1000/darwin_objdir/gcc/testsuite/g++/../../xg++
-B/sw/src/fink.build/gcc50-5.0.0-1000/darwin_objdir/gcc/testsuite/g++/../../
/sw/src/fink.build/gcc50-5.0.0-1000/gcc-5-20150212/gcc/testsuite/g++.dg/gomp/tls-wrap4.C
-fno-diagnostics-show-caret -fdiagnostics-color=never -nostdinc++
-I/sw/src/fink.build/gcc50-5.0.0-1000/darwin_objdir/x86_64-apple-darwin13.4.0/libstdc++-v3/include/x86_64-apple-darwin13.4.0
-I/sw/src/fink.build/gcc50-5.0.0-1000/darwin_objdir/x86_64-apple-darwin13.4.0/libstdc++-v3/include
-I/sw/src/fink.build/gcc50-5.0.0-1000/gcc-5-20150212/libstdc++-v3/libsupc++
-I/sw/src/fink.build/gcc50-5.0.0-1000/gcc-5-20150212/libstdc++-v3/include/backward
-I/sw/src/fink.build/gcc50-5.0.0-1000/gcc-5-20150212/libstdc++-v3/testsuite/util
-fmessage-length=0 -std=gnu++11 -fPIC -S -m64 -o tls-wrap4.s^M
PASS: g++.dg/gomp/tls-wrap4.C  -std=gnu++11 (test for excess errors)
PASS: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT

and produce the attached assembly.
                Jack

>
>> On Thu, Feb 12, 2015 at 1:16 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Wed, Feb 11, 2015 at 10:22 PM, Richard Henderson <rth@redhat.com> wrote:
>>>> On 02/10/2015 01:19 PM, Richard Henderson wrote:
>>>>> 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.
>>>>
>>>> The test is there to differentiate explicit visibility from that implied from
>>>> the command-line.  Without it, we assume hidden visibility for external symbols
>>>> too early, making the command-line option useless.  This is visible even in
>>>> building libgcc.
>>>>
>>>> I believe this set of patches does what we want, and cleans things up a bit in
>>>> the process.
>>>>
>>>>
>>>
>>> I tried them on Linux/x86-64.  They caused:
>>>
>>> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
>>> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
>>> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
>>> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
>>> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
>>> scan-assembler-not _ZTW1i@PLT
>>> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
>>> scan-assembler-not _ZTW1i@PLT
>>> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
>>> scan-assembler-not _ZTW1i@PLT
>>> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
>>> scan-assembler-not _ZTW1i@PLT
>>>
>>>
>>> --
>>> H.J.
>
>
>
> --
> H.J.

[-- Attachment #2: tls-wrap4.s --]
[-- Type: application/octet-stream, Size: 1668 bytes --]

	.section __TEXT,__textcoal_nt,coalesced,pure_instructions
	.globl __ZTW1i
	.weak_definition __ZTW1i
	.private_extern __ZTW1i
__ZTW1i:
LFB1:
	pushq	%rbp
LCFI0:
	movq	%rsp, %rbp
LCFI1:
	movq	___emutls_v.i@GOTPCREL(%rip), %rax
	movq	%rax, %rdi
	call	___emutls_get_address
	popq	%rbp
LCFI2:
	ret
LFE1:
	.text
	.globl _main
_main:
LFB0:
	pushq	%rbp
LCFI3:
	movq	%rsp, %rbp
LCFI4:
	call	__ZTW1i
	movl	(%rax), %eax
	subl	$42, %eax
	popq	%rbp
LCFI5:
	ret
LFE0:
	.section __TEXT,__eh_frame,coalesced,no_toc+strip_static_syms+live_support
EH_frame1:
	.set L$set$0,LECIE1-LSCIE1
	.long L$set$0
LSCIE1:
	.long	0
	.byte	0x1
	.ascii "zR\0"
	.byte	0x1
	.byte	0x78
	.byte	0x10
	.byte	0x1
	.byte	0x10
	.byte	0xc
	.byte	0x7
	.byte	0x8
	.byte	0x90
	.byte	0x1
	.align 3
LECIE1:
LSFDE1:
	.set L$set$1,LEFDE1-LASFDE1
	.long L$set$1
LASFDE1:
	.long	LASFDE1-EH_frame1
	.quad	LFB1-.
	.set L$set$2,LFE1-LFB1
	.quad L$set$2
	.byte	0
	.byte	0x4
	.set L$set$3,LCFI0-LFB1
	.long L$set$3
	.byte	0xe
	.byte	0x10
	.byte	0x86
	.byte	0x2
	.byte	0x4
	.set L$set$4,LCFI1-LCFI0
	.long L$set$4
	.byte	0xd
	.byte	0x6
	.byte	0x4
	.set L$set$5,LCFI2-LCFI1
	.long L$set$5
	.byte	0xc
	.byte	0x7
	.byte	0x8
	.align 3
LEFDE1:
LSFDE3:
	.set L$set$6,LEFDE3-LASFDE3
	.long L$set$6
LASFDE3:
	.long	LASFDE3-EH_frame1
	.quad	LFB0-.
	.set L$set$7,LFE0-LFB0
	.quad L$set$7
	.byte	0
	.byte	0x4
	.set L$set$8,LCFI3-LFB0
	.long L$set$8
	.byte	0xe
	.byte	0x10
	.byte	0x86
	.byte	0x2
	.byte	0x4
	.set L$set$9,LCFI4-LCFI3
	.long L$set$9
	.byte	0xd
	.byte	0x6
	.byte	0x4
	.set L$set$10,LCFI5-LCFI4
	.long L$set$10
	.byte	0xc
	.byte	0x7
	.byte	0x8
	.align 3
LEFDE3:
	.constructor
	.destructor
	.align 1
	.subsections_via_symbols

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

[Richard Henderson]

On 02/12/2015 10:58 AM, H.J. Lu wrote:
>    if (DECL_VISIBILITY_SPECIFIED (exp)
> +      && (resolved_locally
> +	  || !flag_pic

Yes, this essentially goes back to your original patch, which I claim still
conflates issues.

In particular, I believe mentioning flag_pic here is a serious error.

There are two problems that I see so far,

 (1) node->definition isn't set for this symbol.  This is because you
     only fixed varpool_node::finalize_decl, and not
     cgraph_node::finalize_function.

 (2) The weak test should probably be split into two pieces, like the
     visibility test: exclude undefined weak, include specified visibility,
     exclude non-dominant weak, exclude external, include implied visibility.


r~

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

[H.J. Lu]

On Thu, Feb 12, 2015 at 11:16 AM, Jack Howarth <howarth.at.gcc@gmail.com> wrote:
> H.J.,
>    Oddly I saw no regressions in the g++ test suite at -m32/-m64 on
> x86_64-apple-darwin14.
>            Jack

They have

// { dg-require-effective-target tls }

Does x86_64-apple-darwin14 support TLS?  If yes, what does the
generated assembly code look like?

> On Thu, Feb 12, 2015 at 1:16 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Wed, Feb 11, 2015 at 10:22 PM, Richard Henderson <rth@redhat.com> wrote:
>>> On 02/10/2015 01:19 PM, Richard Henderson wrote:
>>>> 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.
>>>
>>> The test is there to differentiate explicit visibility from that implied from
>>> the command-line.  Without it, we assume hidden visibility for external symbols
>>> too early, making the command-line option useless.  This is visible even in
>>> building libgcc.
>>>
>>> I believe this set of patches does what we want, and cleans things up a bit in
>>> the process.
>>>
>>>
>>
>> I tried them on Linux/x86-64.  They caused:
>>
>> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
>> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
>> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
>> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
>> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
>> scan-assembler-not _ZTW1i@PLT
>> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
>> scan-assembler-not _ZTW1i@PLT
>> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
>> scan-assembler-not _ZTW1i@PLT
>> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
>> scan-assembler-not _ZTW1i@PLT
>>
>>
>> --
>> H.J.



-- 
H.J.

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

[Jack Howarth]

H.J.,
   Oddly I saw no regressions in the g++ test suite at -m32/-m64 on
x86_64-apple-darwin14.
           Jack

On Thu, Feb 12, 2015 at 1:16 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Feb 11, 2015 at 10:22 PM, Richard Henderson <rth@redhat.com> wrote:
>> On 02/10/2015 01:19 PM, Richard Henderson wrote:
>>> 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.
>>
>> The test is there to differentiate explicit visibility from that implied from
>> the command-line.  Without it, we assume hidden visibility for external symbols
>> too early, making the command-line option useless.  This is visible even in
>> building libgcc.
>>
>> I believe this set of patches does what we want, and cleans things up a bit in
>> the process.
>>
>>
>
> I tried them on Linux/x86-64.  They caused:
>
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
> scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
> scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
> scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
> scan-assembler-not _ZTW1i@PLT
>
>
> --
> H.J.

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

[H.J. Lu]

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

On Thu, Feb 12, 2015 at 10:16 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Feb 11, 2015 at 10:22 PM, Richard Henderson <rth@redhat.com> wrote:
>> On 02/10/2015 01:19 PM, Richard Henderson wrote:
>>> 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.
>>
>> The test is there to differentiate explicit visibility from that implied from
>> the command-line.  Without it, we assume hidden visibility for external symbols
>> too early, making the command-line option useless.  This is visible even in
>> building libgcc.
>>
>> I believe this set of patches does what we want, and cleans things up a bit in
>> the process.
>>
>>
>
> I tried them on Linux/x86-64.  They caused:
>
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
> scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
> scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
> scan-assembler-not _ZTW1i@PLT
> FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
> scan-assembler-not _ZTW1i@PLT
>

I am testing this patch on top of yours.


-- 
H.J.

[-- Attachment #2: 0001-A-variable-is-local-if-specified-by-user.patch --]
[-- Type: text/x-patch, Size: 1499 bytes --]

From 3b516badec25151acd4a96fa4ef07b3f88e3f053 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 12 Feb 2015 10:55:55 -0800
Subject: [PATCH] A variable is local if specified by user

And it is resolved or defined locally, not compiling for PIC or not weak.
---
 gcc/varasm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/gcc/varasm.c b/gcc/varasm.c
index 9f79416..b14f2d3 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6838,15 +6838,21 @@ default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
 	resolved_locally = true;
     }
 
-  /* Undefined (or non-dominant) weak symbols are not defined locally.  */
-  if (DECL_WEAK (exp) && !resolved_locally)
-    return false;
-
-  /* A variable is local if the user has said explicitly that it will be.  */
+  /* A variable is local if the user has said explicitly that it will
+     be and it is resolved or defined locally, not compiling for PIC or
+     not weak.  */
   if (DECL_VISIBILITY_SPECIFIED (exp)
+      && (resolved_locally
+	  || !flag_pic
+	  || !DECL_EXTERNAL (exp)
+	  || !DECL_WEAK (exp))
       && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
     return true;
 
+  /* Undefined (or non-dominant) weak symbols are not defined locally.  */
+  if (DECL_WEAK (exp) && !resolved_locally)
+    return false;
+
   /* Variables defined outside this object might not be local.  */
   if (DECL_EXTERNAL (exp) && !resolved_locally)
     return false;
-- 
1.9.3

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

[H.J. Lu]

On Wed, Feb 11, 2015 at 10:22 PM, Richard Henderson <rth@redhat.com> wrote:
> On 02/10/2015 01:19 PM, Richard Henderson wrote:
>> 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.
>
> The test is there to differentiate explicit visibility from that implied from
> the command-line.  Without it, we assume hidden visibility for external symbols
> too early, making the command-line option useless.  This is visible even in
> building libgcc.
>
> I believe this set of patches does what we want, and cleans things up a bit in
> the process.
>
>

I tried them on Linux/x86-64.  They caused:

FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++11  scan-assembler-not _ZTW1i@PLT
FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
FAIL: g++.dg/gomp/tls-wrap4.C  -std=gnu++14  scan-assembler-not _ZTW1i@PLT
FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
scan-assembler-not _ZTW1i@PLT
FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++11
scan-assembler-not _ZTW1i@PLT
FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
scan-assembler-not _ZTW1i@PLT
FAIL: g++.dg/tls/thread_local-wrap4.C  -std=gnu++14
scan-assembler-not _ZTW1i@PLT


-- 
H.J.

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

[Richard Henderson]

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

On 02/10/2015 01:19 PM, Richard Henderson wrote:
> 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.

The test is there to differentiate explicit visibility from that implied from
the command-line.  Without it, we assume hidden visibility for external symbols
too early, making the command-line option useless.  This is visible even in
building libgcc.

I believe this set of patches does what we want, and cleans things up a bit in
the process.


r~

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Replace-local_p-with-direct-returns.patch --]
[-- Type: text/x-patch; name="0001-Replace-local_p-with-direct-returns.patch", Size: 3888 bytes --]

From bf5d4f20368d1173a8d586f6bdd258b00b807e33 Mon Sep 17 00:00:00 2001
From: Richard Henderson <rth@redhat.com>
Date: Wed, 11 Feb 2015 17:37:48 -0800
Subject: [PATCH 1/5] Replace local_p with direct returns

---
 gcc/varasm.c | 66 +++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 34 insertions(+), 32 deletions(-)

diff --git a/gcc/varasm.c b/gcc/varasm.c
index 3f62fca..83d9de3 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6814,7 +6814,6 @@ default_binds_local_p (const_tree exp)
 bool
 default_binds_local_p_1 (const_tree exp, int shlib)
 {
-  bool local_p;
   bool resolved_locally = false;
   bool resolved_to_local_def = false;
 
@@ -6845,54 +6844,57 @@ default_binds_local_p_1 (const_tree exp, int shlib)
 
   /* A non-decl is an entry in the constant pool.  */
   if (!DECL_P (exp))
-    local_p = true;
+    return 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))
+  if (lookup_attribute ("weakref", DECL_ATTRIBUTES (exp))
 	   || (TREE_CODE (exp) == FUNCTION_DECL
 	       && lookup_attribute ("ifunc", DECL_ATTRIBUTES (exp))))
-    local_p = false;
+    return 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;
+  if (! TREE_PUBLIC (exp))
+    return true;
+
+  /* A variable is local if the user has said explicitly that it will be.  */
+  if ((DECL_VISIBILITY_SPECIFIED (exp) || resolved_to_local_def)
+      && 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;
+  if (DECL_EXTERNAL (exp) && !resolved_locally)
+    return false;
+
+  /* If defined in this object and visibility is not default,
+     must be local.  */
+  if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
+    return 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 (DECL_WEAK (exp) && !resolved_locally)
+    return false;
+
   /* If PIC, then assume that any global name can be overridden by
      symbols resolved from other modules.  */
-  else if (shlib)
-    local_p = false;
+  if (shlib)
+    return 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_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 local_p;
+  return true;
 }
 
 /* Return true when references to DECL must bind to current definition in
-- 
2.1.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Use-symtab_node-to-unify-var-and-function-handling.patch --]
[-- Type: text/x-patch; name="0002-Use-symtab_node-to-unify-var-and-function-handling.patch", Size: 3821 bytes --]

From 0b1ac37c688093d720a99b5ae446602a0c1120b3 Mon Sep 17 00:00:00 2001
From: Richard Henderson <rth@redhat.com>
Date: Wed, 11 Feb 2015 19:27:14 -0800
Subject: [PATCH 2/5] Use symtab_node to unify var and function handling

Delay that until after static symbols are eliminated.
---
 gcc/varasm.c | 59 +++++++++++++++++++----------------------------------------
 1 file changed, 19 insertions(+), 40 deletions(-)

diff --git a/gcc/varasm.c b/gcc/varasm.c
index 83d9de3..dcc70e3 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6814,34 +6814,6 @@ default_binds_local_p (const_tree exp)
 bool
 default_binds_local_p_1 (const_tree exp, int shlib)
 {
-  bool resolved_locally = false;
-  bool resolved_to_local_def = false;
-
-  /* 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.  */
-  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;
-    }
-  else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp))
-    {
-      struct cgraph_node *node = cgraph_node::get (exp);
-      if (node
-	  && (resolution_local_p (node->resolution) || node->in_other_partition))
-	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))
     return true;
@@ -6860,6 +6832,21 @@ default_binds_local_p_1 (const_tree exp, int shlib)
   if (! TREE_PUBLIC (exp))
     return true;
 
+  /* With resolution file in hand, take look into resolutions.
+     We can't just return true for resolved_locally symbols,
+     because dynamic linking might overwrite symbols
+     in shared libraries.  */
+  bool resolved_locally = false;
+  bool resolved_to_local_def = false;
+  if (symtab_node *node = symtab_node::get (exp))
+    {
+      if (node->in_other_partition
+	  || resolution_local_p (node->resolution))
+	resolved_locally = true;
+      if (resolution_to_local_definition_p (node->resolution))
+	resolved_to_local_def = true;
+    }
+
   /* A variable is local if the user has said explicitly that it will be.  */
   if ((DECL_VISIBILITY_SPECIFIED (exp) || resolved_to_local_def)
       && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
@@ -6916,22 +6903,14 @@ decl_binds_to_current_def_p (const_tree decl)
     return false;
   if (!TREE_PUBLIC (decl))
     return true;
+
   /* When resolution is available, just use it.  */
-  if (TREE_CODE (decl) == VAR_DECL
-      && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
+  if (symtab_node *node = symtab_node::get (decl))
     {
-      varpool_node *vnode = varpool_node::get (decl);
-      if (vnode
-	  && vnode->resolution != LDPR_UNKNOWN)
-	return resolution_to_local_definition_p (vnode->resolution);
-    }
-  else if (TREE_CODE (decl) == FUNCTION_DECL)
-    {
-      struct cgraph_node *node = cgraph_node::get (decl);
-      if (node
-	  && node->resolution != LDPR_UNKNOWN)
+      if (node->resolution != LDPR_UNKNOWN)
 	return resolution_to_local_definition_p (node->resolution);
     }
+
   /* Otherwise we have to assume the worst for DECL_WEAK (hidden weaks
      binds locally but still can be overwritten), DECL_COMMON (can be merged
      with a non-common definition somewhere in the same module) or
-- 
2.1.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Don-t-use-resolution_to_local_definition_p-in-binds_.patch --]
[-- Type: text/x-patch; name="0003-Don-t-use-resolution_to_local_definition_p-in-binds_.patch", Size: 1232 bytes --]

From 2a5208dfd10bf5d9b9c63c1f7464c6b6cacbcd51 Mon Sep 17 00:00:00 2001
From: Richard Henderson <rth@redhat.com>
Date: Wed, 11 Feb 2015 19:37:25 -0800
Subject: [PATCH 3/5] Don't use resolution_to_local_definition_p in
 binds_local_p

---
 gcc/varasm.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/gcc/varasm.c b/gcc/varasm.c
index dcc70e3..f467cc0 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6837,18 +6837,15 @@ default_binds_local_p_1 (const_tree exp, int shlib)
      because dynamic linking might overwrite symbols
      in shared libraries.  */
   bool resolved_locally = false;
-  bool resolved_to_local_def = false;
   if (symtab_node *node = symtab_node::get (exp))
     {
       if (node->in_other_partition
 	  || resolution_local_p (node->resolution))
 	resolved_locally = true;
-      if (resolution_to_local_definition_p (node->resolution))
-	resolved_to_local_def = true;
     }
 
   /* A variable is local if the user has said explicitly that it will be.  */
-  if ((DECL_VISIBILITY_SPECIFIED (exp) || resolved_to_local_def)
+  if (DECL_VISIBILITY_SPECIFIED (exp)
       && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
     return true;
 
-- 
2.1.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Add-weak_dominate-logic.patch --]
[-- Type: text/x-patch; name="0004-Add-weak_dominate-logic.patch", Size: 2952 bytes --]

From aa50d80d97a27e99cc295e3f2a00662dc73ce235 Mon Sep 17 00:00:00 2001
From: Richard Henderson <rth@redhat.com>
Date: Wed, 11 Feb 2015 20:11:09 -0800
Subject: [PATCH 4/5] Add weak_dominate logic

---
 gcc/varasm.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/gcc/varasm.c b/gcc/varasm.c
index f467cc0..dea9b36 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6802,17 +6802,8 @@ resolution_local_p (enum ld_plugin_symbol_resolution resolution)
 	  || resolution == LDPR_RESOLVED_EXEC);
 }
 
-/* Assume ELF-ish defaults, since that's pretty much the most liberal
-   wrt cross-module name binding.  */
-
-bool
-default_binds_local_p (const_tree exp)
-{
-  return default_binds_local_p_1 (exp, flag_shlib);
-}
-
-bool
-default_binds_local_p_1 (const_tree exp, int shlib)
+static bool
+default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate)
 {
   /* A non-decl is an entry in the constant pool.  */
   if (!DECL_P (exp))
@@ -6839,11 +6830,18 @@ default_binds_local_p_1 (const_tree exp, int shlib)
   bool resolved_locally = false;
   if (symtab_node *node = symtab_node::get (exp))
     {
-      if (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
 	  || resolution_local_p (node->resolution))
 	resolved_locally = true;
     }
 
+  /* Undefined (or non-dominant) weak symbols are not defined locally.  */
+  if (DECL_WEAK (exp) && !resolved_locally)
+    return false;
+
   /* A variable is local if the user has said explicitly that it will be.  */
   if (DECL_VISIBILITY_SPECIFIED (exp)
       && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
@@ -6858,11 +6856,6 @@ default_binds_local_p_1 (const_tree exp, int shlib)
   if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
     return true;
 
-  /* Default visibility weak data can be overridden by a strong symbol
-     in another module and so are not local.  */
-  if (DECL_WEAK (exp) && !resolved_locally)
-    return false;
-
   /* If PIC, then assume that any global name can be overridden by
      symbols resolved from other modules.  */
   if (shlib)
@@ -6881,6 +6874,21 @@ default_binds_local_p_1 (const_tree exp, int shlib)
   return true;
 }
 
+/* Assume ELF-ish defaults, since that's pretty much the most liberal
+   wrt cross-module name binding.  */
+
+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
    final executable.
 
-- 
2.1.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Rest-of-HJL-patch.patch --]
[-- Type: text/x-patch; name="0005-Rest-of-HJL-patch.patch", Size: 11482 bytes --]

From 81db9e951ac27c9a7f94f88546d90c852a927d54 Mon Sep 17 00:00:00 2001
From: Richard Henderson <rth@redhat.com>
Date: Wed, 11 Feb 2015 22:16:38 -0800
Subject: [PATCH 5/5] Rest of HJL patch

---
 gcc/cgraphunit.c                          |  4 +++-
 gcc/testsuite/gcc.dg/visibility-22.c      | 17 +++++++++++++++++
 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                              |  5 +++--
 13 files changed, 171 insertions(+), 4 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 f2c40d4..057eedb 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.dg/visibility-22.c b/gcc/testsuite/gcc.dg/visibility-22.c
new file mode 100644
index 0000000..52f59be
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/visibility-22.c
@@ -0,0 +1,17 @@
+/* PR target/32219 */
+/* { dg-do run } */
+/* { dg-require-visibility "" } */
+/* { dg-options "-O2 -fPIC" { target fpic } } */
+/* This test requires support for undefined weak symbols.  This support
+   is not available on hppa*-*-hpux*.  The test is skipped rather than
+   xfailed to suppress the warning that would otherwise arise.  */
+/* { dg-skip-if "" { "hppa*-*-hpux*" "*-*-aix*" "*-*-darwin*" } "*" { "" } }  */
+
+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..0fa9ef4
--- /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 "-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;
+}
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 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/varasm.c b/gcc/varasm.c
index dea9b36..9f79416 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -7435,9 +7435,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);
 }
 
-- 
2.1.0