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 <guih...@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
>
