On Thu, Feb 6, 2020 at 12:01 AM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> "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.
>
This is the patch I am checking in.
Thanks.
--
H.J.
From f4a89f5c9beb31f707c970db0320e4687e962c88 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.
---
gcc/c-family/c-attribs.c | 9 +++++
gcc/cfgexpand.c | 33 +++++++++++++++++++
gcc/doc/invoke.texi | 1 +
gcc/emit-rtl.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 ++---------------
9 files changed, 92 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/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index ac936d5bbbb..a101312c581 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -4553,6 +4553,15 @@ handle_patchable_function_entry_attribute (tree *, tree name, tree args,
*no_add_attrs = true;
return NULL_TREE;
}
+
+ if (tree_to_uhwi (val) > USHRT_MAX)
+ {
+ warning (OPT_Wattributes,
+ "%qE attribute argument %qE is out of range (> 65535)",
+ name, val);
+ *no_add_attrs = true;
+ return NULL_TREE;
+ }
}
return NULL_TREE;
}
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index a7ec77d5c85..86efa22bf60 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -6656,6 +6656,39 @@ pass_expand::execute (function *fun)
if (crtl->tail_call_emit)
fixup_tail_calls ();
+ 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_entry > patch_area_size)
+ {
+ if (patch_area_size > 0)
+ warning (OPT_Wattributes,
+ "patchable function entry %wu exceeds size %wu",
+ patch_area_entry, patch_area_size);
+ patch_area_entry = 0;
+ }
+
+ crtl->patch_area_size = patch_area_size;
+ crtl->patch_area_entry = patch_area_entry;
+
/* BB subdivision may have created basic blocks that are only reachable
from unlikely bbs but not marked as such in the profile. */
if (optimize)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 527d362533a..767d1f07801 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -14112,6 +14112,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/emit-rtl.h b/gcc/emit-rtl.h
index a878efe3cf7..3d6565c8a30 100644
--- a/gcc/emit-rtl.h
+++ b/gcc/emit-rtl.h
@@ -173,6 +173,12 @@ struct GTY(()) rtl_data {
local stack. */
unsigned int stack_alignment_estimated;
+ /* How many NOP insns to place at each function entry by default. */
+ unsigned short patch_area_size;
+
+ /* How far the real asm entry point is into this area. */
+ unsigned short patch_area_entry;
+
/* For reorg. */
/* Nonzero if function being compiled called builtin_return_addr or
diff --git a/gcc/opts.c b/gcc/opts.c
index c212a1a57dc..3dccef39701 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -2615,10 +2615,12 @@ common_handle_option (struct gcc_options *opts,
function_entry_patch_area_start = 0;
}
if (function_entry_patch_area_size < 0
+ || function_entry_patch_area_size > USHRT_MAX
|| function_entry_patch_area_start < 0
+ || function_entry_patch_area_start > USHRT_MAX
|| function_entry_patch_area_size
< function_entry_patch_area_start)
- error ("invalid arguments for %<-fpatchable_function_entry%>");
+ error ("invalid arguments for %<-fpatchable-function-entry%>");
free (patch_area_arg);
}
break;
diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-error-1.c b/gcc/testsuite/c-c++-common/patchable_function_entry-error-1.c
new file mode 100644
index 00000000000..f60bf46cfe3
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/patchable_function_entry-error-1.c
@@ -0,0 +1,9 @@
+/* { dg-do compile { target { ! { nvptx*-*-* visium-*-* } } } } */
+/* { dg-options "-O2 -fpatchable-function-entry=65536,1" } */
+/* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */
+/* { dg-error "invalid arguments for '-fpatchable-function-entry'" "" { target *-*-* } 0 } */
+
+void
+foo (void)
+{
+}
diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-error-2.c b/gcc/testsuite/c-c++-common/patchable_function_entry-error-2.c
new file mode 100644
index 00000000000..90f88c78be7
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/patchable_function_entry-error-2.c
@@ -0,0 +1,9 @@
+/* { dg-do compile { target { ! { nvptx*-*-* visium-*-* } } } } */
+/* { dg-options "-O2 -fpatchable-function-entry=1,65536" } */
+/* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */
+/* { dg-error "invalid arguments for '-fpatchable-function-entry'" "" { target *-*-* } 0 } */
+
+void
+foo (void)
+{
+}
diff --git a/gcc/testsuite/c-c++-common/patchable_function_entry-error-3.c b/gcc/testsuite/c-c++-common/patchable_function_entry-error-3.c
new file mode 100644
index 00000000000..45e93988886
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/patchable_function_entry-error-3.c
@@ -0,0 +1,20 @@
+/* { dg-do compile { target { ! { nvptx*-*-* visium-*-* } } } } */
+/* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */
+
+void
+ __attribute__((patchable_function_entry(65536)))
+foo1 (void)
+{ /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" } */
+}
+
+void
+ __attribute__((patchable_function_entry(65536,1)))
+foo2 (void)
+{ /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" } */
+}
+
+void
+ __attribute__((patchable_function_entry(65536,65536)))
+foo3 (void)
+{ /* { dg-warning "'patchable_function_entry' attribute argument '65536' is out of range" } */
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 271a67abf56..f062e48071f 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -1857,34 +1857,8 @@ assemble_start_function (tree decl, const char *fnname)
if (DECL_PRESERVE_P (decl))
targetm.asm_out.mark_decl_preserved (fnname);
- 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 (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_entry > patch_area_size)
- {
- if (patch_area_size > 0)
- warning (OPT_Wattributes,
- "patchable function entry %wu exceeds size %wu",
- patch_area_entry, patch_area_size);
- patch_area_entry = 0;
- }
+ unsigned short patch_area_size = crtl->patch_area_size;
+ unsigned short patch_area_entry = crtl->patch_area_entry;
/* Emit the patching area before the entry label, if any. */
if (patch_area_entry > 0)
--
2.26.2
