On Mon, May 10, 2021 at 04:39:55PM -0500, Segher Boessenkool wrote:
> 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?
For the new call to default_print_patchable_function_entry in
rs6000_output_function_prologue.
>
> > +/* 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.
There is a fine line between trickiness and elegance. Given the
current available crtl variables dealing with the patch area and the
current patchable_function_entry parameters, any supposedly "less
hacky" but correct expression will simplify down to what I wrote
here.
--
Alan Modra
Australia Development Lab, IBM
