RE: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation

["Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>]

> -----Original Message-----
> From: Uros Bizjak [mailto:ubizjak@gmail.com]
> Sent: Friday, October 13, 2017 10:02 AM
> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation
> 
> On Thu, Oct 12, 2017 at 8:54 PM, Tsimbalist, Igor V
> <igor.v.tsimbalist@intel.com> wrote:
> > Attached is an updated patch according to your comments. New tests are
> > added to test ICF optimization in presence of nocf_check attribute.
> --- a/gcc/testsuite/c-c++-common/fcf-protection-2.c
> +++ b/gcc/testsuite/c-c++-common/fcf-protection-2.c
> @@ -1,4 +1,4 @@
>  /* { dg-do compile } */
>  /* { dg-options "-fcf-protection=branch" } */
> -/* { dg-error "'-fcf-protection=branch' is not supported for this target" "" {
> target { "i?86-*-* x86_64-*-*" } } 0 } */
> +/* { dg-error "'-fcf-protection=branch' requires CET support on this
> target. Use -mcet or one of -mibt, -mshstk options to enable CET" "" { target {
> "i?86-*-* x86_64-*-*" } } 0 } */
> 
> Checking for "-fcf-protection=branch' requires CET support on this target"
> should be enough. No need to check the whole message here and in other
> tests.

Fixed as you suggested. Also shortened the checking string for ignoring the
attribute in attr-nocf-check-1.c and attr-nocf-check-3.c.

>  /* { dg-error "'-fcf-protection=branch' is not supported for this target" "" {
> target { ! "i?86-*-* x86_64-*-*" } } 0 } */ diff --git a/gcc/testsuite/c-c++-
> common/fcf-protection-3.c
> b/gcc/testsuite/c-c++-common/fcf-protection-3.c
> 
> 
> --- a/gcc/testsuite/c-c++-common/fcf-protection-4.c
> +++ b/gcc/testsuite/c-c++-common/fcf-protection-4.c
> @@ -1,4 +1,4 @@
>  /* { dg-do compile } */
>  /* { dg-options "-fcf-protection=none" } */
> -/* { dg-bogus "'-fcf-protection=none' is not supported for this target" "" {
> target { "i?86-*-* x86_64-*-*" } } 0 } */
> +/* { dg-bogus "'-fcf-protection=none' res CET support on this target.
> Use -mcet or one of -mibt, -mshstk options to enable CET" "" { target { "i?86-
> *-* x86_64-*-*" } } 0 } */
>  /* { dg-bogus "'-fcf-protection=none' is not supported for this target" "" {
> target { ! "i?86-*-* x86_64-*-*" } } 0 } */ diff --git a/gcc/testsuite/c-c++-
> common/fcf-protection-5.c
> b/gcc/testsuite/c-c++-common/fcf-protection-5.c
> 
> The above test checks for bogus messages? -fcf-protection=none option
> should not generate any messages. So, the test should check that -fcf-
> protection=none doesn't generate any error. (And, there is a typo in the
> message, /s/res/requires.)

The gcc documentation says about dg-bogus

This DejaGnu directive appears on a source line that should not get a message
matching regexp...

I decided to use dg-bogus to check the absence of the error. Now I removed both
lines as any additional messages should be caught as an extra messages. Actually
I will update the fcf-protection-4.c test in the generic patch.

Updated patch is attached. 

Igor

