From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26320 invoked by alias); 7 Feb 2015 16:45:14 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 26306 invoked by uid 89); 7 Feb 2015 16:45:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.1 required=5.0 tests=AWL,BAYES_50,FREEMAIL_FROM,KAM_STOCKGEN,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=no version=3.3.2 X-HELO: mail-pa0-f50.google.com Received: from mail-pa0-f50.google.com (HELO mail-pa0-f50.google.com) (209.85.220.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Sat, 07 Feb 2015 16:45:11 +0000 Received: by mail-pa0-f50.google.com with SMTP id rd3so23882876pab.9 for ; Sat, 07 Feb 2015 08:45:09 -0800 (PST) X-Received: by 10.68.246.133 with SMTP id xw5mr14963088pbc.34.1423327509775; Sat, 07 Feb 2015 08:45:09 -0800 (PST) Received: from gnu-tools-1.localdomain (c-24-7-26-57.hsd1.ca.comcast.net. [24.7.26.57]) by mx.google.com with ESMTPSA id jc4sm11393941pbb.91.2015.02.07.08.45.08 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 07 Feb 2015 08:45:09 -0800 (PST) Received: by gnu-tools-1.localdomain (Postfix, from userid 1000) id F36A32008F; Sat, 7 Feb 2015 08:45:07 -0800 (PST) Date: Sat, 07 Feb 2015 16:45:00 -0000 From: "H.J. Lu" To: Jack Howarth , GCC Patches , Mike Stump , Iain Sandoe Subject: Re: [PATCH] PR rtl-optimization/32219: optimizer causes wrong code in pic/hidden/weak symbol checking Message-ID: <20150207164507.GA19402@gmail.com> References: <20150206162314.GA12597@intel.com> <20150207122739.GA25185@gmail.com> <20150207155606.GA14159@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150207155606.GA14159@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes X-SW-Source: 2015-02/txt/msg00469.txt.bz2 On Sat, Feb 07, 2015 at 07:56:06AM -0800, H.J. Lu wrote: > On Sat, Feb 07, 2015 at 10:11:01AM -0500, Jack Howarth wrote: > > H.J,, > > Unfortunately, the answer is yes. This patch still introduces > > regressions in the g++ test suite.l These are all some form of... > > > > It looks like Darwin depends on the old behavior of > default_binds_local_p_1. Please try this patch. > Here is an updated patch. H.J. >From d010cd1ddf866f8e10fe7ad2cf483b5a872bc6ea Mon Sep 17 00:00:00 2001 From: "H.J. Lu" Date: Thu, 5 Feb 2015 14:28:58 -0800 Subject: [PATCH] Handle symbol visibility/locality for PIE/PIC If a hidden weak symbol isn't defined in the TU, we can't assume it will be defined in another TU at link time. It makes a difference in code generation when compiling for PIC. If we assume that a hidden weak undefined symbol is local, the address checking may be optimized out and leads to the wrong code. This means that a symbol with user specified visibility is local only if it is locally resolved or defined, not weak or not compiling for PIC. When symbol visibility is specified in the source, we should always output symbol visibility even if symbol isn't local to the TU. If a global data symbol is defined in the TU, it is always local to the executable, regardless if it is a common symbol or not. If we aren't compiling for shared library, locally defined global data symbol binds locally. Since some targets call default_binds_local_p_1 directly and depend on the old behavior of default_binds_local_p_1. This patch renames default_binds_local_p_1 to default_binds_local_p_2 and implements the new behavior in default_binds_local_p_2 controlled by a variable. The old behavior remains with default_binds_local_p_1. gcc/ PR rtl-optimization/32219 * cgraphunit.c (varpool_node::finalize_decl): Set definition first before calling notice_global_symbol so that it is available to notice_global_symbol. * varasm.c (default_binds_local_p_1): Renamed to ... (default_binds_local_p_2): This. Resolve defined data symbol locally if not building shared library. Resolve symbol with user specified visibility locally only if it is locally resolved or defined, not weak or not compiling for PIC. (default_binds_local_p): Replace default_binds_local_p_1 with default_binds_local_p_2. (default_binds_local_p_1): Call default_binds_local_p_2. (default_elf_asm_output_external): Always output visibility specified in the source. gcc/testsuite/ PR rtl-optimization/32219 * gcc.dg/visibility-22.c: New test. * gcc.dg/visibility-23.c: Likewise. * gcc.target/i386/pr32219-1.c: Likewise. * gcc.target/i386/pr32219-2.c: Likewise. * gcc.target/i386/pr32219-3.c: Likewise. * gcc.target/i386/pr32219-4.c: Likewise. * gcc.target/i386/pr32219-5.c: Likewise. * gcc.target/i386/pr32219-6.c: Likewise. * gcc.target/i386/pr32219-7.c: Likewise. * gcc.target/i386/pr32219-8.c: Likewise. * gcc.target/i386/pr64317.c: Expect GOTOFF relocation instead of GOT relocation. --- gcc/cgraphunit.c | 4 +- gcc/testsuite/gcc.dg/visibility-22.c | 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 | 61 +++++++++++++++++++++---------- 13 files changed, 209 insertions(+), 22 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 35b244e..71367a3 100644 --- a/gcc/cgraphunit.c +++ b/gcc/cgraphunit.c @@ -792,8 +792,10 @@ varpool_node::finalize_decl (tree decl) if (node->definition) return; - notice_global_symbol (decl); + /* Set definition first before calling notice_global_symbol so that + it is available to notice_global_symbol. */ node->definition = true; + notice_global_symbol (decl); if (TREE_THIS_VOLATILE (decl) || DECL_PRESERVE_P (decl) /* Traditionally we do not eliminate static variables when not optimizing and when not doing toplevel reoder. */ diff --git a/gcc/testsuite/gcc.dg/visibility-22.c b/gcc/testsuite/gcc.dg/visibility-22.c new file mode 100644 index 0000000..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 eb65b1f..e4bc2c1 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) { bool local_p; bool resolved_locally = false; @@ -6826,11 +6817,18 @@ default_binds_local_p_1 (const_tree exp, int shlib) && (TREE_STATIC (exp) || DECL_EXTERNAL (exp))) { varpool_node *vnode = varpool_node::get (exp); - if (vnode && (resolution_local_p (vnode->resolution) || vnode->in_other_partition)) - resolved_locally = true; - if (vnode - && resolution_to_local_definition_p (vnode->resolution)) - resolved_to_local_def = true; + /* If not building shared library, common or initialized symbols + are also resolved locally, regardless they are weak or not if + weak_dominate is true. */ + if (vnode) + { + if ((weak_dominate && !shlib && vnode->definition) + || vnode->in_other_partition + || resolution_local_p (vnode->resolution)) + resolved_locally = true; + if (resolution_to_local_definition_p (vnode->resolution)) + resolved_to_local_def = true; + } } else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp)) { @@ -6859,9 +6857,15 @@ default_binds_local_p_1 (const_tree exp, int shlib) else if (! TREE_PUBLIC (exp)) local_p = true; /* A variable is local if the user has said explicitly that it will - be. */ + be and it is resolved or defined locally, not compiling for PIC, + not weak or weak_dominate is false. */ else if ((DECL_VISIBILITY_SPECIFIED (exp) || resolved_to_local_def) + && (!weak_dominate + || resolved_locally + || !flag_pic + || !DECL_EXTERNAL (exp) + || !DECL_WEAK (exp)) && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT) local_p = true; /* Variables defined outside this object might not be local. */ @@ -6881,8 +6885,9 @@ default_binds_local_p_1 (const_tree exp, int shlib) else if (shlib) local_p = false; /* Uninitialized COMMON variable may be unified with symbols - resolved from other modules. */ + resolved from other modules unless weak_dominate is true. */ else if (DECL_COMMON (exp) + && !weak_dominate && !resolved_locally && (DECL_INITIAL (exp) == NULL || (!in_lto_p && DECL_INITIAL (exp) == error_mark_node))) @@ -6895,6 +6900,21 @@ default_binds_local_p_1 (const_tree exp, int shlib) return local_p; } +/* 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. @@ -7445,9 +7465,10 @@ default_elf_asm_output_external (FILE *file ATTRIBUTE_UNUSED, { /* We output the name if and only if TREE_SYMBOL_REFERENCED is set in order to avoid putting out names that are never really - used. */ + used. Always output visibility specified in the source. */ if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)) - && targetm.binds_local_p (decl)) + && (DECL_VISIBILITY_SPECIFIED (decl) + || targetm.binds_local_p (decl))) maybe_assemble_visibility (decl); } -- 2.1.0