| Message ID | b66686d7-6796-fcc3-b3b4-2a5f1c3ba197@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Put absolute address jump table in data.rel.ro.local if targets support relocations | expand |
Hi, Gentle ping this: https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553809.html Thanks Gui Haochen On 14/9/2020 上午 11:01, HAO CHEN GUI wrote: > Hi, > > Jump tables are put into text or rodata section originally. On some > platforms, it gains the performance benefit from absolute address jump > tables. So I want to let absolute address jump table be relocatable. > This patch puts absolute jump table in read only relocation section if > the target supports relocations. > > /* Judge if it's a absolute jump table. Set relocatable for > absolute jump table if the target supports relocations. */ > > if (!CASE_VECTOR_PC_RELATIVE > && !targetm.asm_out.generate_pic_addr_diff_vec ()) > relocatable = targetm.asm_out.reloc_rw_mask (); > > switch_to_section (targetm.asm_out.function_rodata_section > > (current_function_decl, > relocatable)); > > The attachments are the patch diff file and change log file. > > Bootstrapped and tested on powerpc64le-linux-gnu with no regressions. > Is this okay for trunk? Any recommendations? Thanks a lot. >
Sorry for the slow review. HAO CHEN GUI via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c > index 513fc5fe295..6f5bf8d7d73 100644 > --- a/gcc/config/mips/mips.c > +++ b/gcc/config/mips/mips.c > @@ -9315,10 +9315,10 @@ mips_select_rtx_section (machine_mode mode, rtx x, > default_function_rodata_section. */ > > static section * > -mips_function_rodata_section (tree decl) > +mips_function_rodata_section (tree decl, bool relocatable ATTRIBUTE_UNUSED) Now that we're C++, it's more idiomatic to leave off the parameter name: mips_function_rodata_section (tree decl, bool) Same for the rest of the patch. > @@ -2491,9 +2491,19 @@ final_scan_insn_1 (rtx_insn *insn, FILE *file, int optimize_p ATTRIBUTE_UNUSED, > if (! JUMP_TABLES_IN_TEXT_SECTION) > { > int log_align; > + bool relocatable; > + > + relocatable = 0; Very minor, but simpler as: bool relocatable = false; Same for the later hunk. > @@ -549,16 +549,17 @@ Whatever the actual target object format, this is often good enough.", > void, (tree decl, int reloc), > default_unique_section) > > -/* Return the readonly data section associated with function DECL. */ > +/* Return the readonly or relocated readonly data section > + associated with function DECL. */ > DEFHOOK > (function_rodata_section, > - "Return the readonly data section associated with\n\ > + "Return the readonly or reloc readonly data section associated with\n\ > @samp{DECL_SECTION_NAME (@var{decl})}.\n\ Maybe add “; @var{relocatable} selects the latter over the former.” > 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\ > -if function is in @code{.text.name}, and the normal readonly-data section\n\ > -otherwise.", > - section *, (tree decl), > +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) > > /* Nonnull if the target wants to override the default ".rodata" prefix > diff --git a/gcc/varasm.c b/gcc/varasm.c > index 4070f9c17e8..91ab75aed06 100644 > --- a/gcc/varasm.c > +++ b/gcc/varasm.c > @@ -726,12 +726,26 @@ switch_to_other_text_partition (void) > switch_to_section (current_function_section ()); > } > > -/* Return the read-only data section associated with function DECL. */ > +/* Return the read-only or relocated read-only data section > + associated with function DECL. */ > > section * > -default_function_rodata_section (tree decl) > +default_function_rodata_section (tree decl, bool relocatable) > { > - if (decl != NULL_TREE && DECL_SECTION_NAME (decl)) > + const char* sname; > + unsigned int flags; > + > + flags = 0; > + > + if (relocatable) > + { > + sname = ".data.rel.ro.local"; > + flags = (SECTION_WRITE | SECTION_RELRO); > + } > + else > + sname = ".rodata"; > + > + if (decl && DECL_SECTION_NAME (decl)) > { > const char *name = DECL_SECTION_NAME (decl); > > @@ -744,12 +758,12 @@ default_function_rodata_section (tree decl) > dot = strchr (name + 1, '.'); > if (!dot) > dot = name; > - len = strlen (dot) + 8; > + len = strlen (dot) + strlen (sname) + 1; > rname = (char *) alloca (len); > > - strcpy (rname, ".rodata"); > + strcpy (rname, sname); > strcat (rname, dot); > - return get_section (rname, SECTION_LINKONCE, decl); > + return get_section (rname, (SECTION_LINKONCE | flags), decl); > } > /* For .gnu.linkonce.t.foo we want to use .gnu.linkonce.r.foo. */ > else if (DECL_COMDAT_GROUP (decl) > @@ -767,15 +781,18 @@ default_function_rodata_section (tree decl) > && strncmp (name, ".text.", 6) == 0) > { > size_t len = strlen (name) + 1; > - char *rname = (char *) alloca (len + 2); > + char *rname = (char *) alloca (len + strlen (sname) - 5); > > - memcpy (rname, ".rodata", 7); > - memcpy (rname + 7, name + 5, len - 5); > - return get_section (rname, 0, decl); > + memcpy (rname, sname, strlen (sname)); > + memcpy (rname + strlen (sname), name + 5, len - 5); > + return get_section (rname, flags, decl); > } > } Don't we need to handle the .gnu.linkonce.t. case too? I believe the suffix there is “.d.rel.ro.local” (replacing “.t”) My main concern is how this interacts with non-ELF targets. It looks like AIX/XCOFF, Darwin and Cygwin already pick default_no_function_rodata_section, so they should be fine. But at the moment, all the fancy stuff in default_function_rodata_section is indirectly guarded by targetm_common.have_named_sections, with the hook falling back to readonly_data_section if the function isn't in a named section. So I wonder if it would be safer to test targetm_common.have_named_sections in this condition: > + > + /* Judge if it's a absolute jump table. Set relocatable for > + absolute jump table if the target supports relocations. */ > + > + if (!CASE_VECTOR_PC_RELATIVE > + && !targetm.asm_out.generate_pic_addr_diff_vec ()) > + relocatable = targetm.asm_out.reloc_rw_mask (); > > switch_to_section (targetm.asm_out.function_rodata_section > - (current_function_decl)); > + (current_function_decl, relocatable)); > > #ifdef ADDR_VEC_ALIGN > log_align = ADDR_VEC_ALIGN (table); Similarly for the other case where the condition appears. I think the condition is subtle enough that it's worth splitting out into a helper. Thanks, Richard
I had a wrong email setting and got your reply later. I modified the patch according to your advice. Could you please review it again? Thanks. On 2/10/2020 上午 1:47, Richard Sandiford wrote: > Sorry for the slow review. > > HAO CHEN GUI via Gcc-patches <gcc-patches@gcc.gnu.org> writes: >> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c >> index 513fc5fe295..6f5bf8d7d73 100644 >> --- a/gcc/config/mips/mips.c >> +++ b/gcc/config/mips/mips.c >> @@ -9315,10 +9315,10 @@ mips_select_rtx_section (machine_mode mode, rtx x, >> default_function_rodata_section. */ >> >> static section * >> -mips_function_rodata_section (tree decl) >> +mips_function_rodata_section (tree decl, bool relocatable ATTRIBUTE_UNUSED) > Now that we're C++, it's more idiomatic to leave off the parameter name: > > mips_function_rodata_section (tree decl, bool) > > Same for the rest of the patch. > >> @@ -2491,9 +2491,19 @@ final_scan_insn_1 (rtx_insn *insn, FILE *file, int optimize_p ATTRIBUTE_UNUSED, >> if (! JUMP_TABLES_IN_TEXT_SECTION) >> { >> int log_align; >> + bool relocatable; >> + >> + relocatable = 0; > Very minor, but simpler as: > > bool relocatable = false; > > Same for the later hunk. > >> @@ -549,16 +549,17 @@ Whatever the actual target object format, this is often good enough.", >> void, (tree decl, int reloc), >> default_unique_section) >> >> -/* Return the readonly data section associated with function DECL. */ >> +/* Return the readonly or relocated readonly data section >> + associated with function DECL. */ >> DEFHOOK >> (function_rodata_section, >> - "Return the readonly data section associated with\n\ >> + "Return the readonly or reloc readonly data section associated with\n\ >> @samp{DECL_SECTION_NAME (@var{decl})}.\n\ > Maybe add “; @var{relocatable} selects the latter over the former.” > >> 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\ >> -if function is in @code{.text.name}, and the normal readonly-data section\n\ >> -otherwise.", >> - section *, (tree decl), >> +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) >> >> /* Nonnull if the target wants to override the default ".rodata" prefix >> diff --git a/gcc/varasm.c b/gcc/varasm.c >> index 4070f9c17e8..91ab75aed06 100644 >> --- a/gcc/varasm.c >> +++ b/gcc/varasm.c >> @@ -726,12 +726,26 @@ switch_to_other_text_partition (void) >> switch_to_section (current_function_section ()); >> } >> >> -/* Return the read-only data section associated with function DECL. */ >> +/* Return the read-only or relocated read-only data section >> + associated with function DECL. */ >> >> section * >> -default_function_rodata_section (tree decl) >> +default_function_rodata_section (tree decl, bool relocatable) >> { >> - if (decl != NULL_TREE && DECL_SECTION_NAME (decl)) >> + const char* sname; >> + unsigned int flags; >> + >> + flags = 0; >> + >> + if (relocatable) >> + { >> + sname = ".data.rel.ro.local"; >> + flags = (SECTION_WRITE | SECTION_RELRO); >> + } >> + else >> + sname = ".rodata"; >> + >> + if (decl && DECL_SECTION_NAME (decl)) >> { >> const char *name = DECL_SECTION_NAME (decl); >> >> @@ -744,12 +758,12 @@ default_function_rodata_section (tree decl) >> dot = strchr (name + 1, '.'); >> if (!dot) >> dot = name; >> - len = strlen (dot) + 8; >> + len = strlen (dot) + strlen (sname) + 1; >> rname = (char *) alloca (len); >> >> - strcpy (rname, ".rodata"); >> + strcpy (rname, sname); >> strcat (rname, dot); >> - return get_section (rname, SECTION_LINKONCE, decl); >> + return get_section (rname, (SECTION_LINKONCE | flags), decl); >> } >> /* For .gnu.linkonce.t.foo we want to use .gnu.linkonce.r.foo. */ >> else if (DECL_COMDAT_GROUP (decl) >> @@ -767,15 +781,18 @@ default_function_rodata_section (tree decl) >> && strncmp (name, ".text.", 6) == 0) >> { >> size_t len = strlen (name) + 1; >> - char *rname = (char *) alloca (len + 2); >> + char *rname = (char *) alloca (len + strlen (sname) - 5); >> >> - memcpy (rname, ".rodata", 7); >> - memcpy (rname + 7, name + 5, len - 5); >> - return get_section (rname, 0, decl); >> + memcpy (rname, sname, strlen (sname)); >> + memcpy (rname + strlen (sname), name + 5, len - 5); >> + return get_section (rname, flags, decl); >> } >> } > Don't we need to handle the .gnu.linkonce.t. case too? I believe > the suffix there is “.d.rel.ro.local” (replacing “.t”) > > My main concern is how this interacts with non-ELF targets. > It looks like AIX/XCOFF, Darwin and Cygwin already pick > default_no_function_rodata_section, so they should be fine. > But at the moment, all the fancy stuff in default_function_rodata_section > is indirectly guarded by targetm_common.have_named_sections, with the > hook falling back to readonly_data_section if the function isn't in a > named section. > > So I wonder if it would be safer to test targetm_common.have_named_sections > in this condition: > >> + >> + /* Judge if it's a absolute jump table. Set relocatable for >> + absolute jump table if the target supports relocations. */ >> + >> + if (!CASE_VECTOR_PC_RELATIVE >> + && !targetm.asm_out.generate_pic_addr_diff_vec ()) >> + relocatable = targetm.asm_out.reloc_rw_mask (); >> >> switch_to_section (targetm.asm_out.function_rodata_section >> - (current_function_decl)); >> + (current_function_decl, relocatable)); >> >> #ifdef ADDR_VEC_ALIGN >> log_align = ADDR_VEC_ALIGN (table); > Similarly for the other case where the condition appears. I think the > condition is subtle enough that it's worth splitting out into a helper. > > Thanks, > Richard * final.c (final_scan_insn_1): Set jump table relocatable as the second argument of targetm.asm_out.function_rodata_section. * output.h (default_function_rodata_section, default_no_function_rodata_section): Add the second argument to the declarations. * target.def (function_rodata_section): Change the doc and add the second argument. * doc/tm.texi: Regenerate. * varasm.c (jumptable_relocatable): Implement. (default_function_rodata_section): Add the second argument and the support for relocatable read only sections. (default_no_function_rodata_section): Add the second argument. (function_mergeable_rodata_prefix): Set the second argument to false. * config/mips/mips.c (mips_function_rodata_section): Add the second arugment and set it to false. * config/s390/s390.c (targetm.asm_out.function_rodata_section): Set the second argument to false. * config/s390/s390.md Likewise. * gcc/config/rs6000/linux64.h (JUMP_TABLES_IN_TEXT_SECTION): Define. diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index 513fc5fe295..58e474e063d 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -9315,10 +9315,10 @@ mips_select_rtx_section (machine_mode mode, rtx x, default_function_rodata_section. */ static section * -mips_function_rodata_section (tree decl) +mips_function_rodata_section (tree decl, bool) { if (!TARGET_ABICALLS || TARGET_ABSOLUTE_ABICALLS || TARGET_GPWORD) - return default_function_rodata_section (decl); + return default_function_rodata_section (decl, false); if (decl && DECL_SECTION_NAME (decl)) { diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h index 73b6c01874c..35038570e04 100644 --- a/gcc/config/rs6000/linux64.h +++ b/gcc/config/rs6000/linux64.h @@ -241,7 +241,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 rs6000_relative_jumptables /* 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/s390/s390.c b/gcc/config/s390/s390.c index dbb541bbea7..86978648299 100644 --- a/gcc/config/s390/s390.c +++ b/gcc/config/s390/s390.c @@ -11697,7 +11697,7 @@ s390_output_split_stack_data (rtx parm_block, rtx call_done, rtx ops[] = { parm_block, call_done }; switch_to_section (targetm.asm_out.function_rodata_section - (current_function_decl)); + (current_function_decl, false)); if (TARGET_64BIT) output_asm_insn (".align\t8", NULL); diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md index 18edea1ce47..aaf41a559cc 100644 --- a/gcc/config/s390/s390.md +++ b/gcc/config/s390/s390.md @@ -11250,7 +11250,7 @@ "" { switch_to_section (targetm.asm_out.function_rodata_section - (current_function_decl)); + (current_function_decl, false)); return ""; } [(set_attr "length" "0")]) diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi index 97437e8274f..d4fbd750fc7 100644 --- a/gcc/doc/tm.texi +++ b/gcc/doc/tm.texi @@ -7711,13 +7711,14 @@ example, the function @code{foo} would be placed in @code{.text.foo}. Whatever the actual target object format, this is often good enough. @end deftypefn -@deftypefn {Target Hook} {section *} TARGET_ASM_FUNCTION_RODATA_SECTION (tree @var{decl}) -Return the readonly data section associated with -@samp{DECL_SECTION_NAME (@var{decl})}. +@deftypefn {Target Hook} {section *} TARGET_ASM_FUNCTION_RODATA_SECTION (tree @var{decl}, bool @var{relocatable}) +Return the readonly data or reloc readonly data section associated with +@samp{DECL_SECTION_NAME (@var{decl})}. @var{relocatable} selects the latter +over the former. The default version of this function selects @code{.gnu.linkonce.r.name} if the function's section is @code{.gnu.linkonce.t.name}, @code{.rodata.name} -if function is in @code{.text.name}, and the normal readonly-data section -otherwise. +or @code{.data.rel.ro.name} if function is in @code{.text.name}, and +the normal readonly-data or reloc readonly data section otherwise. @end deftypefn @deftypevr {Target Hook} {const char *} TARGET_ASM_MERGEABLE_RODATA_PREFIX diff --git a/gcc/final.c b/gcc/final.c index 80423d117d9..fc9a05e335f 100644 --- a/gcc/final.c +++ b/gcc/final.c @@ -81,6 +81,7 @@ along with GCC; see the file COPYING3. If not see #include "rtl-iter.h" #include "print-rtl.h" #include "function-abi.h" +#include "common/common-target.h" #ifdef XCOFF_DEBUGGING_INFO #include "xcoffout.h" /* Needed for external data declarations. */ @@ -2154,6 +2155,21 @@ asm_show_source (const char *filename, int linenum) fputc ('\n', asm_out_file); } +/* Judge if an absolute jump table is relocatable. */ + +bool +jumptable_relocatable (void) +{ + bool relocatable = false; + + if (!CASE_VECTOR_PC_RELATIVE + && !targetm.asm_out.generate_pic_addr_diff_vec () + && targetm_common.have_named_sections) + relocatable = targetm.asm_out.reloc_rw_mask (); + + return relocatable; +} + /* The final scan for one insn, INSN. Args are same as in `final', except that INSN is the insn being scanned. @@ -2493,7 +2509,8 @@ final_scan_insn_1 (rtx_insn *insn, FILE *file, int optimize_p ATTRIBUTE_UNUSED, int log_align; switch_to_section (targetm.asm_out.function_rodata_section - (current_function_decl)); + (current_function_decl, + jumptable_relocatable ())); #ifdef ADDR_VEC_ALIGN log_align = ADDR_VEC_ALIGN (table); @@ -2572,7 +2589,8 @@ final_scan_insn_1 (rtx_insn *insn, FILE *file, int optimize_p ATTRIBUTE_UNUSED, if (! JUMP_TABLES_IN_TEXT_SECTION) switch_to_section (targetm.asm_out.function_rodata_section - (current_function_decl)); + (current_function_decl, + jumptable_relocatable ())); else switch_to_section (current_function_section ()); diff --git a/gcc/output.h b/gcc/output.h index eb253c50329..661ce9074ae 100644 --- a/gcc/output.h +++ b/gcc/output.h @@ -571,8 +571,8 @@ extern void default_ctor_section_asm_out_constructor (rtx, int); extern section *default_select_section (tree, int, unsigned HOST_WIDE_INT); extern section *default_elf_select_section (tree, int, unsigned HOST_WIDE_INT); extern void default_unique_section (tree, int); -extern section *default_function_rodata_section (tree); -extern section *default_no_function_rodata_section (tree); +extern section *default_function_rodata_section (tree, bool); +extern section *default_no_function_rodata_section (tree, bool); extern section *default_clone_table_section (void); extern section *default_select_rtx_section (machine_mode, rtx, unsigned HOST_WIDE_INT); diff --git a/gcc/target.def b/gcc/target.def index ed2da154e30..d538cbbdf6b 100644 --- a/gcc/target.def +++ b/gcc/target.def @@ -549,16 +549,18 @@ Whatever the actual target object format, this is often good enough.", void, (tree decl, int reloc), default_unique_section) -/* Return the readonly data section associated with function DECL. */ +/* Return the readonly data or relocated readonly data section + associated with function DECL. */ DEFHOOK (function_rodata_section, - "Return the readonly data section associated with\n\ -@samp{DECL_SECTION_NAME (@var{decl})}.\n\ + "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\ -if function is in @code{.text.name}, and the normal readonly-data section\n\ -otherwise.", - section *, (tree decl), +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) /* Nonnull if the target wants to override the default ".rodata" prefix diff --git a/gcc/varasm.c b/gcc/varasm.c index ea0b59cf44a..40502049b61 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -727,12 +727,26 @@ switch_to_other_text_partition (void) switch_to_section (current_function_section ()); } -/* Return the read-only data section associated with function DECL. */ +/* Return the read-only or relocated read-only data section + associated with function DECL. */ section * -default_function_rodata_section (tree decl) +default_function_rodata_section (tree decl, bool relocatable) { - if (decl != NULL_TREE && DECL_SECTION_NAME (decl)) + const char* sname; + unsigned int flags; + + flags = 0; + + if (relocatable) + { + sname = ".data.rel.ro.local"; + flags = (SECTION_WRITE | SECTION_RELRO); + } + else + sname = ".rodata"; + + if (decl && DECL_SECTION_NAME (decl)) { const char *name = DECL_SECTION_NAME (decl); @@ -745,38 +759,57 @@ default_function_rodata_section (tree decl) dot = strchr (name + 1, '.'); if (!dot) dot = name; - len = strlen (dot) + 8; + len = strlen (dot) + strlen (sname) + 1; rname = (char *) alloca (len); - strcpy (rname, ".rodata"); + strcpy (rname, sname); strcat (rname, dot); - return get_section (rname, SECTION_LINKONCE, decl); + return get_section (rname, (SECTION_LINKONCE | flags), decl); } - /* For .gnu.linkonce.t.foo we want to use .gnu.linkonce.r.foo. */ + /* For .gnu.linkonce.t.foo we want to use .gnu.linkonce.r.foo or + .gnu.linkonce.d.rel.ro.local.foo if the jump table is relocatable. */ else if (DECL_COMDAT_GROUP (decl) && strncmp (name, ".gnu.linkonce.t.", 16) == 0) { - size_t len = strlen (name) + 1; - char *rname = (char *) alloca (len); + size_t len; + char *rname; - memcpy (rname, name, len); - rname[14] = 'r'; - return get_section (rname, SECTION_LINKONCE, decl); + if (relocatable) + { + len = strlen (name) + strlen (".rel.ro.local") + 1; + rname = (char *) alloca (len); + + strcpy (rname, ".gnu.linkonce.d"); + strcat (rname, ".rel.ro.local"); + strcat (rname, name + 15); + } + else + { + len = strlen (name) + 1; + rname = (char *) alloca (len); + + memcpy (rname, name, len); + rname[14] = 'r'; + } + return get_section (rname, (SECTION_LINKONCE | flags), decl); } /* For .text.foo we want to use .rodata.foo. */ else if (flag_function_sections && flag_data_sections && strncmp (name, ".text.", 6) == 0) { size_t len = strlen (name) + 1; - char *rname = (char *) alloca (len + 2); + char *rname = (char *) alloca (len + strlen (sname) - 5); - memcpy (rname, ".rodata", 7); - memcpy (rname + 7, name + 5, len - 5); - return get_section (rname, 0, decl); + memcpy (rname, sname, strlen (sname)); + memcpy (rname + strlen (sname), name + 5, len - 5); + return get_section (rname, flags, decl); } } - return readonly_data_section; + if (relocatable) + return get_section (sname, flags, decl); + else + return readonly_data_section; } /* Return the read-only data section associated with function DECL @@ -784,7 +817,7 @@ default_function_rodata_section (tree decl) readonly data section. */ section * -default_no_function_rodata_section (tree decl ATTRIBUTE_UNUSED) +default_no_function_rodata_section (tree, bool) { return readonly_data_section; } @@ -794,7 +827,8 @@ default_no_function_rodata_section (tree decl ATTRIBUTE_UNUSED) static const char * function_mergeable_rodata_prefix (void) { - section *s = targetm.asm_out.function_rodata_section (current_function_decl); + section *s = targetm.asm_out.function_rodata_section (current_function_decl, + false); if (SECTION_STYLE (s) == SECTION_NAMED) return s->named.name; else
Hi, Gentle ping this: https://gcc.gnu.org/pipermail/gcc-patches/2020-October/556744.html Thanks Gui Haochen On 22/10/2020 上午 10:53, HAO CHEN GUI wrote: > I had a wrong email setting and got your reply later. I modified the > patch according to your advice. Could you please review it again? Thanks. > > On 2/10/2020 上午 1:47, Richard Sandiford wrote: >> Sorry for the slow review. >> >> HAO CHEN GUI via Gcc-patches <gcc-patches@gcc.gnu.org> writes: >>> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c >>> index 513fc5fe295..6f5bf8d7d73 100644 >>> --- a/gcc/config/mips/mips.c >>> +++ b/gcc/config/mips/mips.c >>> @@ -9315,10 +9315,10 @@ mips_select_rtx_section (machine_mode mode, >>> rtx x, >>> default_function_rodata_section. */ >>> static section * >>> -mips_function_rodata_section (tree decl) >>> +mips_function_rodata_section (tree decl, bool relocatable >>> ATTRIBUTE_UNUSED) >> Now that we're C++, it's more idiomatic to leave off the parameter name: >> >> mips_function_rodata_section (tree decl, bool) >> >> Same for the rest of the patch. >> >>> @@ -2491,9 +2491,19 @@ final_scan_insn_1 (rtx_insn *insn, FILE >>> *file, int optimize_p ATTRIBUTE_UNUSED, >>> if (! JUMP_TABLES_IN_TEXT_SECTION) >>> { >>> int log_align; >>> + bool relocatable; >>> + >>> + relocatable = 0; >> Very minor, but simpler as: >> >> bool relocatable = false; >> >> Same for the later hunk. >> >>> @@ -549,16 +549,17 @@ Whatever the actual target object format, this >>> is often good enough.", >>> void, (tree decl, int reloc), >>> default_unique_section) >>> -/* Return the readonly data section associated with function >>> DECL. */ >>> +/* Return the readonly or relocated readonly data section >>> + associated with function DECL. */ >>> DEFHOOK >>> (function_rodata_section, >>> - "Return the readonly data section associated with\n\ >>> + "Return the readonly or reloc readonly data section associated >>> with\n\ >>> @samp{DECL_SECTION_NAME (@var{decl})}.\n\ >> Maybe add “; @var{relocatable} selects the latter over the former.” >> >>> 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\ >>> -if function is in @code{.text.name}, and the normal readonly-data >>> section\n\ >>> -otherwise.", >>> - section *, (tree decl), >>> +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) >>> /* Nonnull if the target wants to override the default ".rodata" >>> prefix >>> diff --git a/gcc/varasm.c b/gcc/varasm.c >>> index 4070f9c17e8..91ab75aed06 100644 >>> --- a/gcc/varasm.c >>> +++ b/gcc/varasm.c >>> @@ -726,12 +726,26 @@ switch_to_other_text_partition (void) >>> switch_to_section (current_function_section ()); >>> } >>> -/* Return the read-only data section associated with function >>> DECL. */ >>> +/* Return the read-only or relocated read-only data section >>> + associated with function DECL. */ >>> section * >>> -default_function_rodata_section (tree decl) >>> +default_function_rodata_section (tree decl, bool relocatable) >>> { >>> - if (decl != NULL_TREE && DECL_SECTION_NAME (decl)) >>> + const char* sname; >>> + unsigned int flags; >>> + >>> + flags = 0; >>> + >>> + if (relocatable) >>> + { >>> + sname = ".data.rel.ro.local"; >>> + flags = (SECTION_WRITE | SECTION_RELRO); >>> + } >>> + else >>> + sname = ".rodata"; >>> + >>> + if (decl && DECL_SECTION_NAME (decl)) >>> { >>> const char *name = DECL_SECTION_NAME (decl); >>> @@ -744,12 +758,12 @@ default_function_rodata_section (tree decl) >>> dot = strchr (name + 1, '.'); >>> if (!dot) >>> dot = name; >>> - len = strlen (dot) + 8; >>> + len = strlen (dot) + strlen (sname) + 1; >>> rname = (char *) alloca (len); >>> - strcpy (rname, ".rodata"); >>> + strcpy (rname, sname); >>> strcat (rname, dot); >>> - return get_section (rname, SECTION_LINKONCE, decl); >>> + return get_section (rname, (SECTION_LINKONCE | flags), decl); >>> } >>> /* For .gnu.linkonce.t.foo we want to use >>> .gnu.linkonce.r.foo. */ >>> else if (DECL_COMDAT_GROUP (decl) >>> @@ -767,15 +781,18 @@ default_function_rodata_section (tree decl) >>> && strncmp (name, ".text.", 6) == 0) >>> { >>> size_t len = strlen (name) + 1; >>> - char *rname = (char *) alloca (len + 2); >>> + char *rname = (char *) alloca (len + strlen (sname) - 5); >>> - memcpy (rname, ".rodata", 7); >>> - memcpy (rname + 7, name + 5, len - 5); >>> - return get_section (rname, 0, decl); >>> + memcpy (rname, sname, strlen (sname)); >>> + memcpy (rname + strlen (sname), name + 5, len - 5); >>> + return get_section (rname, flags, decl); >>> } >>> } >> Don't we need to handle the .gnu.linkonce.t. case too? I believe >> the suffix there is “.d.rel.ro.local” (replacing “.t”) >> >> My main concern is how this interacts with non-ELF targets. >> It looks like AIX/XCOFF, Darwin and Cygwin already pick >> default_no_function_rodata_section, so they should be fine. >> But at the moment, all the fancy stuff in >> default_function_rodata_section >> is indirectly guarded by targetm_common.have_named_sections, with the >> hook falling back to readonly_data_section if the function isn't in a >> named section. >> >> So I wonder if it would be safer to test >> targetm_common.have_named_sections >> in this condition: >> >>> + >>> + /* Judge if it's a absolute jump table. Set relocatable for >>> + absolute jump table if the target supports relocations. */ >>> + >>> + if (!CASE_VECTOR_PC_RELATIVE >>> + && !targetm.asm_out.generate_pic_addr_diff_vec ()) >>> + relocatable = targetm.asm_out.reloc_rw_mask (); >>> switch_to_section >>> (targetm.asm_out.function_rodata_section >>> - (current_function_decl)); >>> + (current_function_decl, relocatable)); >>> #ifdef ADDR_VEC_ALIGN >>> log_align = ADDR_VEC_ALIGN (table); >> Similarly for the other case where the condition appears. I think the >> condition is subtle enough that it's worth splitting out into a helper. >> >> Thanks, >> Richard
Hi, Sorry for the slow reply. Just one minor nit: HAO CHEN GUI <guihaoc@linux.ibm.com> writes: > diff --git a/gcc/varasm.c b/gcc/varasm.c > index ea0b59cf44a..40502049b61 100644 > --- a/gcc/varasm.c > +++ b/gcc/varasm.c > @@ -727,12 +727,26 @@ switch_to_other_text_partition (void) > switch_to_section (current_function_section ()); > } > > -/* Return the read-only data section associated with function DECL. */ > +/* Return the read-only or relocated read-only data section > + associated with function DECL. */ > > section * > -default_function_rodata_section (tree decl) > +default_function_rodata_section (tree decl, bool relocatable) > { > - if (decl != NULL_TREE && DECL_SECTION_NAME (decl)) > + const char* sname; > + unsigned int flags; > + > + flags = 0; > + > + if (relocatable) > + { > + sname = ".data.rel.ro.local"; > + flags = (SECTION_WRITE | SECTION_RELRO); > + } > + else > + sname = ".rodata"; > + > + if (decl && DECL_SECTION_NAME (decl)) > { > const char *name = DECL_SECTION_NAME (decl); > > @@ -745,38 +759,57 @@ default_function_rodata_section (tree decl) > dot = strchr (name + 1, '.'); > if (!dot) > dot = name; > - len = strlen (dot) + 8; > + len = strlen (dot) + strlen (sname) + 1; > rname = (char *) alloca (len); > > - strcpy (rname, ".rodata"); > + strcpy (rname, sname); > strcat (rname, dot); > - return get_section (rname, SECTION_LINKONCE, decl); > + return get_section (rname, (SECTION_LINKONCE | flags), decl); > } > - /* For .gnu.linkonce.t.foo we want to use .gnu.linkonce.r.foo. */ > + /* For .gnu.linkonce.t.foo we want to use .gnu.linkonce.r.foo or > + .gnu.linkonce.d.rel.ro.local.foo if the jump table is relocatable. */ > else if (DECL_COMDAT_GROUP (decl) > && strncmp (name, ".gnu.linkonce.t.", 16) == 0) > { > - size_t len = strlen (name) + 1; > - char *rname = (char *) alloca (len); > + size_t len; > + char *rname; > > - memcpy (rname, name, len); > - rname[14] = 'r'; > - return get_section (rname, SECTION_LINKONCE, decl); > + if (relocatable) > + { > + len = strlen (name) + strlen (".rel.ro.local") + 1; > + rname = (char *) alloca (len); > + > + strcpy (rname, ".gnu.linkonce.d"); > + strcat (rname, ".rel.ro.local"); > + strcat (rname, name + 15); I realise you probably wrote it like this to make the correlation between the length calculation and the string operations more obvious, but IMO it would be less surprising to have: strcpy (rname, ".gnu.linkonce.d.rel.ro.local"); OK with that change, thanks. Richard
Hi， I just tweaked the patch according to your advice and committed it. Thanks so much for your help and advice. Haochen Gui On 13/11/2020 下午 5:27, Richard Sandiford wrote: > Hi, > > Sorry for the slow reply. Just one minor nit: > > HAO CHEN GUI <guihaoc@linux.ibm.com> writes: >> diff --git a/gcc/varasm.c b/gcc/varasm.c >> index ea0b59cf44a..40502049b61 100644 >> --- a/gcc/varasm.c >> +++ b/gcc/varasm.c >> @@ -727,12 +727,26 @@ switch_to_other_text_partition (void) >> switch_to_section (current_function_section ()); >> } >> >> -/* Return the read-only data section associated with function DECL. */ >> +/* Return the read-only or relocated read-only data section >> + associated with function DECL. */ >> >> section * >> -default_function_rodata_section (tree decl) >> +default_function_rodata_section (tree decl, bool relocatable) >> { >> - if (decl != NULL_TREE && DECL_SECTION_NAME (decl)) >> + const char* sname; >> + unsigned int flags; >> + >> + flags = 0; >> + >> + if (relocatable) >> + { >> + sname = ".data.rel.ro.local"; >> + flags = (SECTION_WRITE | SECTION_RELRO); >> + } >> + else >> + sname = ".rodata"; >> + >> + if (decl && DECL_SECTION_NAME (decl)) >> { >> const char *name = DECL_SECTION_NAME (decl); >> >> @@ -745,38 +759,57 @@ default_function_rodata_section (tree decl) >> dot = strchr (name + 1, '.'); >> if (!dot) >> dot = name; >> - len = strlen (dot) + 8; >> + len = strlen (dot) + strlen (sname) + 1; >> rname = (char *) alloca (len); >> >> - strcpy (rname, ".rodata"); >> + strcpy (rname, sname); >> strcat (rname, dot); >> - return get_section (rname, SECTION_LINKONCE, decl); >> + return get_section (rname, (SECTION_LINKONCE | flags), decl); >> } >> - /* For .gnu.linkonce.t.foo we want to use .gnu.linkonce.r.foo. */ >> + /* For .gnu.linkonce.t.foo we want to use .gnu.linkonce.r.foo or >> + .gnu.linkonce.d.rel.ro.local.foo if the jump table is relocatable. */ >> else if (DECL_COMDAT_GROUP (decl) >> && strncmp (name, ".gnu.linkonce.t.", 16) == 0) >> { >> - size_t len = strlen (name) + 1; >> - char *rname = (char *) alloca (len); >> + size_t len; >> + char *rname; >> >> - memcpy (rname, name, len); >> - rname[14] = 'r'; >> - return get_section (rname, SECTION_LINKONCE, decl); >> + if (relocatable) >> + { >> + len = strlen (name) + strlen (".rel.ro.local") + 1; >> + rname = (char *) alloca (len); >> + >> + strcpy (rname, ".gnu.linkonce.d"); >> + strcat (rname, ".rel.ro.local"); >> + strcat (rname, name + 15); > I realise you probably wrote it like this to make the correlation > between the length calculation and the string operations more > obvious, but IMO it would be less surprising to have: > > strcpy (rname, ".gnu.linkonce.d.rel.ro.local"); > > OK with that change, thanks. > > Richard
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index 513fc5fe295..6f5bf8d7d73 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -9315,10 +9315,10 @@ mips_select_rtx_section (machine_mode mode, rtx x, default_function_rodata_section. */ static section * -mips_function_rodata_section (tree decl) +mips_function_rodata_section (tree decl, bool relocatable ATTRIBUTE_UNUSED) { if (!TARGET_ABICALLS || TARGET_ABSOLUTE_ABICALLS || TARGET_GPWORD) - return default_function_rodata_section (decl); + return default_function_rodata_section (decl, false); if (decl && DECL_SECTION_NAME (decl)) { diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h index 34776c8421e..29d2d7ce40b 100644 --- a/gcc/config/rs6000/linux64.h +++ b/gcc/config/rs6000/linux64.h @@ -324,7 +324,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 rs6000_relative_jumptables /* 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/s390/s390.c b/gcc/config/s390/s390.c index 758315c0c72..eb619d39ad7 100644 --- a/gcc/config/s390/s390.c +++ b/gcc/config/s390/s390.c @@ -11639,7 +11639,7 @@ s390_output_split_stack_data (rtx parm_block, rtx call_done, rtx ops[] = { parm_block, call_done }; switch_to_section (targetm.asm_out.function_rodata_section - (current_function_decl)); + (current_function_decl, false)); if (TARGET_64BIT) output_asm_insn (".align\t8", NULL); diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md index cd1c0634b71..6ca03ed9002 100644 --- a/gcc/config/s390/s390.md +++ b/gcc/config/s390/s390.md @@ -11218,7 +11218,7 @@ "" { switch_to_section (targetm.asm_out.function_rodata_section - (current_function_decl)); + (current_function_decl, false)); return ""; } [(set_attr "length" "0")]) diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi index 6e7d9dc54a9..3907b7b8cc7 100644 --- a/gcc/doc/tm.texi +++ b/gcc/doc/tm.texi @@ -7703,13 +7703,13 @@ example, the function @code{foo} would be placed in @code{.text.foo}. Whatever the actual target object format, this is often good enough. @end deftypefn -@deftypefn {Target Hook} {section *} TARGET_ASM_FUNCTION_RODATA_SECTION (tree @var{decl}) -Return the readonly data section associated with +@deftypefn {Target Hook} {section *} TARGET_ASM_FUNCTION_RODATA_SECTION (tree @var{decl}, bool @var{relocatable}) +Return the readonly or reloc readonly data section associated with @samp{DECL_SECTION_NAME (@var{decl})}. The default version of this function selects @code{.gnu.linkonce.r.name} if the function's section is @code{.gnu.linkonce.t.name}, @code{.rodata.name} -if function is in @code{.text.name}, and the normal readonly-data section -otherwise. +or @code{.data.rel.ro.name} if function is in @code{.text.name}, and +the normal readonly-data or reloc readonly data section otherwise. @end deftypefn @deftypevr {Target Hook} {const char *} TARGET_ASM_MERGEABLE_RODATA_PREFIX diff --git a/gcc/final.c b/gcc/final.c index a3601964a8d..c86e639dc23 100644 --- a/gcc/final.c +++ b/gcc/final.c @@ -2491,9 +2491,19 @@ final_scan_insn_1 (rtx_insn *insn, FILE *file, int optimize_p ATTRIBUTE_UNUSED, if (! JUMP_TABLES_IN_TEXT_SECTION) { int log_align; + bool relocatable; + + relocatable = 0; + + /* Judge if it's a absolute jump table. Set relocatable for + absolute jump table if the target supports relocations. */ + + if (!CASE_VECTOR_PC_RELATIVE + && !targetm.asm_out.generate_pic_addr_diff_vec ()) + relocatable = targetm.asm_out.reloc_rw_mask (); switch_to_section (targetm.asm_out.function_rodata_section - (current_function_decl)); + (current_function_decl, relocatable)); #ifdef ADDR_VEC_ALIGN log_align = ADDR_VEC_ALIGN (table); @@ -2571,8 +2581,21 @@ final_scan_insn_1 (rtx_insn *insn, FILE *file, int optimize_p ATTRIBUTE_UNUSED, #endif if (! JUMP_TABLES_IN_TEXT_SECTION) - switch_to_section (targetm.asm_out.function_rodata_section - (current_function_decl)); + { + bool relocatable; + + relocatable = 0; + + /* Judge if it's a absolute jump table. Set relocatable for + absolute jump table if the target supports relocations. */ + + if (!CASE_VECTOR_PC_RELATIVE + && !targetm.asm_out.generate_pic_addr_diff_vec ()) + relocatable = targetm.asm_out.reloc_rw_mask (); + + switch_to_section (targetm.asm_out.function_rodata_section + (current_function_decl, relocatable)); + } else switch_to_section (current_function_section ()); diff --git a/gcc/output.h b/gcc/output.h index eb253c50329..94eb0d6ebef 100644 --- a/gcc/output.h +++ b/gcc/output.h @@ -571,8 +571,8 @@ extern void default_ctor_section_asm_out_constructor (rtx, int); extern section *default_select_section (tree, int, unsigned HOST_WIDE_INT); extern section *default_elf_select_section (tree, int, unsigned HOST_WIDE_INT); extern void default_unique_section (tree, int); -extern section *default_function_rodata_section (tree); -extern section *default_no_function_rodata_section (tree); +extern section *default_function_rodata_section (tree, bool relocatable); +extern section *default_no_function_rodata_section (tree, bool relocatable); extern section *default_clone_table_section (void); extern section *default_select_rtx_section (machine_mode, rtx, unsigned HOST_WIDE_INT); diff --git a/gcc/target.def b/gcc/target.def index 07059a87caf..8cda974c3c5 100644 --- a/gcc/target.def +++ b/gcc/target.def @@ -549,16 +549,17 @@ Whatever the actual target object format, this is often good enough.", void, (tree decl, int reloc), default_unique_section) -/* Return the readonly data section associated with function DECL. */ +/* Return the readonly or relocated readonly data section + associated with function DECL. */ DEFHOOK (function_rodata_section, - "Return the readonly data section associated with\n\ + "Return the readonly or reloc readonly data section associated with\n\ @samp{DECL_SECTION_NAME (@var{decl})}.\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\ -if function is in @code{.text.name}, and the normal readonly-data section\n\ -otherwise.", - section *, (tree decl), +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) /* Nonnull if the target wants to override the default ".rodata" prefix diff --git a/gcc/varasm.c b/gcc/varasm.c index 4070f9c17e8..91ab75aed06 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -726,12 +726,26 @@ switch_to_other_text_partition (void) switch_to_section (current_function_section ()); } -/* Return the read-only data section associated with function DECL. */ +/* Return the read-only or relocated read-only data section + associated with function DECL. */ section * -default_function_rodata_section (tree decl) +default_function_rodata_section (tree decl, bool relocatable) { - if (decl != NULL_TREE && DECL_SECTION_NAME (decl)) + const char* sname; + unsigned int flags; + + flags = 0; + + if (relocatable) + { + sname = ".data.rel.ro.local"; + flags = (SECTION_WRITE | SECTION_RELRO); + } + else + sname = ".rodata"; + + if (decl && DECL_SECTION_NAME (decl)) { const char *name = DECL_SECTION_NAME (decl); @@ -744,12 +758,12 @@ default_function_rodata_section (tree decl) dot = strchr (name + 1, '.'); if (!dot) dot = name; - len = strlen (dot) + 8; + len = strlen (dot) + strlen (sname) + 1; rname = (char *) alloca (len); - strcpy (rname, ".rodata"); + strcpy (rname, sname); strcat (rname, dot); - return get_section (rname, SECTION_LINKONCE, decl); + return get_section (rname, (SECTION_LINKONCE | flags), decl); } /* For .gnu.linkonce.t.foo we want to use .gnu.linkonce.r.foo. */ else if (DECL_COMDAT_GROUP (decl) @@ -767,15 +781,18 @@ default_function_rodata_section (tree decl) && strncmp (name, ".text.", 6) == 0) { size_t len = strlen (name) + 1; - char *rname = (char *) alloca (len + 2); + char *rname = (char *) alloca (len + strlen (sname) - 5); - memcpy (rname, ".rodata", 7); - memcpy (rname + 7, name + 5, len - 5); - return get_section (rname, 0, decl); + memcpy (rname, sname, strlen (sname)); + memcpy (rname + strlen (sname), name + 5, len - 5); + return get_section (rname, flags, decl); } } - return readonly_data_section; + if (relocatable) + return get_section (sname, flags, decl); + else + return readonly_data_section; } /* Return the read-only data section associated with function DECL @@ -783,7 +800,8 @@ default_function_rodata_section (tree decl) readonly data section. */ section * -default_no_function_rodata_section (tree decl ATTRIBUTE_UNUSED) +default_no_function_rodata_section (tree decl ATTRIBUTE_UNUSED, + bool relocatable ATTRIBUTE_UNUSED) { return readonly_data_section; } @@ -793,7 +811,8 @@ default_no_function_rodata_section (tree decl ATTRIBUTE_UNUSED) static const char * function_mergeable_rodata_prefix (void) { - section *s = targetm.asm_out.function_rodata_section (current_function_decl); + section *s = targetm.asm_out.function_rodata_section (current_function_decl, + false); if (SECTION_STYLE (s) == SECTION_NAMED) return s->named.name; else