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