[v2,rs6000] Enable absolute jump table for PPC AIX and Linux

  Hi,
   This patch enables absolute jump tables on PPC AIX and Linux. For AIX, the jump
table is placed in data section. For Linux, it is placed in RELRO section when
relocation is needed.

   Bootstrapped and tested on AIX,Linux BE and LE with no regressions. Is this okay for trunk?
Any recommendations? Thanks a lot.

ChangeLog
2022-02-16 Haochen Gui <guihaoc@linux.ibm.com>

gcc/
	* config/rs6000/aix.h (JUMP_TABLES_IN_TEXT_SECTION): Define.
	* config/rs6000/linux64.h (JUMP_TABLES_IN_TEXT_SECTION): Likewise.
	* config/rs6000/rs6000.cc (rs6000_option_override_internal): Enable
	absolute jump tables for AIX and Linux.
	(rs6000_xcoff_function_rodata_section): Implement.
	* config/rs6000/xcoff.h (TARGET_ASM_FUNCTION_RODATA_SECTION): Define.

patch.diff
  

Hi Haochen,

Some minor comments are inlined.

on 2022/2/16 下午4:42, HAO CHEN GUI via Gcc-patches wrote:
> Hi,
>    This patch enables absolute jump tables on PPC AIX and Linux. For AIX, the jump
> table is placed in data section. For Linux, it is placed in RELRO section when
> relocation is needed.
> 
>    Bootstrapped and tested on AIX,Linux BE and LE with no regressions. Is this okay for trunk?
> Any recommendations? Thanks a lot.
> 

I may miss something, but there seem no test cases in power target testsuite to check expected
assembly for absolute and relative jump table.  If so, maybe it's better to add one or two?

> ChangeLog
> 2022-02-16 Haochen Gui <guihaoc@linux.ibm.com>
> 
> gcc/
> 	* config/rs6000/aix.h (JUMP_TABLES_IN_TEXT_SECTION): Define.
> 	* config/rs6000/linux64.h (JUMP_TABLES_IN_TEXT_SECTION): Likewise.
> 	* config/rs6000/rs6000.cc (rs6000_option_override_internal): Enable
> 	absolute jump tables for AIX and Linux.
> 	(rs6000_xcoff_function_rodata_section): Implement.
> 	* config/rs6000/xcoff.h (TARGET_ASM_FUNCTION_RODATA_SECTION): Define.
> 
> patch.diff
> diff --git a/gcc/config/rs6000/aix.h b/gcc/config/rs6000/aix.h
> index ad3238bf09a..b52208c2ee7 100644
> --- a/gcc/config/rs6000/aix.h
> +++ b/gcc/config/rs6000/aix.h
> @@ -253,7 +253,7 @@
> 
>  /* Indicate that jump tables go in the text section.  */
> 
> -#define JUMP_TABLES_IN_TEXT_SECTION 1
> +#define JUMP_TABLES_IN_TEXT_SECTION 0
> 
>  /* Define any extra SPECS that the compiler needs to generate.  */
>  #undef  SUBTARGET_EXTRA_SPECS
> diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h
> index b2a7afabc73..16df9ef167f 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.cc b/gcc/config/rs6000/rs6000.cc
> index bc3ef0721a4..e9c5552c082 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -4954,6 +4954,10 @@ rs6000_option_override_internal (bool global_init_p)
>      warning (0, "%qs is deprecated and not recommended in any circumstances",
>  	     "-mno-speculate-indirect-jumps");
> 
> +  /* Enable absolute jump tables for AIX and Linux.  */
> +  if (DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2)
> +    rs6000_relative_jumptables = 0;
> +
>    return ret;
>  }
> 
> @@ -28751,6 +28755,15 @@ constant_generates_xxspltidp (vec_const_128bit_type *vsx_const)
>    return sf_value;
>  }
> 
> +section * rs6000_xcoff_function_rodata_section (tree decl ATTRIBUTE_UNUSED,
> +						bool relocatable)
> +{
> +  if (relocatable)
> +    return data_section;
> +  else
> +    return readonly_data_section;
> +}
> +
>  

There is one area to put xcoff related functions and guarded with "#if TARGET_XCOFF",
it looks good to put this into? and could we make this function static?

Besides, it seems good to put some comments for this function to describe why we
choose to use the data_section.

