| Message ID | 20190206061318.GA9247@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RS6000] Set uses_pic_offset_table on sysv secure-plt calls and tls. | expand |
On Wed, Feb 06, 2019 at 04:43:18PM +1030, Alan Modra wrote: > Segher, you'll recognize these as your patches from pr88343. All I've > done here is give you a testcase for the legitimize_tls_address change > (which you said you had no idea what the patch was for!) I don't see a testcase, and the rs6000_legitimize_tls_address part is new to me. Oh you mean the stuff in the commit message below here :-) I'll make real testcases from that, let's see if things are fixed. Do you know if it helps the glibs libm stuff? Another question below: > ---- > Fixes lack of r30 save/restore on powerpc-linux. > > // -m32 -fpic -ftls-model=initial-exec > __thread char* p; > char** f1 (void) { return &p; } > > and > > // -m32 -fpic -msecure-plt > extern int foo (int); > int f1 (int x) { return foo (x); } > > PR target/88343 > * config/rs6000/rs6000.c (rs6000_legitimize_tls_address), > (rs6000_call_sysv): Set uses_pic_offset_table. > > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > index 883361cabbe..ab01dee9b68 100644 > --- a/gcc/config/rs6000/rs6000.c > +++ b/gcc/config/rs6000/rs6000.c > @@ -8705,7 +8705,10 @@ rs6000_legitimize_tls_address (rtx addr, enum tls_model model) > else > { > if (flag_pic == 1) > - got = gen_rtx_REG (Pmode, RS6000_PIC_OFFSET_TABLE_REGNUM); > + { > + crtl->uses_pic_offset_table = 1; > + got = gen_rtx_REG (Pmode, RS6000_PIC_OFFSET_TABLE_REGNUM); This is the same as just got = pic_offset_table_rtx; or am I missing something? > + } > else > { > rtx gsym = rs6000_got_sym (); > @@ -38068,7 +38071,10 @@ rs6000_call_sysv (rtx value, rtx func_desc, rtx tlsarg, rtx cookie) > && (!SYMBOL_REF_LOCAL_P (func_addr) > || SYMBOL_REF_EXTERNAL_P (func_addr) > || SYMBOL_REF_WEAK (func_addr))) > - call[n++] = gen_rtx_USE (VOIDmode, pic_offset_table_rtx); > + { > + crtl->uses_pic_offset_table = 1; > + call[n++] = gen_rtx_USE (VOIDmode, pic_offset_table_rtx); > + } > > call[n++] = gen_hard_reg_clobber (Pmode, LR_REGNO); > Segher
> On 6 Feb 2019, at 11:45, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Wed, Feb 06, 2019 at 04:43:18PM +1030, Alan Modra wrote: >> Segher, you'll recognize these as your patches from pr88343. All I've >> done here is give you a testcase for the legitimize_tls_address change >> (which you said you had no idea what the patch was for!) > > I don't see a testcase, and the rs6000_legitimize_tls_address part is new > to me. Oh you mean the stuff in the commit message below here :-) > > I'll make real testcases from that, let's see if things are fixed. Do you > know if it helps the glibs libm stuff? I don’t know the answer to this ^ … … but was attempting a manual audit of other possible places and saw that rs6000_emit_toc_load_table() for pic code also uses the picbase without setting crtl->uses_pic_offset_table. However, that’s quite late on - and I wasn’t sure if something should be setting the use in that circumstance earlier on. Iain > > Another question below: > >> ---- >> Fixes lack of r30 save/restore on powerpc-linux. >> >> // -m32 -fpic -ftls-model=initial-exec >> __thread char* p; >> char** f1 (void) { return &p; } >> >> and >> >> // -m32 -fpic -msecure-plt >> extern int foo (int); >> int f1 (int x) { return foo (x); } >> >> PR target/88343 >> * config/rs6000/rs6000.c (rs6000_legitimize_tls_address), >> (rs6000_call_sysv): Set uses_pic_offset_table. >> >> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c >> index 883361cabbe..ab01dee9b68 100644 >> --- a/gcc/config/rs6000/rs6000.c >> +++ b/gcc/config/rs6000/rs6000.c >> @@ -8705,7 +8705,10 @@ rs6000_legitimize_tls_address (rtx addr, enum tls_model model) >> else >> { >> if (flag_pic == 1) >> - got = gen_rtx_REG (Pmode, RS6000_PIC_OFFSET_TABLE_REGNUM); >> + { >> + crtl->uses_pic_offset_table = 1; >> + got = gen_rtx_REG (Pmode, RS6000_PIC_OFFSET_TABLE_REGNUM); > > This is the same as just > > got = pic_offset_table_rtx; > > or am I missing something? > >> + } >> else >> { >> rtx gsym = rs6000_got_sym (); >> @@ -38068,7 +38071,10 @@ rs6000_call_sysv (rtx value, rtx func_desc, rtx tlsarg, rtx cookie) >> && (!SYMBOL_REF_LOCAL_P (func_addr) >> || SYMBOL_REF_EXTERNAL_P (func_addr) >> || SYMBOL_REF_WEAK (func_addr))) >> - call[n++] = gen_rtx_USE (VOIDmode, pic_offset_table_rtx); >> + { >> + crtl->uses_pic_offset_table = 1; >> + call[n++] = gen_rtx_USE (VOIDmode, pic_offset_table_rtx); >> + } >> >> call[n++] = gen_hard_reg_clobber (Pmode, LR_REGNO); >> > > > Segher
On Wed, Feb 06, 2019 at 05:45:17AM -0600, Segher Boessenkool wrote: > On Wed, Feb 06, 2019 at 04:43:18PM +1030, Alan Modra wrote: > > Segher, you'll recognize these as your patches from pr88343. All I've > > done here is give you a testcase for the legitimize_tls_address change > > (which you said you had no idea what the patch was for!) > > I don't see a testcase, and the rs6000_legitimize_tls_address part is new > to me. Oh you mean the stuff in the commit message below here :-) "New to me"... I did not remember it, anyway :-) Segher
On Wed, Feb 06, 2019 at 05:45:17AM -0600, Segher Boessenkool wrote: > On Wed, Feb 06, 2019 at 04:43:18PM +1030, Alan Modra wrote: > > Segher, you'll recognize these as your patches from pr88343. All I've > > done here is give you a testcase for the legitimize_tls_address change > > (which you said you had no idea what the patch was for!) > > I don't see a testcase, and the rs6000_legitimize_tls_address part is new > to me. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88343#c21 > Oh you mean the stuff in the commit message below here :-) Yes. > > I'll make real testcases from that, let's see if things are fixed. Do you > know if it helps the glibs libm stuff? The patch fixes the libm testcase that Joseph attached to the bug. I believe we might have more places where we need to set uses_pic_offset_table for Linux ABIs, builtin_setjmp_receiver and the use of RS6000_PIC_OFFSET_TABLE_REGNUM in rs6000_longcall_ref. (I'd looked at the latter case and decided it wasn't strictly necessary because we'd get use_pic_offset_table set when emitting the call, but that isn't true for old -mbss-plt code.) > > Another question below: > > > ---- > > Fixes lack of r30 save/restore on powerpc-linux. > > > > // -m32 -fpic -ftls-model=initial-exec > > __thread char* p; > > char** f1 (void) { return &p; } > > > > and > > > > // -m32 -fpic -msecure-plt > > extern int foo (int); > > int f1 (int x) { return foo (x); } > > > > PR target/88343 > > * config/rs6000/rs6000.c (rs6000_legitimize_tls_address), > > (rs6000_call_sysv): Set uses_pic_offset_table. > > > > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > > index 883361cabbe..ab01dee9b68 100644 > > --- a/gcc/config/rs6000/rs6000.c > > +++ b/gcc/config/rs6000/rs6000.c > > @@ -8705,7 +8705,10 @@ rs6000_legitimize_tls_address (rtx addr, enum tls_model model) > > else > > { > > if (flag_pic == 1) > > - got = gen_rtx_REG (Pmode, RS6000_PIC_OFFSET_TABLE_REGNUM); > > + { > > + crtl->uses_pic_offset_table = 1; > > + got = gen_rtx_REG (Pmode, RS6000_PIC_OFFSET_TABLE_REGNUM); > > This is the same as just > > got = pic_offset_table_rtx; Yes, I wondered about that too, then decided that it wasn't a stage 4 fix. It's true if rs6000_legitimize_tls_address isn't used by powerpc darwin. I believe you could use pic_offset_table_rtx for the TARGET_64BIT "got = gen_rtx_REG (Pmode, 2);" too. And if we're tidying the function it would be nice to use DEFAULT_ABI tests rather than some of the TARGET_64BIT tests. Register selection for local-exec model is an ABI thing, as is the GOT register choice.
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 883361cabbe..ab01dee9b68 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -8705,7 +8705,10 @@ rs6000_legitimize_tls_address (rtx addr, enum tls_model model) else { if (flag_pic == 1) - got = gen_rtx_REG (Pmode, RS6000_PIC_OFFSET_TABLE_REGNUM); + { + crtl->uses_pic_offset_table = 1; + got = gen_rtx_REG (Pmode, RS6000_PIC_OFFSET_TABLE_REGNUM); + } else { rtx gsym = rs6000_got_sym (); @@ -38068,7 +38071,10 @@ rs6000_call_sysv (rtx value, rtx func_desc, rtx tlsarg, rtx cookie) && (!SYMBOL_REF_LOCAL_P (func_addr) || SYMBOL_REF_EXTERNAL_P (func_addr) || SYMBOL_REF_WEAK (func_addr))) - call[n++] = gen_rtx_USE (VOIDmode, pic_offset_table_rtx); + { + crtl->uses_pic_offset_table = 1; + call[n++] = gen_rtx_USE (VOIDmode, pic_offset_table_rtx); + } call[n++] = gen_hard_reg_clobber (Pmode, LR_REGNO);
Segher, you'll recognize these as your patches from pr88343. All I've done here is give you a testcase for the legitimize_tls_address change (which you said you had no idea what the patch was for!) ---- Fixes lack of r30 save/restore on powerpc-linux. // -m32 -fpic -ftls-model=initial-exec __thread char* p; char** f1 (void) { return &p; } and // -m32 -fpic -msecure-plt extern int foo (int); int f1 (int x) { return foo (x); } PR target/88343 * config/rs6000/rs6000.c (rs6000_legitimize_tls_address), (rs6000_call_sysv): Set uses_pic_offset_table.