"H.J. Lu" <hjl.to...@gmail.com> writes:
> On Wed, Feb 5, 2020 at 2:51 PM H.J. Lu <hjl.to...@gmail.com> wrote:
>>
>> On Wed, Feb 5, 2020 at 2:37 PM Marek Polacek <pola...@redhat.com> wrote:
>> >
>> > On Wed, Feb 05, 2020 at 02:24:48PM -0800, H.J. Lu wrote:
>> > > 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++?
>> >
>> > I think better would be
>> >
>> > /* { dg-error "foo" "" { target c } } */
>> > /* { dg-error "bar" "" { target c++ } .-1 } */
>> >
>> > Marek
>> >
>>
>> It worked.
>
> Here is the patch with updated tests. There are no regressions on
> Linux/x86-64.
> OK for master?
>
> Thanks.
>
>
> --
> H.J.
>
> From 8320bbb83e53f9e135fe1eca50840baacf157881 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.to...@gmail.com>
> Date: Wed, 5 Feb 2020 04:01:59 -0800
> Subject: [PATCH] Add patch_area_size and patch_area_entry to crtl
>
> Currently patchable area is at the wrong place. 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 crtl so that the patchable
> area info is available in RTL passes.
>
> It also limits patch_area_size and patch_area_entry to 65535, which is
> a reasonable maximum size for patchable area.
>
> gcc/c-family/
>
> PR target/93492
> * c-attribs.c (handle_patchable_function_entry_attribute): Limit
> value to USHRT_MAX (65535).
>
> gcc/
>
> PR target/93492
> * cfgexpand.c (pass_expand::execute): Set crtl->patch_area_size
> and crtl->patch_area_entry.
> * emit-rtl.h (rtl_data): 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 crtl->patch_area_size
> and crtl->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.
LGTM, but an RM should have the final say on whether this is
suitable for stage 4.
Thanks,
Richard
