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
