Hi!
On Fri, May 07, 2021 at 12:19:52PM +0930, Alan Modra wrote:
> PowerPC64 ELFv2 dual entry point functions have a couple of problems
> with -fpatchable-function-entry. One is that the nops added after the
> global entry land in the global entry code which is constrained to be
> a power of two number of instructions, and zero global entry code has
> special meaning for linkage. The other is that the global entry code
> isn't always used on function entry, and some uses of
> -fpatchable-function-entry might want to affect all entries to the
> function. So this patch arranges to put one batch of nops before the
> global entry, and the other after the local entry point.
Wow ugly. Not worse than patchable-funtion-enbtry itself I suppose :-)
> * config/rs6000/rs6000-logue.c: Include targhooks.h.
Huh, did it not already do that?! Hrm, all the other hooks seem to be
called via rs6000.c currently. But you do the same, so why do you need
to include the header in rs6000-logue.c?
> +/* Write NOPs into the asm outfile FILE around a function entry. This
> + routine may be called twice per function to put NOPs before and after
> + the function entry. If RECORD_P is true the location of the NOPs will
> + be recorded by default_print_patchable_function_entry in a special
> + object section called "__patchable_function_entries". Disable output
> + of any NOPs for the second call. Those, if any, are output by
> + rs6000_output_function_prologue. This means that for ELFv2 any NOPs
> + after the function entry are placed after the local entry point, not
> + the global entry point. NOPs after the entry may be found at
> + record_loc + nops_before * 4 + local_entry_offset. This holds true
> + when nops_before is zero. */
> +
> +static void
> +rs6000_print_patchable_function_entry (FILE *file,
> + unsigned HOST_WIDE_INT patch_area_size
> ATTRIBUTE_UNUSED,
> + bool record_p)
It is not a predicate. Do not call it _p please. Yes, I know the
generic code calls is this. That needs fixing.
A better name (from the viewpoint of callers, which is what matters!)
might be first_time or similar? Or something that really says what it
*does*?
> +{
> + /* Always call default_print_patchable_function_entry when RECORD_P in
> + order to output the location of the NOPs, but use the size of the
> + area before the entry on both possible calls. If RECORD_P is true
> + on the second call then the area before the entry was zero size and
> + thus no NOPs will be output. */
> + if (record_p)
> + default_print_patchable_function_entry (file, crtl->patch_area_entry,
> + record_p);
> +}
That is trickiness that will break later.
Can you make this less hacky please? Changing the generic code is just
fine as well, it needs some love.
Segher
