Message ID | CAAs8HmzLve+KEJjxMVHrZVEVF+ZxrtJ4rO3Ov1jmBiXb2Jg55Q@mail.gmail.com |
---|---|
State | New |
Headers | show |
Hi Richard, I also ran the gcc testsuite with RUNTESTFLAGS="--tool_opts=-mcopyrelocs" to check for issues. The only test that failed was g++.dg/tsan/default_options.C. It uses -fpie -pie and BFD ld to link. Since BFD ld does not support copy relocations with -pie, it does not link. I linked with gold to make the test pass. Could you please take another look at this patch? Thanks Sri On Mon, Sep 8, 2014 at 3:19 PM, Sriraman Tallam <tmsriram@google.com> wrote: > On Tue, Sep 2, 2014 at 1:40 PM, Richard Henderson <rth@redhat.com> wrote: >> On 06/20/2014 05:17 PM, Sriraman Tallam wrote: >>> Index: config/i386/i386.c >>> =================================================================== >>> --- config/i386/i386.c (revision 211826) >>> +++ config/i386/i386.c (working copy) >>> @@ -12691,7 +12691,9 @@ legitimate_pic_address_disp_p (rtx disp) >>> return true; >>> } >>> else if (!SYMBOL_REF_FAR_ADDR_P (op0) >>> - && SYMBOL_REF_LOCAL_P (op0) >>> + && (SYMBOL_REF_LOCAL_P (op0) >>> + || (TARGET_64BIT && ix86_copyrelocs && flag_pie >>> + && !SYMBOL_REF_FUNCTION_P (op0))) >>> && ix86_cmodel != CM_LARGE_PIC) >>> return true; >>> break; >> >> This is the wrong place to patch. >> >> You ought to be adjusting SYMBOL_REF_LOCAL_P, by providing a modified >> TARGET_BINDS_LOCAL_P. > > I have done this in the new attached patch, I added a new function > i386_binds_local_p which will check for this and call > default_binds_local_p otherwise. > >> >> Note in particular that I believe that you are doing the wrong thing with weak >> and COMMON symbols, in that you probably ought not force a copy reloc there. > > I added an extra check to not do this for WEAK symbols. I also added a > check for DECL_EXTERNAL so I believe this will also not be called for > COMMON symbols. > >> >> Note the complexity of default_binds_local_p_1, and the fact that all you >> really want to modify is >> >> /* If PIC, then assume that any global name can be overridden by >> symbols resolved from other modules. */ >> else if (shlib) >> local_p = false; >> >> near the bottom of that function. > > I did not understand what you mean here? Were you suggesting an > alternative way of doing this? > > Thanks for reviewing > Sri > >> >> >> r~
Ping. On Fri, Sep 19, 2014 at 2:11 PM, Sriraman Tallam <tmsriram@google.com> wrote: > Hi Richard, > > I also ran the gcc testsuite with > RUNTESTFLAGS="--tool_opts=-mcopyrelocs" to check for issues. The only > test that failed was g++.dg/tsan/default_options.C. It uses -fpie > -pie and BFD ld to link. Since BFD ld does not support copy > relocations with -pie, it does not link. I linked with gold to make > the test pass. > > Could you please take another look at this patch? > > Thanks > Sri > > On Mon, Sep 8, 2014 at 3:19 PM, Sriraman Tallam <tmsriram@google.com> wrote: >> On Tue, Sep 2, 2014 at 1:40 PM, Richard Henderson <rth@redhat.com> wrote: >>> On 06/20/2014 05:17 PM, Sriraman Tallam wrote: >>>> Index: config/i386/i386.c >>>> =================================================================== >>>> --- config/i386/i386.c (revision 211826) >>>> +++ config/i386/i386.c (working copy) >>>> @@ -12691,7 +12691,9 @@ legitimate_pic_address_disp_p (rtx disp) >>>> return true; >>>> } >>>> else if (!SYMBOL_REF_FAR_ADDR_P (op0) >>>> - && SYMBOL_REF_LOCAL_P (op0) >>>> + && (SYMBOL_REF_LOCAL_P (op0) >>>> + || (TARGET_64BIT && ix86_copyrelocs && flag_pie >>>> + && !SYMBOL_REF_FUNCTION_P (op0))) >>>> && ix86_cmodel != CM_LARGE_PIC) >>>> return true; >>>> break; >>> >>> This is the wrong place to patch. >>> >>> You ought to be adjusting SYMBOL_REF_LOCAL_P, by providing a modified >>> TARGET_BINDS_LOCAL_P. >> >> I have done this in the new attached patch, I added a new function >> i386_binds_local_p which will check for this and call >> default_binds_local_p otherwise. >> >>> >>> Note in particular that I believe that you are doing the wrong thing with weak >>> and COMMON symbols, in that you probably ought not force a copy reloc there. >> >> I added an extra check to not do this for WEAK symbols. I also added a >> check for DECL_EXTERNAL so I believe this will also not be called for >> COMMON symbols. >> >>> >>> Note the complexity of default_binds_local_p_1, and the fact that all you >>> really want to modify is >>> >>> /* If PIC, then assume that any global name can be overridden by >>> symbols resolved from other modules. */ >>> else if (shlib) >>> local_p = false; >>> >>> near the bottom of that function. >> >> I did not understand what you mean here? Were you suggesting an >> alternative way of doing this? >> >> Thanks for reviewing >> Sri >> >>> >>> >>> r~
Ping. On Mon, Sep 29, 2014 at 10:57 AM, Sriraman Tallam <tmsriram@google.com> wrote: > Ping. > > On Fri, Sep 19, 2014 at 2:11 PM, Sriraman Tallam <tmsriram@google.com> wrote: >> Hi Richard, >> >> I also ran the gcc testsuite with >> RUNTESTFLAGS="--tool_opts=-mcopyrelocs" to check for issues. The only >> test that failed was g++.dg/tsan/default_options.C. It uses -fpie >> -pie and BFD ld to link. Since BFD ld does not support copy >> relocations with -pie, it does not link. I linked with gold to make >> the test pass. >> >> Could you please take another look at this patch? >> >> Thanks >> Sri >> >> On Mon, Sep 8, 2014 at 3:19 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>> On Tue, Sep 2, 2014 at 1:40 PM, Richard Henderson <rth@redhat.com> wrote: >>>> On 06/20/2014 05:17 PM, Sriraman Tallam wrote: >>>>> Index: config/i386/i386.c >>>>> =================================================================== >>>>> --- config/i386/i386.c (revision 211826) >>>>> +++ config/i386/i386.c (working copy) >>>>> @@ -12691,7 +12691,9 @@ legitimate_pic_address_disp_p (rtx disp) >>>>> return true; >>>>> } >>>>> else if (!SYMBOL_REF_FAR_ADDR_P (op0) >>>>> - && SYMBOL_REF_LOCAL_P (op0) >>>>> + && (SYMBOL_REF_LOCAL_P (op0) >>>>> + || (TARGET_64BIT && ix86_copyrelocs && flag_pie >>>>> + && !SYMBOL_REF_FUNCTION_P (op0))) >>>>> && ix86_cmodel != CM_LARGE_PIC) >>>>> return true; >>>>> break; >>>> >>>> This is the wrong place to patch. >>>> >>>> You ought to be adjusting SYMBOL_REF_LOCAL_P, by providing a modified >>>> TARGET_BINDS_LOCAL_P. >>> >>> I have done this in the new attached patch, I added a new function >>> i386_binds_local_p which will check for this and call >>> default_binds_local_p otherwise. >>> >>>> >>>> Note in particular that I believe that you are doing the wrong thing with weak >>>> and COMMON symbols, in that you probably ought not force a copy reloc there. >>> >>> I added an extra check to not do this for WEAK symbols. I also added a >>> check for DECL_EXTERNAL so I believe this will also not be called for >>> COMMON symbols. >>> >>>> >>>> Note the complexity of default_binds_local_p_1, and the fact that all you >>>> really want to modify is >>>> >>>> /* If PIC, then assume that any global name can be overridden by >>>> symbols resolved from other modules. */ >>>> else if (shlib) >>>> local_p = false; >>>> >>>> near the bottom of that function. >>> >>> I did not understand what you mean here? Were you suggesting an >>> alternative way of doing this? >>> >>> Thanks for reviewing >>> Sri >>> >>>> >>>> >>>> r~
Ping. On Mon, Oct 6, 2014 at 1:43 PM, Sriraman Tallam <tmsriram@google.com> wrote: > Ping. > > On Mon, Sep 29, 2014 at 10:57 AM, Sriraman Tallam <tmsriram@google.com> wrote: >> Ping. >> >> On Fri, Sep 19, 2014 at 2:11 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>> Hi Richard, >>> >>> I also ran the gcc testsuite with >>> RUNTESTFLAGS="--tool_opts=-mcopyrelocs" to check for issues. The only >>> test that failed was g++.dg/tsan/default_options.C. It uses -fpie >>> -pie and BFD ld to link. Since BFD ld does not support copy >>> relocations with -pie, it does not link. I linked with gold to make >>> the test pass. >>> >>> Could you please take another look at this patch? >>> >>> Thanks >>> Sri >>> >>> On Mon, Sep 8, 2014 at 3:19 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>>> On Tue, Sep 2, 2014 at 1:40 PM, Richard Henderson <rth@redhat.com> wrote: >>>>> On 06/20/2014 05:17 PM, Sriraman Tallam wrote: >>>>>> Index: config/i386/i386.c >>>>>> =================================================================== >>>>>> --- config/i386/i386.c (revision 211826) >>>>>> +++ config/i386/i386.c (working copy) >>>>>> @@ -12691,7 +12691,9 @@ legitimate_pic_address_disp_p (rtx disp) >>>>>> return true; >>>>>> } >>>>>> else if (!SYMBOL_REF_FAR_ADDR_P (op0) >>>>>> - && SYMBOL_REF_LOCAL_P (op0) >>>>>> + && (SYMBOL_REF_LOCAL_P (op0) >>>>>> + || (TARGET_64BIT && ix86_copyrelocs && flag_pie >>>>>> + && !SYMBOL_REF_FUNCTION_P (op0))) >>>>>> && ix86_cmodel != CM_LARGE_PIC) >>>>>> return true; >>>>>> break; >>>>> >>>>> This is the wrong place to patch. >>>>> >>>>> You ought to be adjusting SYMBOL_REF_LOCAL_P, by providing a modified >>>>> TARGET_BINDS_LOCAL_P. >>>> >>>> I have done this in the new attached patch, I added a new function >>>> i386_binds_local_p which will check for this and call >>>> default_binds_local_p otherwise. >>>> >>>>> >>>>> Note in particular that I believe that you are doing the wrong thing with weak >>>>> and COMMON symbols, in that you probably ought not force a copy reloc there. >>>> >>>> I added an extra check to not do this for WEAK symbols. I also added a >>>> check for DECL_EXTERNAL so I believe this will also not be called for >>>> COMMON symbols. >>>> >>>>> >>>>> Note the complexity of default_binds_local_p_1, and the fact that all you >>>>> really want to modify is >>>>> >>>>> /* If PIC, then assume that any global name can be overridden by >>>>> symbols resolved from other modules. */ >>>>> else if (shlib) >>>>> local_p = false; >>>>> >>>>> near the bottom of that function. >>>> >>>> I did not understand what you mean here? Were you suggesting an >>>> alternative way of doing this? >>>> >>>> Thanks for reviewing >>>> Sri >>>> >>>>> >>>>> >>>>> r~
Ping. On Mon, Nov 10, 2014 at 3:22 PM, Sriraman Tallam <tmsriram@google.com> wrote: > Ping. > > On Mon, Oct 6, 2014 at 1:43 PM, Sriraman Tallam <tmsriram@google.com> wrote: >> Ping. >> >> On Mon, Sep 29, 2014 at 10:57 AM, Sriraman Tallam <tmsriram@google.com> wrote: >>> Ping. >>> >>> On Fri, Sep 19, 2014 at 2:11 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>>> Hi Richard, >>>> >>>> I also ran the gcc testsuite with >>>> RUNTESTFLAGS="--tool_opts=-mcopyrelocs" to check for issues. The only >>>> test that failed was g++.dg/tsan/default_options.C. It uses -fpie >>>> -pie and BFD ld to link. Since BFD ld does not support copy >>>> relocations with -pie, it does not link. I linked with gold to make >>>> the test pass. >>>> >>>> Could you please take another look at this patch? >>>> >>>> Thanks >>>> Sri >>>> >>>> On Mon, Sep 8, 2014 at 3:19 PM, Sriraman Tallam <tmsriram@google.com> wrote: >>>>> On Tue, Sep 2, 2014 at 1:40 PM, Richard Henderson <rth@redhat.com> wrote: >>>>>> On 06/20/2014 05:17 PM, Sriraman Tallam wrote: >>>>>>> Index: config/i386/i386.c >>>>>>> =================================================================== >>>>>>> --- config/i386/i386.c (revision 211826) >>>>>>> +++ config/i386/i386.c (working copy) >>>>>>> @@ -12691,7 +12691,9 @@ legitimate_pic_address_disp_p (rtx disp) >>>>>>> return true; >>>>>>> } >>>>>>> else if (!SYMBOL_REF_FAR_ADDR_P (op0) >>>>>>> - && SYMBOL_REF_LOCAL_P (op0) >>>>>>> + && (SYMBOL_REF_LOCAL_P (op0) >>>>>>> + || (TARGET_64BIT && ix86_copyrelocs && flag_pie >>>>>>> + && !SYMBOL_REF_FUNCTION_P (op0))) >>>>>>> && ix86_cmodel != CM_LARGE_PIC) >>>>>>> return true; >>>>>>> break; >>>>>> >>>>>> This is the wrong place to patch. >>>>>> >>>>>> You ought to be adjusting SYMBOL_REF_LOCAL_P, by providing a modified >>>>>> TARGET_BINDS_LOCAL_P. >>>>> >>>>> I have done this in the new attached patch, I added a new function >>>>> i386_binds_local_p which will check for this and call >>>>> default_binds_local_p otherwise. >>>>> >>>>>> >>>>>> Note in particular that I believe that you are doing the wrong thing with weak >>>>>> and COMMON symbols, in that you probably ought not force a copy reloc there. >>>>> >>>>> I added an extra check to not do this for WEAK symbols. I also added a >>>>> check for DECL_EXTERNAL so I believe this will also not be called for >>>>> COMMON symbols. >>>>> >>>>>> >>>>>> Note the complexity of default_binds_local_p_1, and the fact that all you >>>>>> really want to modify is >>>>>> >>>>>> /* If PIC, then assume that any global name can be overridden by >>>>>> symbols resolved from other modules. */ >>>>>> else if (shlib) >>>>>> local_p = false; >>>>>> >>>>>> near the bottom of that function. >>>>> >>>>> I did not understand what you mean here? Were you suggesting an >>>>> alternative way of doing this? >>>>> >>>>> Thanks for reviewing >>>>> Sri >>>>> >>>>>> >>>>>> >>>>>> r~
On Mon, Sep 8, 2014 at 3:19 PM, Sriraman Tallam <tmsriram@google.com> wrote: > On Tue, Sep 2, 2014 at 1:40 PM, Richard Henderson <rth@redhat.com> wrote: >> On 06/20/2014 05:17 PM, Sriraman Tallam wrote: >>> Index: config/i386/i386.c >>> =================================================================== >>> --- config/i386/i386.c (revision 211826) >>> +++ config/i386/i386.c (working copy) >>> @@ -12691,7 +12691,9 @@ legitimate_pic_address_disp_p (rtx disp) >>> return true; >>> } >>> else if (!SYMBOL_REF_FAR_ADDR_P (op0) >>> - && SYMBOL_REF_LOCAL_P (op0) >>> + && (SYMBOL_REF_LOCAL_P (op0) >>> + || (TARGET_64BIT && ix86_copyrelocs && flag_pie >>> + && !SYMBOL_REF_FUNCTION_P (op0))) >>> && ix86_cmodel != CM_LARGE_PIC) >>> return true; >>> break; >> >> This is the wrong place to patch. >> >> You ought to be adjusting SYMBOL_REF_LOCAL_P, by providing a modified >> TARGET_BINDS_LOCAL_P. > > I have done this in the new attached patch, I added a new function > i386_binds_local_p which will check for this and call > default_binds_local_p otherwise. > >> >> Note in particular that I believe that you are doing the wrong thing with weak >> and COMMON symbols, in that you probably ought not force a copy reloc there. > > I added an extra check to not do this for WEAK symbols. I also added a > check for DECL_EXTERNAL so I believe this will also not be called for > COMMON symbols. > >> >> Note the complexity of default_binds_local_p_1, and the fact that all you >> really want to modify is >> >> /* If PIC, then assume that any global name can be overridden by >> symbols resolved from other modules. */ >> else if (shlib) >> local_p = false; >> >> near the bottom of that function. > > I did not understand what you mean here? Were you suggesting an > alternative way of doing this? > > Thanks for reviewing I'd like to see a few testcases: 1. One test to show it does the right thing for external variable. 2. One test to show it does the right thing for common symbol. 3. One test to show it does the right thing for weak symbol. 4. One test to show it does the right thing for external function. Thanks.
Index: testsuite/gcc.target/i386/pie-copyrelocs-2.c =================================================================== --- testsuite/gcc.target/i386/pie-copyrelocs-2.c (revision 0) +++ testsuite/gcc.target/i386/pie-copyrelocs-2.c (revision 0) @@ -0,0 +1,13 @@ +/* Test if -mno-copyrelocs does the right thing. */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fpie -mno-copyrelocs" } */ + +extern int glob_a; + +int foo () +{ + return glob_a; +} + +/* glob_a should always be accessed via GOT */ +/* { dg-final { scan-assembler "glob_a\\@GOT" { target { x86_64-*-* } } } } */ Index: testsuite/gcc.target/i386/pie-copyrelocs-1.c =================================================================== --- testsuite/gcc.target/i386/pie-copyrelocs-1.c (revision 0) +++ testsuite/gcc.target/i386/pie-copyrelocs-1.c (revision 0) @@ -0,0 +1,13 @@ +/* Test if -mcopyrelocs does the right thing. */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fpie -mcopyrelocs" } */ + +extern int glob_a; + +int foo () +{ + return glob_a; +} + +/* glob_a should never be accessed with a GOTPCREL */ +/* { dg-final { scan-assembler-not "glob_a\\@GOTPCREL" { target { x86_64-*-* } } } } */ Index: config/i386/i386.c =================================================================== --- config/i386/i386.c (revision 214973) +++ config/i386/i386.c (working copy) @@ -12642,6 +12642,18 @@ legitimate_pic_operand_p (rtx x) } } +bool +i386_binds_local_p (const_tree exp) +{ + /* Globals marked extern are treated as local when linker copy relocations + support is available with -f{pie|PIE}. */ + if (TARGET_64BIT && ix86_copyrelocs && flag_pie + && TREE_CODE (exp) == VAR_DECL + && DECL_EXTERNAL (exp) && !DECL_WEAK (exp)) + return true; + return default_binds_local_p (exp); +} + /* Determine if a given CONST RTX is a valid memory displacement in PIC mode. */ @@ -47157,6 +47169,9 @@ ix86_atomic_assign_expand_fenv (tree *hold, tree * #undef TARGET_MS_BITFIELD_LAYOUT_P #define TARGET_MS_BITFIELD_LAYOUT_P ix86_ms_bitfield_layout_p +#undef TARGET_BINDS_LOCAL_P +#define TARGET_BINDS_LOCAL_P i386_binds_local_p + #if TARGET_MACHO #undef TARGET_BINDS_LOCAL_P #define TARGET_BINDS_LOCAL_P darwin_binds_local_p Index: config/i386/i386.opt =================================================================== --- config/i386/i386.opt (revision 214973) +++ config/i386/i386.opt (working copy) @@ -108,6 +108,10 @@ int x_ix86_dump_tunes TargetSave int x_ix86_force_align_arg_pointer +;; -mcopyrelocs +TargetSave +int x_ix86_copyrelocs + ;; -mforce-drap= TargetSave int x_ix86_force_drap @@ -291,6 +295,10 @@ mfancy-math-387 Target RejectNegative Report InverseMask(NO_FANCY_MATH_387, USE_FANCY_MATH_387) Save Generate sin, cos, sqrt for FPU +mcopyrelocs +Target Report Var(ix86_copyrelocs) Init(0) +Use linker copy relocs for pie + mforce-drap Target Report Var(ix86_force_drap) Always use Dynamic Realigned Argument Pointer (DRAP) to realign stack