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

[H.J. Lu]

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


-- 
H.J.