Re: [PATCH] i386: Don't generate ENDBR if function is only called directly

[Uros Bizjak <ubizjak at gmail dot com>]

On Tue, Oct 24, 2017 at 10:39 AM, Tsimbalist, Igor V
<igor.v.tsimbalist@intel.com> wrote:
> OK.

+/* { dg-final { scan-assembler-times "endbr32" 2 { target ia32 } } } */
+/* { dg-final { scan-assembler-times "endbr64" 2 { target { ! ia32 } } } } */

I think we can only check for {\mendbr} in the testcases. There are
already plenty of testcases that check for the correct instruction.

Otherwise, LGTM

Thanks,
Uros.

> Igor
>
>
>> -----Original Message-----
>> From: H.J. Lu [mailto:hjl.tools@gmail.com]
>> Sent: Tuesday, October 24, 2017 1:01 AM
>> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>
>> Cc: Uros Bizjak <ubizjak@gmail.com>; gcc-patches@gcc.gnu.org
>> Subject: Re: [PATCH] i386: Don't generate ENDBR if function is only called
>> directly
>>
>> On Mon, Oct 23, 2017 at 3:19 PM, Tsimbalist, Igor V
>> <igor.v.tsimbalist@intel.com> wrote:
>> > You are right. The functions in the tests should be changed to static scope
>> to trigger the check in the patch. After that I expect there should be no
>> endbr generated at all for the static functions and that's is wrong.
>> >
>>
>> Here is the updated patch with new testcases.  OK for trunk if there are
>> no regressions?
>>
>> Thanks.
>>
>> H.J.
>> >
>> >> -----Original Message-----
>> >> From: H.J. Lu [mailto:hjl.tools@gmail.com]
>> >> Sent: Tuesday, October 24, 2017 12:06 AM
>> >> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>
>> >> Cc: Uros Bizjak <ubizjak@gmail.com>; gcc-patches@gcc.gnu.org
>> >> Subject: Re: [PATCH] i386: Don't generate ENDBR if function is only called
>> >> directly
>> >>
>> >> On Mon, Oct 23, 2017 at 3:01 PM, Tsimbalist, Igor V
>> >> <igor.v.tsimbalist@intel.com> wrote:
>> >> > Existing tests cet-label.c cet-switch-2.c cet-sjlj-1.c cet-sjlj-3.c should
>> catch
>> >> this.
>> >>
>> >> There are no regressions with my patch.  Did I miss something?
>> >>
>> >> > Igor
>> >> >
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: H.J. Lu [mailto:hjl.tools@gmail.com]
>> >> >> Sent: Monday, October 23, 2017 11:50 PM
>> >> >> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>
>> >> >> Cc: Uros Bizjak <ubizjak@gmail.com>; gcc-patches@gcc.gnu.org
>> >> >> Subject: Re: [PATCH] i386: Don't generate ENDBR if function is only
>> called
>> >> >> directly
>> >> >>
>> >> >> On Mon, Oct 23, 2017 at 2:44 PM, Tsimbalist, Igor V
>> >> >> <igor.v.tsimbalist@intel.com> wrote:
>> >> >> > The change will skip a whole function from endbr processing by
>> >> >> rest_of_insert_endbranch,
>> >> >> > which inserts endbr not only at the beginning of the function but
>> inside
>> >> the
>> >> >> function's
>> >> >> > body also. For example, tests with setjmp should fail.
>> >> >> >
>> >> >> > I would suggest to insert the check in rest_of_insert_endbranch
>> >> function,
>> >> >> something like this
>> >> >> >
>> >> >> >   if (!(lookup_attribute ("nocf_check",
>> >> >> >                           TYPE_ATTRIBUTES (TREE_TYPE (cfun->decl)))
>> >> >> >         || cgraph_node::get (fun->decl)->only_called_directly_p ())
>> >> >> >
>> >> >> > Igor
>> >> >>
>> >> >> Can you provide one test for each case to cover all of them?
>> >> >>
>> >> >>
>> >> >> >
>> >> >> >> -----Original Message-----
>> >> >> >> From: Uros Bizjak [mailto:ubizjak@gmail.com]
>> >> >> >> Sent: Monday, October 23, 2017 9:26 PM
>> >> >> >> To: H.J. Lu <hjl.tools@gmail.com>
>> >> >> >> Cc: gcc-patches@gcc.gnu.org; Tsimbalist, Igor V
>> >> >> >> <igor.v.tsimbalist@intel.com>
>> >> >> >> Subject: Re: [PATCH] i386: Don't generate ENDBR if function is only
>> >> called
>> >> >> >> directly
>> >> >> >>
>> >> >> >> On Sun, Oct 22, 2017 at 4:13 PM, H.J. Lu <hjl.tools@gmail.com>
>> wrote:
>> >> >> >> > There is no need to insert ENDBR instruction if function is only
>> called
>> >> >> >> > directly.
>> >> >> >> >
>> >> >> >> > OK for trunk if there is no regressions?
>> >> >> >>
>> >> >> >> Patch needs to be OK'd by Igor first.
>> >> >> >>
>> >> >> >> Uros.
>> >> >> >>
>> >> >> >> > H.J.
>> >> >> >> > ----
>> >> >> >> > gcc/
>> >> >> >> >
>> >> >> >> >         PR target/82659
>> >> >> >> >         * config/i386/i386.c (pass_insert_endbranch::gate): Return
>> >> >> >> >         false if function is only called directly.
>> >> >> >> >
>> >> >> >> > gcc/testsuite/
>> >> >> >> >
>> >> >> >> >         PR target/82659
>> >> >> >> >         * gcc.target/i386/pr82659-1.c: New test.
>> >> >> >> >         * gcc.target/i386/pr82659-2.c: Likewise.
>> >> >> >> >         * gcc.target/i386/pr82659-3.c: Likewise.
>> >> >> >> >         * gcc.target/i386/pr82659-4.c: Likewise.
>> >> >> >> >         * gcc.target/i386/pr82659-5.c: Likewise.
>> >> >> >> >         * gcc.target/i386/pr82659-6.c: Likewise.
>> >> >> >> > ---
>> >> >> >> >  gcc/config/i386/i386.c                    |  6 ++++--
>> >> >> >> >  gcc/testsuite/gcc.target/i386/pr82659-1.c | 19
>> >> +++++++++++++++++++
>> >> >> >> >  gcc/testsuite/gcc.target/i386/pr82659-2.c | 18
>> >> ++++++++++++++++++
>> >> >> >> >  gcc/testsuite/gcc.target/i386/pr82659-3.c | 21
>> >> >> +++++++++++++++++++++
>> >> >> >> >  gcc/testsuite/gcc.target/i386/pr82659-4.c | 15
>> +++++++++++++++
>> >> >> >> >  gcc/testsuite/gcc.target/i386/pr82659-5.c | 10 ++++++++++
>> >> >> >> >  gcc/testsuite/gcc.target/i386/pr82659-6.c | 19
>> >> +++++++++++++++++++
>> >> >> >> >  7 files changed, 106 insertions(+), 2 deletions(-)
>> >> >> >> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr82659-1.c
>> >> >> >> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr82659-2.c
>> >> >> >> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr82659-3.c
>> >> >> >> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr82659-4.c
>> >> >> >> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr82659-5.c
>> >> >> >> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr82659-6.c
>> >> >> >> >
>> >> >> >> > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> >> >> >> > index fb0b7e71469..b86504378ae 100644
>> >> >> >> > --- a/gcc/config/i386/i386.c
>> >> >> >> > +++ b/gcc/config/i386/i386.c
>> >> >> >> > @@ -2693,9 +2693,11 @@ public:
>> >> >> >> >    {}
>> >> >> >> >
>> >> >> >> >    /* opt_pass methods: */
>> >> >> >> > -  virtual bool gate (function *)
>> >> >> >> > +  virtual bool gate (function *fun)
>> >> >> >> >      {
>> >> >> >> > -      return ((flag_cf_protection & CF_BRANCH) && TARGET_IBT);
>> >> >> >> > +      return ((flag_cf_protection & CF_BRANCH)
>> >> >> >> > +             && TARGET_IBT
>> >> >> >> > +             && !cgraph_node::get (fun->decl)-
>> >only_called_directly_p
>> >> ());
>> >> >> >> >      }
>> >> >> >> >
>> >> >> >> >    virtual unsigned int execute (function *)
>> >> >> >> > diff --git a/gcc/testsuite/gcc.target/i386/pr82659-1.c
>> >> >> >> b/gcc/testsuite/gcc.target/i386/pr82659-1.c
>> >> >> >> > new file mode 100644
>> >> >> >> > index 00000000000..8f0a6906815
>> >> >> >> > --- /dev/null
>> >> >> >> > +++ b/gcc/testsuite/gcc.target/i386/pr82659-1.c
>> >> >> >> > @@ -0,0 +1,19 @@
>> >> >> >> > +/* { dg-do compile } */
>> >> >> >> > +/* { dg-options "-O2 -fcf-protection -mcet" } */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr32" 1 { target ia32 } } }
>> */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr64" 1 { target { ! ia32 }
>> } }
>> >> } */
>> >> >> >> > +
>> >> >> >> > +extern int x;
>> >> >> >> > +
>> >> >> >> > +static void
>> >> >> >> > +__attribute__ ((noinline, noclone))
>> >> >> >> > +test (int i)
>> >> >> >> > +{
>> >> >> >> > +  x = i;
>> >> >> >> > +}
>> >> >> >> > +
>> >> >> >> > +void
>> >> >> >> > +bar (int i)
>> >> >> >> > +{
>> >> >> >> > +  test (i);
>> >> >> >> > +}
>> >> >> >> > diff --git a/gcc/testsuite/gcc.target/i386/pr82659-2.c
>> >> >> >> b/gcc/testsuite/gcc.target/i386/pr82659-2.c
>> >> >> >> > new file mode 100644
>> >> >> >> > index 00000000000..228a20006b6
>> >> >> >> > --- /dev/null
>> >> >> >> > +++ b/gcc/testsuite/gcc.target/i386/pr82659-2.c
>> >> >> >> > @@ -0,0 +1,18 @@
>> >> >> >> > +/* { dg-do compile } */
>> >> >> >> > +/* { dg-options "-O2 -fcf-protection -mcet" } */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr32" 2 { target ia32 } } }
>> */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr64" 2 { target { ! ia32 }
>> } }
>> >> } */
>> >> >> >> > +
>> >> >> >> > +extern int x;
>> >> >> >> > +
>> >> >> >> > +void
>> >> >> >> > +test (int i)
>> >> >> >> > +{
>> >> >> >> > +  x = i;
>> >> >> >> > +}
>> >> >> >> > +
>> >> >> >> > +void
>> >> >> >> > +bar (int i)
>> >> >> >> > +{
>> >> >> >> > +  test (i);
>> >> >> >> > +}
>> >> >> >> > diff --git a/gcc/testsuite/gcc.target/i386/pr82659-3.c
>> >> >> >> b/gcc/testsuite/gcc.target/i386/pr82659-3.c
>> >> >> >> > new file mode 100644
>> >> >> >> > index 00000000000..6ae23e40abc
>> >> >> >> > --- /dev/null
>> >> >> >> > +++ b/gcc/testsuite/gcc.target/i386/pr82659-3.c
>> >> >> >> > @@ -0,0 +1,21 @@
>> >> >> >> > +/* { dg-do compile } */
>> >> >> >> > +/* { dg-options "-O2 -fcf-protection -mcet" } */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr32" 2 { target ia32 } } }
>> */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr64" 2 { target { ! ia32 }
>> } }
>> >> } */
>> >> >> >> > +
>> >> >> >> > +extern int x;
>> >> >> >> > +
>> >> >> >> > +static void
>> >> >> >> > +__attribute__ ((noinline, noclone))
>> >> >> >> > +test (int i)
>> >> >> >> > +{
>> >> >> >> > +  x = i;
>> >> >> >> > +}
>> >> >> >> > +
>> >> >> >> > +extern __typeof (test) foo __attribute__ ((alias ("test")));
>> >> >> >> > +
>> >> >> >> > +void
>> >> >> >> > +bar (int i)
>> >> >> >> > +{
>> >> >> >> > +  test (i);
>> >> >> >> > +}
>> >> >> >> > diff --git a/gcc/testsuite/gcc.target/i386/pr82659-4.c
>> >> >> >> b/gcc/testsuite/gcc.target/i386/pr82659-4.c
>> >> >> >> > new file mode 100644
>> >> >> >> > index 00000000000..ca87264e98b
>> >> >> >> > --- /dev/null
>> >> >> >> > +++ b/gcc/testsuite/gcc.target/i386/pr82659-4.c
>> >> >> >> > @@ -0,0 +1,15 @@
>> >> >> >> > +/* { dg-do compile } */
>> >> >> >> > +/* { dg-options "-O2 -fcf-protection -mcet" } */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr32" 2 { target ia32 } } }
>> */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr64" 2 { target { ! ia32 }
>> } }
>> >> } */
>> >> >> >> > +
>> >> >> >> > +static void
>> >> >> >> > +test (void)
>> >> >> >> > +{
>> >> >> >> > +}
>> >> >> >> > +
>> >> >> >> > +void *
>> >> >> >> > +bar (void)
>> >> >> >> > +{
>> >> >> >> > +  return test;
>> >> >> >> > +}
>> >> >> >> > diff --git a/gcc/testsuite/gcc.target/i386/pr82659-5.c
>> >> >> >> b/gcc/testsuite/gcc.target/i386/pr82659-5.c
>> >> >> >> > new file mode 100644
>> >> >> >> > index 00000000000..c34eade0f90
>> >> >> >> > --- /dev/null
>> >> >> >> > +++ b/gcc/testsuite/gcc.target/i386/pr82659-5.c
>> >> >> >> > @@ -0,0 +1,10 @@
>> >> >> >> > +/* { dg-do compile } */
>> >> >> >> > +/* { dg-options "-O2 -fcf-protection -mcet" } */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr32" 1 { target ia32 } } }
>> */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr64" 1 { target { ! ia32 }
>> } }
>> >> } */
>> >> >> >> > +static void
>> >> >> >> > +test (void)
>> >> >> >> > +{
>> >> >> >> > +}
>> >> >> >> > +
>> >> >> >> > +void (*test_p) (void) = test;
>> >> >> >> > diff --git a/gcc/testsuite/gcc.target/i386/pr82659-6.c
>> >> >> >> b/gcc/testsuite/gcc.target/i386/pr82659-6.c
>> >> >> >> > new file mode 100644
>> >> >> >> > index 00000000000..b4ffd65c74f
>> >> >> >> > --- /dev/null
>> >> >> >> > +++ b/gcc/testsuite/gcc.target/i386/pr82659-6.c
>> >> >> >> > @@ -0,0 +1,19 @@
>> >> >> >> > +/* { dg-do compile } */
>> >> >> >> > +/* { dg-options "-O2 -fcf-protection -mcet" } */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr32" 2 { target ia32 } } }
>> */
>> >> >> >> > +/* { dg-final { scan-assembler-times "endbr64" 2 { target { ! ia32 }
>> } }
>> >> } */
>> >> >> >> > +
>> >> >> >> > +extern int x;
>> >> >> >> > +
>> >> >> >> > + __attribute__ ((visibility ("hidden")))
>> >> >> >> > +void
>> >> >> >> > +test (int i)
>> >> >> >> > +{
>> >> >> >> > +  x = i;
>> >> >> >> > +}
>> >> >> >> > +
>> >> >> >> > +void
>> >> >> >> > +bar (int i)
>> >> >> >> > +{
>> >> >> >> > +  test (i);
>> >> >> >> > +}
>> >> >> >> > --
>> >> >> >> > 2.13.6
>> >> >> >> >
>> >> >>
>> >> >>
>> >> >>
>> >> >> --
>> >> >> H.J.
>> >>
>> >>
>> >>
>> >> --
>> >> H.J.
>>
>>
>>
>> --
>> H.J.