> Uros.
> 
> > Igor
> >
> >
> >> -----Original Message-----
> >> From: Tsimbalist, Igor V
> >> Sent: Tuesday, September 19, 2017 11:30 PM
> >> To: Uros Bizjak <ubizjak@gmail.com>
> >> Cc: gcc-patches@gcc.gnu.org; Tsimbalist, Igor V
> >> <igor.v.tsimbalist@intel.com>
> >> Subject: RE: 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation
> >>
> >> > -----Original Message-----
> >> > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> >> > owner@gcc.gnu.org] On Behalf Of Uros Bizjak
> >> > Sent: Tuesday, September 19, 2017 6:13 PM
> >> > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>
> >> > Cc: gcc-patches@gcc.gnu.org
> >> > Subject: Re:
> >> > 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation
> >> >
> >> > On Tue, Sep 19, 2017 at 5:18 PM, Tsimbalist, Igor V
> >> > <igor.v.tsimbalist@intel.com> wrote:
> >> > >> -----Original Message-----
> >> > >> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> >> > >> owner@gcc.gnu.org] On Behalf Of Uros Bizjak
> >> > >> Sent: Monday, September 18, 2017 12:17 PM
> >> > >> To: gcc-patches@gcc.gnu.org
> >> > >> Cc: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>;
> >> > >> Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>
> >> > >> Subject: Re:
> >> > >> 0006-Part-6.-Add-x86-tests-for-Intel-CET-implementation
> >> > >>
> >> > >> Hello!
> >> > >>
> >> > >> > gcc/testsuite/
> >> > >> >
> >> > >> > * g++.dg/cet-notrack-1.C: New test.
> >> > >> > * gcc.target/i386/cet-intrin-1.c: Likewise.
> >> > >> > * gcc.target/i386/cet-intrin-10.c: Likewise.
> >> > >> > * gcc.target/i386/cet-intrin-2.c: Likewise.
> >> > >> > * gcc.target/i386/cet-intrin-3.c: Likewise.
> >> > >> > * gcc.target/i386/cet-intrin-4.c: Likewise.
> >> > >> > * gcc.target/i386/cet-intrin-5.c: Likewise.
> >> > >> > * gcc.target/i386/cet-intrin-6.c: Likewise.
> >> > >> > * gcc.target/i386/cet-intrin-7.c: Likewise.
> >> > >> > * gcc.target/i386/cet-intrin-8.c: Likewise.
> >> > >> > * gcc.target/i386/cet-intrin-9.c: Likewise.
> >> > >> > * gcc.target/i386/cet-label.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-1a.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-1b.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-2a.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-2b.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-3.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-4a.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-4b.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-5a.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-5b.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-6a.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-6b.c: Likewise.
> >> > >> > * gcc.target/i386/cet-notrack-7.c: Likewise.
> >> > >> > * gcc.target/i386/cet-property-1.c: Likewise.
> >> > >> > * gcc.target/i386/cet-property-2.c: Likewise.
> >> > >> > * gcc.target/i386/cet-rdssp-1.c: Likewise.
> >> > >> > * gcc.target/i386/cet-sjlj-1.c: Likewise.
> >> > >> > * gcc.target/i386/cet-sjlj-2.c: Likewise.
> >> > >> > * gcc.target/i386/cet-sjlj-3.c: Likewise.
> >> > >> > * gcc.target/i386/cet-switch-1.c: Likewise.
> >> > >> > * gcc.target/i386/cet-switch-2.c: Likewise.
> >> > >> > * lib/target-supports.exp (check_effective_target_cet): New proc.
> >> > >>
> >> > >> A couple of questions:
> >> > >>
> >> > >> +/* { dg-do compile } */
> >> > >> +/* { dg-options "-O2 -mcet" } */
> >> > >> +/* { dg-final { scan-assembler-times "setssbsy" 2 } } */
> >> > >> +
> >> > >> +#include <immintrin.h>
> >> > >> +
> >> > >> +void f1 (void)
> >> > >> +{
> >> > >> +  __builtin_ia32_setssbsy ();
> >> > >> +}
> >> > >> +
> >> > >> +void f2 (void)
> >> > >> +{
> >> > >> +  _setssbsy ();
> >> > >> +}
> >> > >>
> >> > >> Is there a reason that both, __builtin and intrinsic versions
> >> > >> are tested in a couple of places? The intrinsic version is just
> >> > >> a wrapper for __builtin, so IMO testing intrinsic version should
> >> > >> be
> >> enough.
> >> > > No strong reason. Just to check that intrinsic names are
> >> > > recognized and
> >> > processed correctly.
> >> > > The implementation could change and the test will catch
> inconsistency.
> >> > > I would also assume a user will use intrinsics that's why I add
> >> > > intrinsic
> >> check.
> >> > Should I remove it?
> >> >
> >> > Actually, these __builtins are considered as implementation detail,
> >> > and their use should be discouraged. They are deliberately not
> >> > documented, and users should use intrinsic headers instead. That
> >> > said, builtins won't change without a reason, since Ada needs them.
> >> >
> >> > It can happen that the test fails due to change of intrinsics, so
> >> > I'd recommend to remove them.
> >> Ok, I will remove intrinsic.
> >>
> >> > >> diff --git a/gcc/testsuite/gcc.target/i386/cet-rdssp-1.c
> >> > >> b/gcc/testsuite/gcc.target/i386/cet-rdssp-1.c
> >> > >> new file mode 100644
> >> > >> index 0000000..f9223a5
> >> > >> --- /dev/null
> >> > >> +++ b/gcc/testsuite/gcc.target/i386/cet-rdssp-1.c
> >> > >> @@ -0,0 +1,39 @@
> >> > >> +/* { dg-do run { target cet } } */
> >> > >> +/* { dg-options "-O2 -finstrument-control-flow -mcet" } */
> >> > >>
> >> > >> The "target cet" directive just checks that CET instructions can
> >> > >> be
> >> > compiled.
> >> > >> The test will (probably?) fail on targets with binutils that can
> >> > >> compile CET instructions, but the target itself doesn't support CET.
> >> > >> If this is the case, then check header has to be introduced, so
> >> > >> the test can be bypassed on targets without runtime support.
> >> > > The test will not fail even if a target doesn't support CET as 'rdssp'
> >> > > instruction is a NOP on such target and further usage of CET
> >> > > instruction is bypassed. In this case the code
> >> > >
> >> > > +  ssp = rdssp (ssp);
> >> > >
> >> > > Will keep ssp as 0.
> >> >
> >> > I assume that this is true also for other runtime tests, and this
> >> > clears my concern about runtime failures with updated binutils.
> >> Yes, that's true for other runtime tests.
> >>
> >> Igor
> >>
> >> >
> >> > Uros.

Attachment: 0006-Add-x86-tests-for-Intel-CET-implementation.patch
Description: 0006-Add-x86-tests-for-Intel-CET-implementation.patch