On Wed, Feb 5, 2020 at 12:20 PM H.J. Lu <hjl.to...@gmail.com> wrote:
>
> On Wed, Feb 5, 2020 at 9:00 AM Richard Sandiford
> <richard.sandif...@arm.com> wrote:
> >
> > "H.J. Lu" <hjl.to...@gmail.com> writes:
> > > Currently patchable area is at the wrong place.
> >
> > Agreed :-)
> >
> > > It is placed immediately
> > > after function label and before .cfi_startproc. A backend should be able
> > > to add a pseudo patchable area instruction durectly into RTL. This patch
> > > adds patch_area_size and patch_area_entry to cfun so that the patchable
> > > area info is available in RTL passes.
> >
> > It might be better to add it to crtl, since it should only be needed
> > during rtl generation.
> >
> > > It also limits patch_area_size and patch_area_entry to 65535, which is
> > > a reasonable maximum size for patchable area.
> > >
> > > gcc/
> > >
> > > PR target/93492
> > > * function.c (expand_function_start): Set cfun->patch_area_size
> > > and cfun->patch_area_entry.
> > > * function.h (function): Add patch_area_size and patch_area_entry.
> > > * opts.c (common_handle_option): Limit
> > > function_entry_patch_area_size and function_entry_patch_area_start
> > > to USHRT_MAX. Fix a typo in error message.
> > > * varasm.c (assemble_start_function): Use cfun->patch_area_size
> > > and cfun->patch_area_entry.
> > > * doc/invoke.texi: Document the maximum value for
> > > -fpatchable-function-entry.
> > >
> > > gcc/testsuite/
> > >
> > > PR target/93492
> > > * c-c++-common/patchable_function_entry-error-1.c: New test.
> > > * c-c++-common/patchable_function_entry-error-2.c: Likewise.
> > > * c-c++-common/patchable_function_entry-error-3.c: Likewise.
> > > ---
> > > gcc/doc/invoke.texi | 1 +
> > > gcc/function.c | 35 +++++++++++++++++++
> > > gcc/function.h | 6 ++++
> > > gcc/opts.c | 4 ++-
> > > .../patchable_function_entry-error-1.c | 9 +++++
> > > .../patchable_function_entry-error-2.c | 9 +++++
> > > .../patchable_function_entry-error-3.c | 20 +++++++++++
> > > gcc/varasm.c | 30 ++--------------
> > > 8 files changed, 85 insertions(+), 29 deletions(-)
> > > create mode 100644
> > > gcc/testsuite/c-c++-common/patchable_function_entry-error-1.c
> > > create mode 100644
> > > gcc/testsuite/c-c++-common/patchable_function_entry-error-2.c
> > > create mode 100644
> > > gcc/testsuite/c-c++-common/patchable_function_entry-error-3.c
> > >
> > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > > index 35b341e759f..dd4835199b0 100644
> > > --- a/gcc/doc/invoke.texi
> > > +++ b/gcc/doc/invoke.texi
> > > @@ -13966,6 +13966,7 @@ If @code{N=0}, no pad location is recorded.
> > > The NOP instructions are inserted at---and maybe before, depending on
> > > @var{M}---the function entry address, even before the prologue.
> > >
> > > +The maximum value of @var{N} and @var{M} is 65535.
> > > @end table
> > >
> > >
> > > diff --git a/gcc/function.c b/gcc/function.c
> > > index d8008f60422..badbf538eec 100644
> > > --- a/gcc/function.c
> > > +++ b/gcc/function.c
> > > @@ -5202,6 +5202,41 @@ expand_function_start (tree subr)
> > > /* If we are doing generic stack checking, the probe should go here.
> > > */
> > > if (flag_stack_check == GENERIC_STACK_CHECK)
> > > stack_check_probe_note = emit_note (NOTE_INSN_DELETED);
> > > +
> > > + unsigned HOST_WIDE_INT patch_area_size =
> > > function_entry_patch_area_size;
> > > + unsigned HOST_WIDE_INT patch_area_entry =
> > > function_entry_patch_area_start;
> > > +
> > > + tree patchable_function_entry_attr
> > > + = lookup_attribute ("patchable_function_entry",
> > > + DECL_ATTRIBUTES (cfun->decl));
> > > + if (patchable_function_entry_attr)
> > > + {
> > > + tree pp_val = TREE_VALUE (patchable_function_entry_attr);
> > > + tree patchable_function_entry_value1 = TREE_VALUE (pp_val);
> > > +
> > > + patch_area_size = tree_to_uhwi (patchable_function_entry_value1);
> > > + patch_area_entry = 0;
> > > + if (TREE_CHAIN (pp_val) != NULL_TREE)
> > > + {
> > > + tree patchable_function_entry_value2
> > > + = TREE_VALUE (TREE_CHAIN (pp_val));
> > > + patch_area_entry = tree_to_uhwi (patchable_function_entry_value2);
> > > + }
> > > + if (patch_area_size > USHRT_MAX || patch_area_size > USHRT_MAX)
> > > + error ("invalid values for %<patchable_function_entry%> attribute");
> >
> > This should probably go in handle_patchable_function_entry_attribute
> > instead. It doesn't look like we have a clear policy about whether
> > errors or warnings are right for unrecognised arguments to known
> > attribute names, but handle_patchable_function_entry_attribute reports
> > an OPT_Wattributes warning for arguments that aren't integers. Doing the
> > same for out-of-range integers would be more consistent and means that
> > we wouldn't break existing code if we relaxed/changed the rules in future.
>
> Like this? OK for master if there is no regression?
>
There is a small problem. Warnings from C and C++ frond-ends are different:
[hjl@gnu-skx-1 gcc]$ cat x.c
void
__attribute__((patchable_function_entry(65536)))
foo1 (void)
{ /* { dg-warning "'patchable_function_entry' attribute argument
'65536' is out of range" } */
}
[hjl@gnu-skx-1 gcc]$ ./xgcc -B./ -S x.c
x.c:4:1: warning: ‘patchable_function_entry’ attribute argument
‘65536’ is out of range (> 65535) [-Wattributes]
4 | { /* { dg-warning "'patchable_function_entry' attribute
argument '65536' is out of range" } */
| ^
[hjl@gnu-skx-1 gcc]$ ./xg++ -B./ -S x.c
x.c:3:11: warning: ‘patchable_function_entry’ attribute argument
‘65536’ is out of range (> 65535) [-Wattributes]
3 | foo1 (void)
| ^
[hjl@gnu-skx-1 gcc]$
C warns at line 4 and C++ warns at line 3. Do I need separate tests
for C and C++?
--
H.J.
