[PATCH, rs6000] Enable absolute jump table by default

[PATCH, rs6000] Enable absolute jump table by default

[HAO CHEN GUI]

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);
 }

 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)

Re: [PATCH, rs6000] Enable absolute jump table by default

[David Edelsohn]

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)

Re: [PATCH, rs6000] Enable absolute jump table by default

[HAO CHEN GUI]

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)

Re: [PATCH, rs6000] Enable absolute jump table by default

[David Edelsohn]

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