>  struct gcc_target targetm = TARGET_INITIALIZER;
> 
> diff --git a/gcc/config/rs6000/xcoff.h b/gcc/config/rs6000/xcoff.h
> index cd0f99cb9c6..0dacd86eed9 100644
> --- a/gcc/config/rs6000/xcoff.h
> +++ b/gcc/config/rs6000/xcoff.h
> @@ -98,7 +98,7 @@
>  #define TARGET_ASM_SELECT_SECTION  rs6000_xcoff_select_section
>  #define TARGET_ASM_SELECT_RTX_SECTION  rs6000_xcoff_select_rtx_section
>  #define TARGET_ASM_UNIQUE_SECTION  rs6000_xcoff_unique_section
> -#define TARGET_ASM_FUNCTION_RODATA_SECTION default_no_function_rodata_section
> +#define TARGET_ASM_FUNCTION_RODATA_SECTION rs6000_xcoff_function_rodata_section


IIUC, the behavior of rs6000_xcoff_function_rodata_section isn't consistent with what we have
in the hook description.  If so, we need to update the hook description to align with this change.
Otherwise, some future codes might expect this hook only return readonly section (not possible
like data section) and get unexpected results.

BR,
Kewen
  

Kewen,
  Thanks so much for your advice.

On 21/2/2022 下午 5:42, Kewen.Lin wrote:
> Hi Haochen,
> 
> Some minor comments are inlined.
> 
> on 2022/2/16 下午4:42, HAO CHEN GUI via Gcc-patches wrote:
>> Hi,
>>    This patch enables absolute jump tables on PPC AIX and Linux. For AIX, the jump
>> table is placed in data section. For Linux, it is placed in RELRO section when
>> relocation is needed.
>>
>>    Bootstrapped and tested on AIX,Linux BE and LE with no regressions. Is this okay for trunk?
>> Any recommendations? Thanks a lot.
>>
> 
> I may miss something, but there seem no test cases in power target testsuite to check expected
> assembly for absolute and relative jump table.  If so, maybe it's better to add one or two?
I will add some testcases.

> 
>> ChangeLog
>> 2022-02-16 Haochen Gui <guihaoc@linux.ibm.com>
>>
>> gcc/
>> 	* config/rs6000/aix.h (JUMP_TABLES_IN_TEXT_SECTION): Define.
>> 	* config/rs6000/linux64.h (JUMP_TABLES_IN_TEXT_SECTION): Likewise.
>> 	* config/rs6000/rs6000.cc (rs6000_option_override_internal): Enable
>> 	absolute jump tables for AIX and Linux.
>> 	(rs6000_xcoff_function_rodata_section): Implement.
>> 	* config/rs6000/xcoff.h (TARGET_ASM_FUNCTION_RODATA_SECTION): Define.
>>
>> patch.diff
>> diff --git a/gcc/config/rs6000/aix.h b/gcc/config/rs6000/aix.h
>> index ad3238bf09a..b52208c2ee7 100644
>> --- a/gcc/config/rs6000/aix.h
>> +++ b/gcc/config/rs6000/aix.h
>> @@ -253,7 +253,7 @@
>>
>>  /* Indicate that jump tables go in the text section.  */
>>
>> -#define JUMP_TABLES_IN_TEXT_SECTION 1
>> +#define JUMP_TABLES_IN_TEXT_SECTION 0
>>
>>  /* Define any extra SPECS that the compiler needs to generate.  */
>>  #undef  SUBTARGET_EXTRA_SPECS
>> diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h
>> index b2a7afabc73..16df9ef167f 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.cc b/gcc/config/rs6000/rs6000.cc
>> index bc3ef0721a4..e9c5552c082 100644
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -4954,6 +4954,10 @@ rs6000_option_override_internal (bool global_init_p)
>>      warning (0, "%qs is deprecated and not recommended in any circumstances",
>>  	     "-mno-speculate-indirect-jumps");
>>
>> +  /* Enable absolute jump tables for AIX and Linux.  */
>> +  if (DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2)
>> +    rs6000_relative_jumptables = 0;
>> +
>>    return ret;
>>  }
>>
>> @@ -28751,6 +28755,15 @@ constant_generates_xxspltidp (vec_const_128bit_type *vsx_const)
>>    return sf_value;
>>  }
>>
>> +section * rs6000_xcoff_function_rodata_section (tree decl ATTRIBUTE_UNUSED,
>> +						bool relocatable)
>> +{
>> +  if (relocatable)
>> +    return data_section;
>> +  else
>> +    return readonly_data_section;
>> +}
>> +
>>  
> 
> There is one area to put xcoff related functions and guarded with "#if TARGET_XCOFF",
> it looks good to put this into? and could we make this function static?
From my understanding, the function should be only called by xcoff target. Of course,
it's safe with the guard. But we couldn't make the function static as it will be called
in other files.

