This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
RE: [PATCH] x86: Allow -fcf-protection with multi-byte NOPs
- From: "Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>
- To: "H.J. Lu" <hjl dot tools at gmail dot com>, Jakub Jelinek <jakub at redhat dot com>
- Cc: Richard Biener <richard dot guenther at gmail dot com>, Uros Bizjak <ubizjak at gmail dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, "Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>
- Date: Thu, 19 Apr 2018 21:18:59 +0000
- Subject: RE: [PATCH] x86: Allow -fcf-protection with multi-byte NOPs
- Dlp-product: dlpe-windows
- Dlp-reaction: no-action
- Dlp-version: 11.0.200.100
- References: <20180417184224.GA22831@intel.com> <CAFULd4bKLossrJKNqUpz0A6Jo+mygfZ6GrHYyYYwtosEUeqi-g@mail.gmail.com> <CAMe9rOreCsxS6=xU9m2ZS_-BwLYPHR0k5wL9Zpwk-J-8qBHgPw@mail.gmail.com> <CAMe9rOpki1DdsBr6y9XWL9Nxh7_=SEsQrkVoth5Eka5OeXgGYA@mail.gmail.com> <CAMe9rOqgatF__WY8kdyj+X=aJMq--_V1ODk7L4Dze2R=1iQO6g@mail.gmail.com> <CAMe9rOqrZcOEghYkJFRqWET7bMn0bCF21TSthaRQLratSvxbFg@mail.gmail.com> <CAFiYyc3R5jqFsX8=HwOzudq-qZ6nDFMcOykh2anhwPam3UAPKw@mail.gmail.com> <20180419133037.GA22554@gmail.com> <20180419192555.GC8577@tucnak> <CAMe9rOpwcZJ+-sx9KZkojU_CD10EO=e-OjHqDjM9gFP7NhaR5Q@mail.gmail.com>
> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of H.J. Lu
> Sent: Thursday, April 19, 2018 10:02 PM
> To: Jakub Jelinek <jakub@redhat.com>
> Cc: Richard Biener <richard.guenther@gmail.com>; Uros Bizjak
> <ubizjak@gmail.com>; gcc-patches@gcc.gnu.org; Tsimbalist, Igor V
> <igor.v.tsimbalist@intel.com>
> Subject: Re: [PATCH] x86: Allow -fcf-protection with multi-byte NOPs
>
> On Thu, Apr 19, 2018 at 12:25 PM, Jakub Jelinek <jakub@redhat.com>
> wrote:
> > On Thu, Apr 19, 2018 at 06:30:37AM -0700, H.J. Lu wrote:
> >> * config/i386/i386-c.c (ix86_target_macros_internal): Also
> >> define __IBT__ and __SHSTK__ for -fcf-protection.
> >
> >> --- a/gcc/config/i386/i386-c.c
> >> +++ b/gcc/config/i386/i386-c.c
> >> @@ -499,13 +499,15 @@ ix86_target_macros_internal (HOST_WIDE_INT
> isa_flag,
> >> def_or_undef (parse_in, "__RDPID__");
> >> if (isa_flag & OPTION_MASK_ISA_GFNI)
> >> def_or_undef (parse_in, "__GFNI__");
> >> - if (isa_flag2 & OPTION_MASK_ISA_IBT)
> >> + if ((isa_flag2 & OPTION_MASK_ISA_IBT)
> >> + || (flag_cf_protection & CF_BRANCH))
> >> {
> >> def_or_undef (parse_in, "__IBT__");
> >> if (flag_cf_protection != CF_NONE)
> >> def_or_undef (parse_in, "__CET__");
> >> }
> >> - if (isa_flag & OPTION_MASK_ISA_SHSTK)
> >> + if ((isa_flag & OPTION_MASK_ISA_SHSTK)
> >> + || (flag_cf_protection & CF_RETURN))
> >> {
> >> def_or_undef (parse_in, "__SHSTK__");
> >> if (flag_cf_protection != CF_NONE)
> >> def_or_undef (parse_in, "__CET__");
> >> }
> >
> > This looks completely wrong to me.
> > 1) there is no way to find out through preprocessor macros if
> > -mibt or -mshstk was actually used or not, so e.g. if you
> > #include <cetintrin.h>
> > and compile with -fcf-protection -mno-cet, then
> > #ifndef __SHSTK__
> > #pragma GCC push_options
> > #pragma GCC target ("shstk")
> > #define __DISABLE_SHSTK__
> > #endif /* __SHSTK__ */
> > will not be done and thus the intrinsics will appear to be in
> > in the default target (-mno-cet)
> > 2) preexisting - __CET__ is predefined twice, it should be done only
> > once using a condition that covers all cases when the macro should be
> > defined
> >
> > Don't you want to just predefine __CET__ and not __IBT__/__SHSTK__
> > if -fcf-protection -mno-cet, to make it clear?
> >
>
> We are removing -mibt:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85469
>
> How about this?
>
>
> diff --git a/gcc/config/i386/i386-c.c b/gcc/config/i386/i386-c.c
> index fa8b3682b0c..26c7641075d 100644
> --- a/gcc/config/i386/i386-c.c
> +++ b/gcc/config/i386/i386-c.c
> @@ -499,20 +499,14 @@ ix86_target_macros_internal (HOST_WIDE_INT
> isa_flag,
> def_or_undef (parse_in, "__RDPID__");
> if (isa_flag & OPTION_MASK_ISA_GFNI)
> def_or_undef (parse_in, "__GFNI__");
> - if ((isa_flag2 & OPTION_MASK_ISA_IBT)
> - || (flag_cf_protection & CF_BRANCH))
> - {
> - def_or_undef (parse_in, "__IBT__");
> - if (flag_cf_protection != CF_NONE)
> - def_or_undef (parse_in, "__CET__");
> - }
> - if ((isa_flag & OPTION_MASK_ISA_SHSTK)
> - || (flag_cf_protection & CF_RETURN))
> - {
> - def_or_undef (parse_in, "__SHSTK__");
> - if (flag_cf_protection != CF_NONE)
> - def_or_undef (parse_in, "__CET__");
> - }
> + if ((isa_flag & OPTION_MASK_ISA_SHSTK))
> + def_or_undef (parse_in, "__SHSTK__");
> + if (flag_cf_protection != CF_NONE)
> + def_or_undef (parse_in, "__CET__");
> + if ((flag_cf_protection & CF_BRANCH))
> + def_or_undef (parse_in, "__CET_IBT__");
> + if ((flag_cf_protection & CF_RETURN))
> + def_or_undef (parse_in, "__CET_SHSTK__");
> if (isa_flag2 & OPTION_MASK_ISA_VAES)
> def_or_undef (parse_in, "__VAES__");
> if (isa_flag & OPTION_MASK_ISA_VPCLMULQDQ)
>
> This adds __CET_IBT__ and __CET_SHSTK__.
As -fcf-protection and -mcet/-mibt/-mshstk are are disjoint and
control different parts I agree with
+ if ((isa_flag & OPTION_MASK_ISA_SHSTK))
+ def_or_undef (parse_in, "__SHSTK__");
+ if (flag_cf_protection != CF_NONE)
+ def_or_undef (parse_in, "__CET__");
Why __CET_IBT__ and __CET_SHSTK__ are needed? Moreover the naming is
confusing as 'IBT' and 'SHSTK' are related to HW features which are controlled
by -m options. __CET__ seems to be enough.
Igor
>
> --
> H.J.