[PATCH] x86: Allow -fcf-protection with multi-byte NOPs

Tsimbalist, Igor V igor.v.tsimbalist@intel.com
Thu Apr 19 21:19:00 GMT 2018


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


More information about the Gcc-patches mailing list