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

[Tsimbalist, Igor V]

> -----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.