> -----Original Message----- > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > ow...@gcc.gnu.org] On Behalf Of H.J. Lu > Sent: Thursday, April 19, 2018 10:02 PM > To: Jakub Jelinek <ja...@redhat.com> > Cc: Richard Biener <richard.guent...@gmail.com>; Uros Bizjak > <ubiz...@gmail.com>; gcc-patches@gcc.gnu.org; Tsimbalist, Igor V > <igor.v.tsimbal...@intel.com> > Subject: Re: [PATCH] x86: Allow -fcf-protection with multi-byte NOPs > > On Thu, Apr 19, 2018 at 12:25 PM, Jakub Jelinek <ja...@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.