| Message ID | CAMe9rOpAnkfsJMXMM=MGNfqsCsLeBdNwmyCqLk6FbbrULbKigQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On February 5, 2015 12:29:40 AM GMT+01:00, "H.J. Lu" <hjl.tools@gmail.com> wrote: >On Wed, Feb 4, 2015 at 3:10 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Wed, Feb 4, 2015 at 2:47 PM, Bernhard Reutner-Fischer >> <rep.dot.nop@gmail.com> wrote: >>> On February 4, 2015 11:37:01 PM GMT+01:00, "H.J. Lu" ><hjl.tools@gmail.com> wrote: >>>>On Wed, Feb 4, 2015 at 1:53 PM, Sriraman Tallam ><tmsriram@google.com> >>>>wrote: >>>>> On Wed, Feb 4, 2015 at 10:57 AM, H.J. Lu <hjl.tools@gmail.com> >wrote: >>>>>> On Wed, Feb 4, 2015 at 10:51 AM, Sriraman Tallam >>>><tmsriram@google.com> wrote: >>>>>>> On Wed, Feb 4, 2015 at 10:45 AM, H.J. Lu <hjl.tools@gmail.com> >>>>wrote: >>>>>>>> On Wed, Feb 4, 2015 at 10:42 AM, Jakub Jelinek ><jakub@redhat.com> >>>>wrote: >>>>>>>>> On Wed, Feb 04, 2015 at 10:38:48AM -0800, H.J. Lu wrote: >>>>>>>>>> Common symbol should be resolved locally for PIE. >>>>>>>>> >>>>>>>>> binds_local_p yes, binds_to_current_def_p no. >>>>>>>>> >>>>>>>> >>>>>>>> Is SYMBOL_REF_LOCAL_P set to binds_local_p or >>>>>>>> binds_to_current_def_p? >>>>>>> >>>>>>> Looks like binds_local_p: >>>>>>> >>>>>>> varasm.c: >>>>>>> void >>>>>>> default_encode_section_info (tree decl, rtx rtl, int first >>>>ATTRIBUTE_UNUSED) >>>>>>> { >>>>>>> ... >>>>>>> if (targetm.binds_local_p (decl)) >>>>>>> flags |= SYMBOL_FLAG_LOCAL; >>>>>>> >>>>>> >>>>>> Why is SYMBOL_REF_LOCAL_P false? >>>>> >>>>> In varasm.c, default_binds_local_p_1 >>>>> >>>>> >>>>> /* 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) >>>> ^^^^^^^^^^^^^^^^^^^ >>>>Why is resolved_locally false? It should be true for common >>>>symbol when compiling for PIE. >>>> >>>>> local_p = false; >>>>> >>>>> For weak definition, it is set to false here. >>> >>> Yea and i think this is still wrong and known as >>> http://gcc.gnu.org/PR32219 >>> >> > >I am testing this patch. I cannot test it ATM, sorry. Please make sure to add the test case from the PR32219, comment13 https://gcc.gnu.org/bugzilla/attachment.cgi?id=27716&action=diff#gcc-4_7-branch/gcc/testsuite/gcc.dg/visibility-21.c_sec1 The PR33219 should be marked as 4.8, 4.9, 5.0 regression, too. Thanks for taking care of this one!
On 02/04/2015 03:29 PM, H.J. Lu wrote: > +++ b/gcc/varasm.c > @@ -6826,11 +6826,17 @@ default_binds_local_p_1 (const_tree exp, int shlib) > && (TREE_STATIC (exp) || DECL_EXTERNAL (exp))) > { > varpool_node *vnode = varpool_node::get (exp); > - if (vnode && (resolution_local_p (vnode->resolution) || vnode->in_other_partition)) > - resolved_locally = true; > - if (vnode > - && resolution_to_local_definition_p (vnode->resolution)) > - resolved_to_local_def = true; > + /* If not building shared library, common or initialized symbols > + are also resolved locally, regardless they are weak or not. */ > + if (vnode) > + { > + if ((!shlib && vnode->definition) > + || vnode->in_other_partition > + || resolution_local_p (vnode->resolution)) > + resolved_locally = true; > + if (resolution_to_local_definition_p (vnode->resolution)) > + resolved_to_local_def = true; > + } This is only true if the target uses COPY relocations, which is not universally true for all ELF targets. You can't just make this change here in varasm.c and change everyone. r~
On Thu, Feb 5, 2015 at 10:54 AM, Richard Henderson <rth@redhat.com> wrote: > On 02/04/2015 03:29 PM, H.J. Lu wrote: >> +++ b/gcc/varasm.c >> @@ -6826,11 +6826,17 @@ default_binds_local_p_1 (const_tree exp, int shlib) >> && (TREE_STATIC (exp) || DECL_EXTERNAL (exp))) >> { >> varpool_node *vnode = varpool_node::get (exp); >> - if (vnode && (resolution_local_p (vnode->resolution) || vnode->in_other_partition)) >> - resolved_locally = true; >> - if (vnode >> - && resolution_to_local_definition_p (vnode->resolution)) >> - resolved_to_local_def = true; >> + /* If not building shared library, common or initialized symbols >> + are also resolved locally, regardless they are weak or not. */ >> + if (vnode) >> + { >> + if ((!shlib && vnode->definition) >> + || vnode->in_other_partition >> + || resolution_local_p (vnode->resolution)) >> + resolved_locally = true; >> + if (resolution_to_local_definition_p (vnode->resolution)) >> + resolved_to_local_def = true; >> + } > > This is only true if the target uses COPY relocations, which is not universally > true for all ELF targets. > Can you elaborate why it depends on COPY relocation? There is no COPY relocation on x86-64.
On 02/05/2015 11:01 AM, H.J. Lu wrote: > Can you elaborate why it depends on COPY relocation? There > is no COPY relocation on x86-64. Ho hum, we appear to have switched topics mid-thread. I agree that we cannot override a weak symbol in the executable with even a non-weak symbol in a shared library. r~
On Thu, Feb 5, 2015 at 11:59 AM, Richard Henderson <rth@redhat.com> wrote: > On 02/05/2015 11:01 AM, H.J. Lu wrote: >> Can you elaborate why it depends on COPY relocation? There >> is no COPY relocation on x86-64. > > Ho hum, we appear to have switched topics mid-thread. > > I agree that we cannot override a weak symbol in the executable with even a > non-weak symbol in a shared library. Hi HJ, Is your patch supposed to fix weak symbols too? Will SYMBOL_REF_LOCAL_P evaluate to true for weak defined symbols with this patch? I tested this in gcc-4_9 and it didnt seem to do that. Thanks Sri > > > r~
On Thu, Feb 5, 2015 at 2:05 PM, Sriraman Tallam <tmsriram@google.com> wrote: > On Thu, Feb 5, 2015 at 11:59 AM, Richard Henderson <rth@redhat.com> wrote: >> On 02/05/2015 11:01 AM, H.J. Lu wrote: >>> Can you elaborate why it depends on COPY relocation? There >>> is no COPY relocation on x86-64. >> >> Ho hum, we appear to have switched topics mid-thread. >> >> I agree that we cannot override a weak symbol in the executable with even a >> non-weak symbol in a shared library. > > Hi HJ, > > Is your patch supposed to fix weak symbols too? Will > SYMBOL_REF_LOCAL_P evaluate to true for weak defined symbols with this > patch? I tested this in gcc-4_9 and it didnt seem to do that. I am working on a comprehensive patch. I will post it after testing is finished.
On Thu, Feb 5, 2015 at 2:23 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Thu, Feb 5, 2015 at 2:05 PM, Sriraman Tallam <tmsriram@google.com> wrote: >> On Thu, Feb 5, 2015 at 11:59 AM, Richard Henderson <rth@redhat.com> wrote: >>> On 02/05/2015 11:01 AM, H.J. Lu wrote: >>>> Can you elaborate why it depends on COPY relocation? There >>>> is no COPY relocation on x86-64. >>> >>> Ho hum, we appear to have switched topics mid-thread. >>> >>> I agree that we cannot override a weak symbol in the executable with even a >>> non-weak symbol in a shared library. >> >> Hi HJ, >> >> Is your patch supposed to fix weak symbols too? Will >> SYMBOL_REF_LOCAL_P evaluate to true for weak defined symbols with this >> patch? I tested this in gcc-4_9 and it didnt seem to do that. > > I am working on a comprehensive patch. I will post it > after testing is finished. Thanks! Sri > > -- > H.J. > -- > [hjl@gnu-6 copyreloc-3]$ cat initweak.i > __attribute__((weak)) > int xxxxxxxxxxxx = -1; > > int > foo () > { > return xxxxxxxxxxxx; > } > [hjl@gnu-6 copyreloc-3]$ cat commonweak.i > __attribute__((weak)) > int xxxxxxxxxxxx; > > int > foo () > { > return xxxxxxxxxxxx; > } > [hjl@gnu-6 copyreloc-3]$ make initweak.s commonweak.s > /export/build/gnu/gcc-x32/build-x86_64-linux/gcc/xgcc > -B/export/build/gnu/gcc-x32/build-x86_64-linux/gcc/ -pie -fpie -O3 > -fuse-ld=gold -S initweak.i > /export/build/gnu/gcc-x32/build-x86_64-linux/gcc/xgcc > -B/export/build/gnu/gcc-x32/build-x86_64-linux/gcc/ -pie -fpie -O3 > -fuse-ld=gold -S commonweak.i > [hjl@gnu-6 copyreloc-3]$ cat commonweak.s initweak.s > .file "commonweak.i" > .section .text.unlikely,"ax",@progbits > .LCOLDB0: > .text > .LHOTB0: > .p2align 4,,15 > .globl foo > .type foo, @function > foo: > .LFB0: > .cfi_startproc > movl xxxxxxxxxxxx(%rip), %eax > ret > .cfi_endproc > .LFE0: > .size foo, .-foo > .section .text.unlikely > .LCOLDE0: > .text > .LHOTE0: > .weak xxxxxxxxxxxx > .bss > .align 4 > .type xxxxxxxxxxxx, @object > .size xxxxxxxxxxxx, 4 > xxxxxxxxxxxx: > .zero 4 > .ident "GCC: (GNU) 5.0.0 20150205 (experimental)" > .section .note.GNU-stack,"",@progbits > .file "initweak.i" > .section .text.unlikely,"ax",@progbits > .LCOLDB0: > .text > .LHOTB0: > .p2align 4,,15 > .globl foo > .type foo, @function > foo: > .LFB0: > .cfi_startproc > movl xxxxxxxxxxxx(%rip), %eax > ret > .cfi_endproc > .LFE0: > .size foo, .-foo > .section .text.unlikely > .LCOLDE0: > .text > .LHOTE0: > .weak xxxxxxxxxxxx > .data > .align 4 > .type xxxxxxxxxxxx, @object > .size xxxxxxxxxxxx, 4 > xxxxxxxxxxxx: > .long -1 > .ident "GCC: (GNU) 5.0.0 20150205 (experimental)" > .section .note.GNU-stack,"",@progbits > [hjl@gnu-6 copyreloc-3]$
On Thu, Feb 5, 2015 at 2:05 PM, Sriraman Tallam <tmsriram@google.com> wrote: > On Thu, Feb 5, 2015 at 11:59 AM, Richard Henderson <rth@redhat.com> wrote: >> On 02/05/2015 11:01 AM, H.J. Lu wrote: >>> Can you elaborate why it depends on COPY relocation? There >>> is no COPY relocation on x86-64. >> >> Ho hum, we appear to have switched topics mid-thread. >> >> I agree that we cannot override a weak symbol in the executable with even a >> non-weak symbol in a shared library. > > Hi HJ, > > Is your patch supposed to fix weak symbols too? Will > SYMBOL_REF_LOCAL_P evaluate to true for weak defined symbols with this > patch? I tested this in gcc-4_9 and it didnt seem to do that. A patch is posted at https://gcc.gnu.org/ml/gcc-patches/2015-02/msg00410.html
diff --git a/gcc/varasm.c b/gcc/varasm.c index eb65b1f..36fd393 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -6826,11 +6826,17 @@ default_binds_local_p_1 (const_tree exp, int shlib) && (TREE_STATIC (exp) || DECL_EXTERNAL (exp))) { varpool_node *vnode = varpool_node::get (exp); - if (vnode && (resolution_local_p (vnode->resolution) || vnode->in_other_partition)) - resolved_locally = true; - if (vnode - && resolution_to_local_definition_p (vnode->resolution)) - resolved_to_local_def = true; + /* If not building shared library, common or initialized symbols + are also resolved locally, regardless they are weak or not. */ + if (vnode) + { + if ((!shlib && vnode->definition) + || vnode->in_other_partition + || resolution_local_p (vnode->resolution)) + resolved_locally = true; + if (resolution_to_local_definition_p (vnode->resolution)) + resolved_to_local_def = true; + } } else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp)) { @@ -6880,13 +6886,6 @@ default_binds_local_p_1 (const_tree exp, int shlib) symbols resolved from other modules. */ else if (shlib) local_p = false; - /* Uninitialized COMMON variable may be unified with symbols - resolved from other modules. */ - else if (DECL_COMMON (exp) - && !resolved_locally - && (DECL_INITIAL (exp) == NULL - || (!in_lto_p && DECL_INITIAL (exp) == error_mark_node))) - local_p = false; /* Otherwise we're left with initialized (or non-common) global data which is of necessity defined locally. */ else