| Message ID | 7672daad-b86e-df92-bf9a-f3f4e489d5e3@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [rs6000] Enable absolute jump table by default | expand |
On Wed, Jan 12, 2022 at 7:22 AM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote: > > Hi, > This patch enables absolute jump table by default on rs6000. The relative jump tables are used when > it's explicit set by "rs6000_relative_jumptables", > or jump tables are placed in text section but global relocation is required. > > Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. Is this okay for trunk? > Any recommendations? Thanks a lot. > > ChangeLog > 2022-01-12 Haochen Gui <guihaoc@linux.ibm.com> > > gcc/ > * config/rs6000/linux64.h (JUMP_TABLES_IN_TEXT_SECTION): Define. > * config/rs6000/rs6000.c (rs6000_gen_pic_addr_diff_vec): Return > true when relative jump table is explicit required or jump tables have > to be put in text section but global relocation is also required. > * config/rs6000/rs6000.opt (rs6000_relative_jumptables): Disable. > > patch.diff > diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h > index d617f346f81..2e257c60f8c 100644 > --- a/gcc/config/rs6000/linux64.h > +++ b/gcc/config/rs6000/linux64.h > @@ -239,7 +239,7 @@ extern int dot_symbols; > > /* Indicate that jump tables go in the text section. */ > #undef JUMP_TABLES_IN_TEXT_SECTION > -#define JUMP_TABLES_IN_TEXT_SECTION TARGET_64BIT > +#define JUMP_TABLES_IN_TEXT_SECTION 0 > > /* The linux ppc64 ABI isn't explicit on whether aggregates smaller > than a doubleword should be padded upward or downward. You could > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > index 319182e94d9..9fba893a27a 100644 > --- a/gcc/config/rs6000/rs6000.c > +++ b/gcc/config/rs6000/rs6000.c > @@ -28465,7 +28465,9 @@ rs6000_emit_xxspltidp_v2df (rtx dst, long value) > static bool > rs6000_gen_pic_addr_diff_vec (void) > { > - return rs6000_relative_jumptables; > + return rs6000_relative_jumptables > + || (JUMP_TABLES_IN_TEXT_SECTION > + && targetm.asm_out.reloc_rw_mask () == 3); > } This seems like contorted logic and overriding the rs6000_relative_jumptables option change. The later part of the patch overrides rs6000_relative_jumptables for all rs6000 configurations, and then changes this one use of rs6000_relative_jumptables to add more logic to revert to the old meaning for some targets. What about all of the other uses of rs6000_relative_jumptables in the target? What about rs6000.md? I highly doubt that this patch is correct. Why not override rs6000_relative_jumptables for PPC64 Linux instead of changing its value globally and then trying to fix it up? Thanks, David > > void > diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt > index c2a77182a9e..75e3fa86829 100644 > --- a/gcc/config/rs6000/rs6000.opt > +++ b/gcc/config/rs6000/rs6000.opt > @@ -630,7 +630,7 @@ Target Mask(MMA) Var(rs6000_isa_flags) > Generate (do not generate) MMA instructions. > > mrelative-jumptables > -Target Undocumented Var(rs6000_relative_jumptables) Init(1) Save > +Target Undocumented Var(rs6000_relative_jumptables) Init(0) Save > > mrop-protect > Target Var(rs6000_rop_protect) Init(0)
Hi David, On 12/1/2022 下午 10:44, David Edelsohn wrote: > On Wed, Jan 12, 2022 at 7:22 AM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote: >> >> Hi, >> This patch enables absolute jump table by default on rs6000. The relative jump tables are used when >> it's explicit set by "rs6000_relative_jumptables", >> or jump tables are placed in text section but global relocation is required. >> >> Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. Is this okay for trunk? >> Any recommendations? Thanks a lot. >> >> ChangeLog >> 2022-01-12 Haochen Gui <guihaoc@linux.ibm.com> >> >> gcc/ >> * config/rs6000/linux64.h (JUMP_TABLES_IN_TEXT_SECTION): Define. >> * config/rs6000/rs6000.c (rs6000_gen_pic_addr_diff_vec): Return >> true when relative jump table is explicit required or jump tables have >> to be put in text section but global relocation is also required. >> * config/rs6000/rs6000.opt (rs6000_relative_jumptables): Disable. >> >> patch.diff >> diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h >> index d617f346f81..2e257c60f8c 100644 >> --- a/gcc/config/rs6000/linux64.h >> +++ b/gcc/config/rs6000/linux64.h >> @@ -239,7 +239,7 @@ extern int dot_symbols; >> >> /* Indicate that jump tables go in the text section. */ >> #undef JUMP_TABLES_IN_TEXT_SECTION >> -#define JUMP_TABLES_IN_TEXT_SECTION TARGET_64BIT >> +#define JUMP_TABLES_IN_TEXT_SECTION 0 >> >> /* The linux ppc64 ABI isn't explicit on whether aggregates smaller >> than a doubleword should be padded upward or downward. You could >> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c >> index 319182e94d9..9fba893a27a 100644 >> --- a/gcc/config/rs6000/rs6000.c >> +++ b/gcc/config/rs6000/rs6000.c >> @@ -28465,7 +28465,9 @@ rs6000_emit_xxspltidp_v2df (rtx dst, long value) >> static bool >> rs6000_gen_pic_addr_diff_vec (void) >> { >> - return rs6000_relative_jumptables; >> + return rs6000_relative_jumptables >> + || (JUMP_TABLES_IN_TEXT_SECTION >> + && targetm.asm_out.reloc_rw_mask () == 3); >> } > > This seems like contorted logic and overriding the > rs6000_relative_jumptables option change. The later part of the patch > overrides rs6000_relative_jumptables for all rs6000 configurations, > and then changes this one use of rs6000_relative_jumptables to add > more logic to revert to the old meaning for some targets. > > What about all of the other uses of rs6000_relative_jumptables in the > target? What about rs6000.md? > > I highly doubt that this patch is correct. > > Why not override rs6000_relative_jumptables for PPC64 Linux instead of > changing its value globally and then trying to fix it up? > > Thanks, David Thanks for your comments. In this patch, I tried to enable absolute jump table on all rs6000 targets. For PPC64 Linux, it supports RELRO section (e.g. .data.rel.ro.local) as "JUMP_TABLES_IN_TEXT_SECTION" is set to 0. So, absolute jump tables could be placed in RELRO section whatever global relocation is required or not. The absolute jump table can't be placed in text section when global relocation is required as text section is not writable. So for other rs6000 targets, absolute jump table can't be used if the target doesn't support RELRO and global relocation is required also. Looking forward to your advice. Thanks again. > >> >> void >> diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt >> index c2a77182a9e..75e3fa86829 100644 >> --- a/gcc/config/rs6000/rs6000.opt >> +++ b/gcc/config/rs6000/rs6000.opt >> @@ -630,7 +630,7 @@ Target Mask(MMA) Var(rs6000_isa_flags) >> Generate (do not generate) MMA instructions. >> >> mrelative-jumptables >> -Target Undocumented Var(rs6000_relative_jumptables) Init(1) Save >> +Target Undocumented Var(rs6000_relative_jumptables) Init(0) Save >> >> mrop-protect >> Target Var(rs6000_rop_protect) Init(0)
On Wed, Jan 12, 2022 at 8:40 PM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote: > > Hi David, > > On 12/1/2022 下午 10:44, David Edelsohn wrote: > > On Wed, Jan 12, 2022 at 7:22 AM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote: > >> > >> Hi, > >> This patch enables absolute jump table by default on rs6000. The relative jump tables are used when > >> it's explicit set by "rs6000_relative_jumptables", > >> or jump tables are placed in text section but global relocation is required. > >> > >> Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. Is this okay for trunk? > >> Any recommendations? Thanks a lot. > >> > >> ChangeLog > >> 2022-01-12 Haochen Gui <guihaoc@linux.ibm.com> > >> > >> gcc/ > >> * config/rs6000/linux64.h (JUMP_TABLES_IN_TEXT_SECTION): Define. > >> * config/rs6000/rs6000.c (rs6000_gen_pic_addr_diff_vec): Return > >> true when relative jump table is explicit required or jump tables have > >> to be put in text section but global relocation is also required. > >> * config/rs6000/rs6000.opt (rs6000_relative_jumptables): Disable. > >> > >> patch.diff > >> diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h > >> index d617f346f81..2e257c60f8c 100644 > >> --- a/gcc/config/rs6000/linux64.h > >> +++ b/gcc/config/rs6000/linux64.h > >> @@ -239,7 +239,7 @@ extern int dot_symbols; > >> > >> /* Indicate that jump tables go in the text section. */ > >> #undef JUMP_TABLES_IN_TEXT_SECTION > >> -#define JUMP_TABLES_IN_TEXT_SECTION TARGET_64BIT > >> +#define JUMP_TABLES_IN_TEXT_SECTION 0 > >> > >> /* The linux ppc64 ABI isn't explicit on whether aggregates smaller > >> than a doubleword should be padded upward or downward. You could > >> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > >> index 319182e94d9..9fba893a27a 100644 > >> --- a/gcc/config/rs6000/rs6000.c > >> +++ b/gcc/config/rs6000/rs6000.c > >> @@ -28465,7 +28465,9 @@ rs6000_emit_xxspltidp_v2df (rtx dst, long value) > >> static bool > >> rs6000_gen_pic_addr_diff_vec (void) > >> { > >> - return rs6000_relative_jumptables; > >> + return rs6000_relative_jumptables > >> + || (JUMP_TABLES_IN_TEXT_SECTION > >> + && targetm.asm_out.reloc_rw_mask () == 3); > >> } > > > > This seems like contorted logic and overriding the > > rs6000_relative_jumptables option change. The later part of the patch > > overrides rs6000_relative_jumptables for all rs6000 configurations, > > and then changes this one use of rs6000_relative_jumptables to add > > more logic to revert to the old meaning for some targets. > > > > What about all of the other uses of rs6000_relative_jumptables in the > > target? What about rs6000.md? > > > > I highly doubt that this patch is correct. > > > > Why not override rs6000_relative_jumptables for PPC64 Linux instead of > > changing its value globally and then trying to fix it up? > > > > Thanks, David > Thanks for your comments. > > In this patch, I tried to enable absolute jump table on all rs6000 targets. > For PPC64 Linux, it supports RELRO section (e.g. .data.rel.ro.local) as > "JUMP_TABLES_IN_TEXT_SECTION" is set to 0. So, absolute jump tables could be placed > in RELRO section whatever global relocation is required or not. The absolute jump > table can't be placed in text section when global relocation is required as text > section is not writable. So for other rs6000 targets, absolute jump table can't be > used if the target doesn't support RELRO and global relocation is required also. > > Looking forward to your advice. Thanks again. Why not override rs6000_relative_jumptables in rs6000_option_override_internal() for PPC64 Linux subtarget instead of changing the default value and then trying to fix the behavior for other configurations in rs6000_gen_pic_addr_diff_vec()? Or override it in SUBSUBTARGET_OVERRIDE_OPTIONS in linux64.h, or whichever header file(s) is appropriate for the subtarget variants. Your initial patch also changed rs6000_gen_pic_addr_diff_vec but didn't address the use of rs6000_relative_jumptables in the definition of CASE_VECTOR_MODE in rs6000.h. That cannot have been correct. At least without the change to the default value of rs6000_relative_jumptables you don't need to add kludges to all of the places where that variable is used for other subtarget variants of the rs6000 target. Thanks, David
diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h index d617f346f81..2e257c60f8c 100644 --- a/gcc/config/rs6000/linux64.h +++ b/gcc/config/rs6000/linux64.h @@ -239,7 +239,7 @@ extern int dot_symbols; /* Indicate that jump tables go in the text section. */ #undef JUMP_TABLES_IN_TEXT_SECTION -#define JUMP_TABLES_IN_TEXT_SECTION TARGET_64BIT +#define JUMP_TABLES_IN_TEXT_SECTION 0 /* The linux ppc64 ABI isn't explicit on whether aggregates smaller than a doubleword should be padded upward or downward. You could diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 319182e94d9..9fba893a27a 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -28465,7 +28465,9 @@ rs6000_emit_xxspltidp_v2df (rtx dst, long value) static bool rs6000_gen_pic_addr_diff_vec (void) { - return rs6000_relative_jumptables; + return rs6000_relative_jumptables + || (JUMP_TABLES_IN_TEXT_SECTION + && targetm.asm_out.reloc_rw_mask () == 3); } void diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt index c2a77182a9e..75e3fa86829 100644 --- a/gcc/config/rs6000/rs6000.opt +++ b/gcc/config/rs6000/rs6000.opt @@ -630,7 +630,7 @@ Target Mask(MMA) Var(rs6000_isa_flags) Generate (do not generate) MMA instructions. mrelative-jumptables -Target Undocumented Var(rs6000_relative_jumptables) Init(1) Save +Target Undocumented Var(rs6000_relative_jumptables) Init(0) Save mrop-protect Target Var(rs6000_rop_protect) Init(0)