> -----Original Message----- > From: H.J. Lu [mailto:hjl.to...@gmail.com] > Sent: Friday, April 20, 2018 12:08 AM > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com> > Cc: Jakub Jelinek <ja...@redhat.com>; Richard Biener > <richard.guent...@gmail.com>; Uros Bizjak <ubiz...@gmail.com>; gcc- > patc...@gcc.gnu.org > Subject: Re: [PATCH] x86: Allow -fcf-protection with multi-byte NOPs > > On Thu, Apr 19, 2018 at 2:18 PM, Tsimbalist, Igor V > <igor.v.tsimbal...@intel.com> wrote: > >> -----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. > > > > One needs to know if IBT and SHSTK are enabled by -fcf-protection. They will > be checked by <cet.h> and glibc.
Ok. But then __CET__ could be removed as it's easily calculated given these two _CET_IBT_ and _CET_SHSTK_ macros. Also individual macro is used mostly. Igor > -- > H.J.