> 
> Besides, it seems good to put some comments for this function to describe why we
> choose to use the data_section.
> 
>>  struct gcc_target targetm = TARGET_INITIALIZER;
>>
>> diff --git a/gcc/config/rs6000/xcoff.h b/gcc/config/rs6000/xcoff.h
>> index cd0f99cb9c6..0dacd86eed9 100644
>> --- a/gcc/config/rs6000/xcoff.h
>> +++ b/gcc/config/rs6000/xcoff.h
>> @@ -98,7 +98,7 @@
>>  #define TARGET_ASM_SELECT_SECTION  rs6000_xcoff_select_section
>>  #define TARGET_ASM_SELECT_RTX_SECTION  rs6000_xcoff_select_rtx_section
>>  #define TARGET_ASM_UNIQUE_SECTION  rs6000_xcoff_unique_section
>> -#define TARGET_ASM_FUNCTION_RODATA_SECTION default_no_function_rodata_section
>> +#define TARGET_ASM_FUNCTION_RODATA_SECTION rs6000_xcoff_function_rodata_section
> 
> 
> IIUC, the behavior of rs6000_xcoff_function_rodata_section isn't consistent with what we have
> in the hook description.  If so, we need to update the hook description to align with this change.
> Otherwise, some future codes might expect this hook only return readonly section (not possible
> like data section) and get unexpected results.
For the AIX/xcoff, the relocatable data section has to be the data section.
Though it's not read-only. I think it's consistent with the definition?

DEFHOOK
(function_rodata_section,
 "Return the readonly data or reloc readonly data section associated with\n\
@samp{DECL_SECTION_NAME (@var{decl})}. @var{relocatable} selects the latter\n\
over the former.\n\
The default version of this function selects @code{.gnu.linkonce.r.name} if\n\
the function's section is @code{.gnu.linkonce.t.name}, @code{.rodata.name}\n\
or @code{.data.rel.ro.name} if function is in @code{.text.name}, and\n\
the normal readonly-data or reloc readonly data section otherwise.",
 section *, (tree decl, bool relocatable),
 default_function_rodata_section)


> 
> BR,
> Kewen
>
  

on 2022/2/22 上午9:11, HAO CHEN GUI wrote:
> 
> Kewen,
>   Thanks so much for your advice.
> 
> On 21/2/2022 下午 5:42, Kewen.Lin wrote:
>> Hi Haochen,
>>
>> Some minor comments are inlined.
>>
>> on 2022/2/16 下午4:42, HAO CHEN GUI via Gcc-patches wrote:
>>> Hi,
>>>    This patch enables absolute jump tables on PPC AIX and Linux. For AIX, the jump
>>> table is placed in data section. For Linux, it is placed in RELRO section when
>>> relocation is needed.
>>>
>>>    Bootstrapped and tested on AIX,Linux BE and LE with no regressions. Is this okay for trunk?
>>> Any recommendations? Thanks a lot.
>>>
>>
>> I may miss something, but there seem no test cases in power target testsuite to check expected
>> assembly for absolute and relative jump table.  If so, maybe it's better to add one or two?
> I will add some testcases.
> 

Thanks!

>>
>>> ChangeLog
>>> 2022-02-16 Haochen Gui <guihaoc@linux.ibm.com>
>>>
>>> gcc/
>>> 	* config/rs6000/aix.h (JUMP_TABLES_IN_TEXT_SECTION): Define.
>>> 	* config/rs6000/linux64.h (JUMP_TABLES_IN_TEXT_SECTION): Likewise.
>>> 	* config/rs6000/rs6000.cc (rs6000_option_override_internal): Enable
>>> 	absolute jump tables for AIX and Linux.
>>> 	(rs6000_xcoff_function_rodata_section): Implement.
>>> 	* config/rs6000/xcoff.h (TARGET_ASM_FUNCTION_RODATA_SECTION): Define.
>>>
>>> patch.diff
>>> diff --git a/gcc/config/rs6000/aix.h b/gcc/config/rs6000/aix.h
>>> index ad3238bf09a..b52208c2ee7 100644
>>> --- a/gcc/config/rs6000/aix.h
>>> +++ b/gcc/config/rs6000/aix.h
>>> @@ -253,7 +253,7 @@
>>>
>>>  /* Indicate that jump tables go in the text section.  */
>>>
>>> -#define JUMP_TABLES_IN_TEXT_SECTION 1
>>> +#define JUMP_TABLES_IN_TEXT_SECTION 0
>>>
>>>  /* Define any extra SPECS that the compiler needs to generate.  */
>>>  #undef  SUBTARGET_EXTRA_SPECS
>>> diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h
>>> index b2a7afabc73..16df9ef167f 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.cc b/gcc/config/rs6000/rs6000.cc
>>> index bc3ef0721a4..e9c5552c082 100644
>>> --- a/gcc/config/rs6000/rs6000.cc
>>> +++ b/gcc/config/rs6000/rs6000.cc
>>> @@ -4954,6 +4954,10 @@ rs6000_option_override_internal (bool global_init_p)
>>>      warning (0, "%qs is deprecated and not recommended in any circumstances",
>>>  	     "-mno-speculate-indirect-jumps");
>>>
>>> +  /* Enable absolute jump tables for AIX and Linux.  */
>>> +  if (DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2)
>>> +    rs6000_relative_jumptables = 0;
>>> +
>>>    return ret;
>>>  }
>>>
>>> @@ -28751,6 +28755,15 @@ constant_generates_xxspltidp (vec_const_128bit_type *vsx_const)
>>>    return sf_value;
>>>  }
>>>
>>> +section * rs6000_xcoff_function_rodata_section (tree decl ATTRIBUTE_UNUSED,
>>> +						bool relocatable)
>>> +{
>>> +  if (relocatable)
>>> +    return data_section;
>>> +  else
>>> +    return readonly_data_section;
>>> +}
>>> +
>>>  
>>
>> There is one area to put xcoff related functions and guarded with "#if TARGET_XCOFF",
>> it looks good to put this into? and could we make this function static?
> From my understanding, the function should be only called by xcoff target. Of course,
> it's safe with the guard. But we couldn't make the function static as it will be called
> in other files.
> 

Yeah, that's why I suggested moving it.  Here the function is used for hook implementation,
just like some other existing hook functions, it would be used for function pointer
initialization (address taken), I think it's fine to make it static.

You can refer to some examples like: rs6000_xcoff_debug_unwind_info, etc...

>>
>> Besides, it seems good to put some comments for this function to describe why we
>> choose to use the data_section.
>>
>>>  struct gcc_target targetm = TARGET_INITIALIZER;
>>>
>>> diff --git a/gcc/config/rs6000/xcoff.h b/gcc/config/rs6000/xcoff.h
>>> index cd0f99cb9c6..0dacd86eed9 100644
>>> --- a/gcc/config/rs6000/xcoff.h
>>> +++ b/gcc/config/rs6000/xcoff.h
>>> @@ -98,7 +98,7 @@
>>>  #define TARGET_ASM_SELECT_SECTION  rs6000_xcoff_select_section
>>>  #define TARGET_ASM_SELECT_RTX_SECTION  rs6000_xcoff_select_rtx_section
>>>  #define TARGET_ASM_UNIQUE_SECTION  rs6000_xcoff_unique_section
>>> -#define TARGET_ASM_FUNCTION_RODATA_SECTION default_no_function_rodata_section
>>> +#define TARGET_ASM_FUNCTION_RODATA_SECTION rs6000_xcoff_function_rodata_section
>>
>>
>> IIUC, the behavior of rs6000_xcoff_function_rodata_section isn't consistent with what we have
>> in the hook description.  If so, we need to update the hook description to align with this change.
>> Otherwise, some future codes might expect this hook only return readonly section (not possible
>> like data section) and get unexpected results.
> For the AIX/xcoff, the relocatable data section has to be the data section.
> Though it's not read-only. I think it's consistent with the definition?
> 
> DEFHOOK
> (function_rodata_section,
>  "Return the readonly data or reloc readonly data section associated with\n\
> @samp{DECL_SECTION_NAME (@var{decl})}. @var{relocatable} selects the latter\n\
> over the former.\n\
> The default version of this function selects @code{.gnu.linkonce.r.name} if\n\
> the function's section is @code{.gnu.linkonce.t.name}, @code{.rodata.name}\n\
> or @code{.data.rel.ro.name} if function is in @code{.text.name}, and\n\
> the normal readonly-data or reloc readonly data section otherwise.",
>  section *, (tree decl, bool relocatable),
>  default_function_rodata_section)
> 
> 

Sorry, I still think it's inconsistent.  As you quoted, the description is
"Return the readonly data or reloc readonly data section", here it gives the users
the impression that it's for "readonly" data section.  And without your patch, I
think the previous default_no_function_rodata_section returns readonly_data_section
which also holds the "readonly" property.

With your patch, it can return the data section (r/w) which is NOT read-only.
Ideally, I would expect we can leave this hook alone for readonly purpose and it can
return NULL when some target doesn't have the expected section like the case you met
here.  The hook caller can decide whether just bypass this target which doesn't
support readonly section or switch to workaround with some other section like r/w data
for further processing.  IMHO, it seems more clear than putting one section which
isn't readonly but in one hook for readonly.

BR,
Kewen