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

[Tsimbalist, Igor V]

> -Original Message-
> From: H.J. Lu [mailto:hjl.to...@gmail.com]
> Sent: Friday, April 20, 2018 1:15 PM
> To: Jakub Jelinek <ja...@redhat.com>
> Cc: Tsimbalist, Igor V <igor.v.tsimbal...@intel.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 Fri, Apr 20, 2018 at 09:39:58AM +0200, Jakub Jelinek wrote:
> > On Fri, Apr 20, 2018 at 06:25:10AM +, Tsimbalist, Igor V wrote:
> > > > Something like this?
> > >
> > > Shouldn't this
> > >
> > > -# ifdef __IBT__
> > > +# if (__CET__ & 1) != 0
> > >
> > > Be as
> > >
> > > -# ifdef __IBT__
> > > +#ifdef __CET__
> > > +# if (__CET__ & 1) != 0
> > >
> > > OK otherwise.
> >
> > Only if you use -Wundef warning (not part of -Wall or -W) and, if this
> > is a system header, only with -Wundef -Wsystem-headers.
> > But perhaps it doesn't hurt to wrap it.
> >
> > Jakub
> 
> Here is the patch.  OK for trunk?
> 
> Thanks.

OK.

Igor

> 
> H.J.
> ---
> With revision 259496:
> 
> commit b1384095a7c1d06a44b70853372ebe037b2f7867
> Author: hjl <hjl@138bc75d-0d04-0410-961f-82ee72b054a4>
> Date:   Thu Apr 19 15:15:04 2018 +
> 
> x86: Enable -fcf-protection with multi-byte NOPs
> 
> -mibt does nothing and can be removed.  Define __CET__ to indicate level
> protection with -fcf-protection:
> 
> (__CET__ & 1) != 0: -fcf-protection=branch or -fcf-protection=full
> (__CET__ & 2) != 0: -fcf-protection=return or -fcf-protection=full
> 
> gcc/
> 
>   PR target/85469
>   * common/config/i386/i386-common.c
> (OPTION_MASK_ISA_IBT_SET):
>   Removed.
>   (OPTION_MASK_ISA_IBT_UNSET): Likewise.
>   (ix86_handle_option): Don't handle OPT_mibt.
>   * config/i386/cet.h: Check __CET__ instead of __IBT__ and
>   __SHSTK__.
>   * config/i386/driver-i386.c (host_detect_local_cpu): Remove
>   has_ibt and ibt.
>   * config/i386/i386-c.c (ix86_target_macros_internal): Don't
>   check OPTION_MASK_ISA_IBT nor flag_cf_protection.
>   (ix86_target_macros): Define __CET__ with flag_cf_protection
>   for -fcf-protection.
>   * config/i386/i386.c (isa2_opts): Remove -mibt.
>   * config/i386/i386.h (TARGET_IBT): Removed.
>   (TARGET_IBT_P): Likewise.
>   (ix86_valid_target_attribute_inner_p): Don't check OPT_mibt.
>   * config/i386/i386.md (nop_endbr): Don't check TARGET_IBT.
>   * config/i386/i386.opt (mcet): Update help message.
>   (mshstk): Likewise.
>   (mibt): Removed.
>   * doc/invoke.texi: Remove -mibt.  Document __CET__.  Document
>   -mcet as an alias for -mshstk.
> 
> gcc/testsuite/
> 
>   PR target/85469
>   * gcc.target/i386/pr85044.c (dg-options): Remove -mibt.
>   * gcc.target/i386/sse-26.c (dg-options): Remove -mno-ibt.
> ---
>  gcc/common/config/i386/i386-common.c| 17 -
>  gcc/config/i386/cet.h   |  6 +++---
>  gcc/config/i386/driver-i386.c   |  6 ++
>  gcc/config/i386/i386-c.c| 20 ++--
>  gcc/config/i386/i386.c  |  2 --
>  gcc/config/i386/i386.h  |  2 --
>  gcc/config/i386/i386.md |  2 +-
>  gcc/config/i386/i386.opt| 12 
>  gcc/doc/invoke.texi | 28 +++-
>  gcc/testsuite/gcc.target/i386/pr85044.c |  2 +-
>  gcc/testsuite/gcc.target/i386/sse-26.c  |  2 +-
>  11 files changed, 29 insertions(+), 70 deletions(-)
> 
> diff --git a/gcc/common/config/i386/i386-common.c
> b/gcc/common/config/i386/i386-common.c
> index 0bb2783cfab..74a3490f7a3 100644
> --- a/gcc/common/config/i386/i386-common.c
> +++ b/gcc/common/config/i386/i386-common.c
> @@ -147,7 +147,6 @@ along with GCC; see the file COPYING3.  If not see
>  #define OPTION_MASK_ISA_PKU_SET OPTION_MASK_ISA_PKU
>  #define OPTION_MASK_ISA_RDPID_SET OPTION_MASK_ISA_RDPID
>  #define OPTION_MASK_ISA_GFNI_SET OPTION_MASK_ISA_GFNI
> -#define OPTION_MASK_ISA_IBT_SET OPTION_MASK_ISA_IBT
>  #define OPTION_MASK_ISA_SHSTK_SET OPTION_MASK_ISA_SHSTK
>  #define OPTION_MASK_ISA_VAES_SET OPTION_MASK_ISA_VAES
>  #define OPTION_MASK_ISA_VPCLMULQDQ_SET
> OPTION_MASK_ISA_VPCLMULQDQ
> @@ -224,7 +223,6 @@ along with GCC; see the file COPYING3.  If not see
>  #define OPTION_MASK_ISA_PKU_UNSET OPTION_MASK_ISA_PKU
>  #define OPTION_MASK_ISA_RDPID_UNSET OPTION_MASK_ISA_RDPID
>  #define OPTION_MASK_ISA_GFNI_UNSET OPTION_MASK_I

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

[H.J. Lu]

On Fri, Apr 20, 2018 at 09:39:58AM +0200, Jakub Jelinek wrote:
> On Fri, Apr 20, 2018 at 06:25:10AM +, Tsimbalist, Igor V wrote:
> > > Something like this?
> > 
> > Shouldn't this
> > 
> > -# ifdef __IBT__
> > +# if (__CET__ & 1) != 0
> > 
> > Be as
> > 
> > -# ifdef __IBT__
> > +#ifdef __CET__
> > +# if (__CET__ & 1) != 0
> > 
> > OK otherwise.
> 
> Only if you use -Wundef warning (not part of -Wall or -W) and, if this
> is a system header, only with -Wundef -Wsystem-headers.
> But perhaps it doesn't hurt to wrap it.
> 
>   Jakub

Here is the patch.  OK for trunk?

Thanks.


H.J.
---
With revision 259496:

commit b1384095a7c1d06a44b70853372ebe037b2f7867
Author: hjl 
Date:   Thu Apr 19 15:15:04 2018 +

x86: Enable -fcf-protection with multi-byte NOPs

-mibt does nothing and can be removed.  Define __CET__ to indicate level
protection with -fcf-protection:

(__CET__ & 1) != 0: -fcf-protection=branch or -fcf-protection=full
(__CET__ & 2) != 0: -fcf-protection=return or -fcf-protection=full

gcc/

PR target/85469
* common/config/i386/i386-common.c (OPTION_MASK_ISA_IBT_SET):
Removed.
(OPTION_MASK_ISA_IBT_UNSET): Likewise.
(ix86_handle_option): Don't handle OPT_mibt.
* config/i386/cet.h: Check __CET__ instead of __IBT__ and
__SHSTK__.
* config/i386/driver-i386.c (host_detect_local_cpu): Remove
has_ibt and ibt.
* config/i386/i386-c.c (ix86_target_macros_internal): Don't
check OPTION_MASK_ISA_IBT nor flag_cf_protection.
(ix86_target_macros): Define __CET__ with flag_cf_protection
for -fcf-protection.
* config/i386/i386.c (isa2_opts): Remove -mibt.
* config/i386/i386.h (TARGET_IBT): Removed.
(TARGET_IBT_P): Likewise.
(ix86_valid_target_attribute_inner_p): Don't check OPT_mibt.
* config/i386/i386.md (nop_endbr): Don't check TARGET_IBT.
* config/i386/i386.opt (mcet): Update help message.
(mshstk): Likewise.
(mibt): Removed.
* doc/invoke.texi: Remove -mibt.  Document __CET__.  Document
-mcet as an alias for -mshstk.

gcc/testsuite/

PR target/85469
* gcc.target/i386/pr85044.c (dg-options): Remove -mibt.
* gcc.target/i386/sse-26.c (dg-options): Remove -mno-ibt.
---
 gcc/common/config/i386/i386-common.c| 17 -
 gcc/config/i386/cet.h   |  6 +++---
 gcc/config/i386/driver-i386.c   |  6 ++
 gcc/config/i386/i386-c.c| 20 ++--
 gcc/config/i386/i386.c  |  2 --
 gcc/config/i386/i386.h  |  2 --
 gcc/config/i386/i386.md |  2 +-
 gcc/config/i386/i386.opt| 12 
 gcc/doc/invoke.texi | 28 +++-
 gcc/testsuite/gcc.target/i386/pr85044.c |  2 +-
 gcc/testsuite/gcc.target/i386/sse-26.c  |  2 +-
 11 files changed, 29 insertions(+), 70 deletions(-)

diff --git a/gcc/common/config/i386/i386-common.c 
b/gcc/common/config/i386/i386-common.c
index 0bb2783cfab..74a3490f7a3 100644
--- a/gcc/common/config/i386/i386-common.c
+++ b/gcc/common/config/i386/i386-common.c
@@ -147,7 +147,6 @@ along with GCC; see the file COPYING3.  If not see
 #define OPTION_MASK_ISA_PKU_SET OPTION_MASK_ISA_PKU
 #define OPTION_MASK_ISA_RDPID_SET OPTION_MASK_ISA_RDPID
 #define OPTION_MASK_ISA_GFNI_SET OPTION_MASK_ISA_GFNI
-#define OPTION_MASK_ISA_IBT_SET OPTION_MASK_ISA_IBT
 #define OPTION_MASK_ISA_SHSTK_SET OPTION_MASK_ISA_SHSTK
 #define OPTION_MASK_ISA_VAES_SET OPTION_MASK_ISA_VAES
 #define OPTION_MASK_ISA_VPCLMULQDQ_SET OPTION_MASK_ISA_VPCLMULQDQ
@@ -224,7 +223,6 @@ along with GCC; see the file COPYING3.  If not see
 #define OPTION_MASK_ISA_PKU_UNSET OPTION_MASK_ISA_PKU
 #define OPTION_MASK_ISA_RDPID_UNSET OPTION_MASK_ISA_RDPID
 #define OPTION_MASK_ISA_GFNI_UNSET OPTION_MASK_ISA_GFNI
-#define OPTION_MASK_ISA_IBT_UNSET OPTION_MASK_ISA_IBT
 #define OPTION_MASK_ISA_SHSTK_UNSET OPTION_MASK_ISA_SHSTK
 #define OPTION_MASK_ISA_VAES_UNSET OPTION_MASK_ISA_VAES
 #define OPTION_MASK_ISA_VPCLMULQDQ_UNSET OPTION_MASK_ISA_VPCLMULQDQ
@@ -546,21 +544,6 @@ ix86_handle_option (struct gcc_options *opts,
   return true;
 
 case OPT_mcet:
-case OPT_mibt:
-  if (value)
-   {
- opts->x_ix86_isa_flags2 |= OPTION_MASK_ISA_IBT_SET;
- opts->x_ix86_isa_flags2_explicit |= OPTION_MASK_ISA_IBT_SET;
-   }
-  else
-   {
- opts->x_ix86_isa_flags2 &= ~OPTION_MASK_ISA_IBT_UNSET;
- opts->x_ix86_isa_flags2_explicit |= OPTION_MASK_ISA_IBT_UNSET;
-   }
-  if (code != OPT_mcet)
-   return true;
-  /* fall through.  */
-
 case OPT_mshstk:
   if (value)
{
diff --git a/gcc/config/i386/cet.h b/gcc/config/i386/cet.h
index 9dca41bad2d..309f6428735 100644
--- a/gcc/config/i386/cet.h
+++ b/gcc/config/i386/cet.h
@@ -32,7 +32,7 @@
 
 #ifdef __ASSEMBLER__
 
-# ifdef 

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

[Jakub Jelinek]

On Fri, Apr 20, 2018 at 06:25:10AM +, Tsimbalist, Igor V wrote:
> > Something like this?
> 
> Shouldn't this
> 
> -# ifdef __IBT__
> +# if (__CET__ & 1) != 0
> 
> Be as
> 
> -# ifdef __IBT__
> +#ifdef __CET__
> +# if (__CET__ & 1) != 0
> 
> OK otherwise.

Only if you use -Wundef warning (not part of -Wall or -W) and, if this
is a system header, only with -Wundef -Wsystem-headers.
But perhaps it doesn't hurt to wrap it.

Jakub

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

[Jakub Jelinek]

On Thu, Apr 19, 2018 at 03:08:06PM -0700, H.J. Lu wrote:
> > 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  and glibc.

So can't you define __CET__ to 3 if CF_FULL, to 1 if CF_BRANCH and 2 if
CF_RETURN?  Then if code doesn't care which one it is, it can just #ifdef
__CET__, otherwise it can test which of those is enabled.
Implementation-wise it would probably need to be:
  if (flag_cf_protection != CF_NONE)
{
  if (def_or_undef == cpp_undef)
def_or_undef (parse_in, "__CET__");
  else if ((flag_cf_protection & CF_FULL) == CF_FULL)
def_or_undef (parse_in, "__CET__=3");
  else if (flag_cf_protection & CF_BRANCH)
def_or_undef (parse_in, "__CET__=1");
  else if (flag_cf_protection & CF_RETURN)
def_or_undef (parse_in, "__CET__=2");
}
or so.  Actually, because it doesn't depend on something that can change
depending on target attributes, it probably doesn't even belong in this
function, but to ix86_target_macros and there you can just cpp_define
it, don't deal with cpp_undef at all.

Jakub

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

[Tsimbalist, Igor V]

> -Original Message-
> From: H.J. Lu [mailto:hjl.to...@gmail.com]
> Sent: Friday, April 20, 2018 3:17 AM
> To: Jakub Jelinek <ja...@redhat.com>
> Cc: Tsimbalist, Igor V <igor.v.tsimbal...@intel.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 3:37 PM, Jakub Jelinek <ja...@redhat.com> wrote:
> > On Thu, Apr 19, 2018 at 03:08:06PM -0700, H.J. Lu wrote:
> >> > 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  and glibc.
> >
> > So can't you define __CET__ to 3 if CF_FULL, to 1 if CF_BRANCH and 2 if
> > CF_RETURN?  Then if code doesn't care which one it is, it can just #ifdef
> > __CET__, otherwise it can test which of those is enabled.
> > Implementation-wise it would probably need to be:
> >   if (flag_cf_protection != CF_NONE)
> > {
> >   if (def_or_undef == cpp_undef)
> > def_or_undef (parse_in, "__CET__");
> >   else if ((flag_cf_protection & CF_FULL) == CF_FULL)
> > def_or_undef (parse_in, "__CET__=3");
> >   else if (flag_cf_protection & CF_BRANCH)
> > def_or_undef (parse_in, "__CET__=1");
> >   else if (flag_cf_protection & CF_RETURN)
> > def_or_undef (parse_in, "__CET__=2");
> > }
> > or so.  Actually, because it doesn't depend on something that can change
> > depending on target attributes, it probably doesn't even belong in this
> > function, but to ix86_target_macros and there you can just cpp_define
> > it, don't deal with cpp_undef at all.
> 
> Something like this?

Shouldn't this

-# ifdef __IBT__
+# if (__CET__ & 1) != 0

Be as

-# ifdef __IBT__
+#ifdef __CET__
+# if (__CET__ & 1) != 0

OK otherwise.

Igor

> 
> --
> H.J.

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

[H.J. Lu]

On Thu, Apr 19, 2018 at 3:37 PM, Jakub Jelinek  wrote:
> On Thu, Apr 19, 2018 at 03:08:06PM -0700, H.J. Lu wrote:
>> > 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  and glibc.
>
> So can't you define __CET__ to 3 if CF_FULL, to 1 if CF_BRANCH and 2 if
> CF_RETURN?  Then if code doesn't care which one it is, it can just #ifdef
> __CET__, otherwise it can test which of those is enabled.
> Implementation-wise it would probably need to be:
>   if (flag_cf_protection != CF_NONE)
> {
>   if (def_or_undef == cpp_undef)
> def_or_undef (parse_in, "__CET__");
>   else if ((flag_cf_protection & CF_FULL) == CF_FULL)
> def_or_undef (parse_in, "__CET__=3");
>   else if (flag_cf_protection & CF_BRANCH)
> def_or_undef (parse_in, "__CET__=1");
>   else if (flag_cf_protection & CF_RETURN)
> def_or_undef (parse_in, "__CET__=2");
> }
> or so.  Actually, because it doesn't depend on something that can change
> depending on target attributes, it probably doesn't even belong in this
> function, but to ix86_target_macros and there you can just cpp_define
> it, don't deal with cpp_undef at all.

Something like this?


-- 
H.J.
From 435b905d0dd60c3e6b00f42d7614a47972d69551 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" 
Date: Thu, 19 Apr 2018 14:09:51 -0700
Subject: [PATCH] Define __CET__ with flag_cf_protection and remove -mibt

---
 gcc/common/config/i386/i386-common.c | 17 -
 gcc/config/i386/cet.h|  6 +++---
 gcc/config/i386/i386-c.c | 20 ++--
 gcc/config/i386/i386.c   |  2 --
 gcc/config/i386/i386.md  |  2 +-
 gcc/config/i386/i386.opt | 12 
 gcc/doc/invoke.texi  | 23 ++-
 7 files changed, 20 insertions(+), 62 deletions(-)

diff --git a/gcc/common/config/i386/i386-common.c b/gcc/common/config/i386/i386-common.c
index 0bb2783cfab..74a3490f7a3 100644
--- a/gcc/common/config/i386/i386-common.c
+++ b/gcc/common/config/i386/i386-common.c
@@ -147,7 +147,6 @@ along with GCC; see the file COPYING3.  If not see
 #define OPTION_MASK_ISA_PKU_SET OPTION_MASK_ISA_PKU
 #define OPTION_MASK_ISA_RDPID_SET OPTION_MASK_ISA_RDPID
 #define OPTION_MASK_ISA_GFNI_SET OPTION_MASK_ISA_GFNI
-#define OPTION_MASK_ISA_IBT_SET OPTION_MASK_ISA_IBT
 #define OPTION_MASK_ISA_SHSTK_SET OPTION_MASK_ISA_SHSTK
 #define OPTION_MASK_ISA_VAES_SET OPTION_MASK_ISA_VAES
 #define OPTION_MASK_ISA_VPCLMULQDQ_SET OPTION_MASK_ISA_VPCLMULQDQ
@@ -224,7 +223,6 @@ along with GCC; see the file COPYING3.  If not see
 #define OPTION_MASK_ISA_PKU_UNSET OPTION_MASK_ISA_PKU
 #define OPTION_MASK_ISA_RDPID_UNSET OPTION_MASK_ISA_RDPID
 #define OPTION_MASK_ISA_GFNI_UNSET OPTION_MASK_ISA_GFNI
-#define OPTION_MASK_ISA_IBT_UNSET OPTION_MASK_ISA_IBT
 #define OPTION_MASK_ISA_SHSTK_UNSET OPTION_MASK_ISA_SHSTK
 #define OPTION_MASK_ISA_VAES_UNSET OPTION_MASK_ISA_VAES
 #define OPTION_MASK_ISA_VPCLMULQDQ_UNSET OPTION_MASK_ISA_VPCLMULQDQ
@@ -546,21 +544,6 @@ ix86_handle_option (struct gcc_options *opts,
   return true;
 
 case OPT_mcet:
-case OPT_mibt:
-  if (value)
-	{
-	  opts->x_ix86_isa_flags2 |= OPTION_MASK_ISA_IBT_SET;
-	  opts->x_ix86_isa_flags2_explicit |= OPTION_MASK_ISA_IBT_SET;
-	}
-  else
-	{
-	  opts->x_ix86_isa_flags2 &= ~OPTION_MASK_ISA_IBT_UNSET;
-	  opts->x_ix86_isa_flags2_explicit |= OPTION_MASK_ISA_IBT_UNSET;
-	}
-  if (code != OPT_mcet)
-	return true;
-  /* fall through.  */
-
 case OPT_mshstk:
   if (value)
 	{
diff --git a/gcc/config/i386/cet.h b/gcc/config/i386/cet.h
index 9dca41bad2d..c38e594ad92 100644
--- a/gcc/config/i386/cet.h
+++ b/gcc/config/i386/cet.h
@@ -32,7 +32,7 @@
 
 #ifdef __ASSEMBLER__
 
-# ifdef __IBT__
+# if (__CET__ & 1) != 0
 #  ifdef __x86_64__
 #   define _CET_ENDBR endbr64
 #  else
@@ -44,14 +44,14 @@
 
 # ifdef __ELF__
 #  ifdef __CET__
-#   ifdef __IBT__
+#   if (__CET__ & 1) != 0
 /* GNU_PROPERTY_X86_FEATURE_1_IBT.  */
 #define __PROPERTY_IBT 0x1
 #   else
 #define __PROPERTY_IBT 0x0
 #   endif
 
-#   ifdef __SHSTK__
+#   if (__CET__ & 2) != 0
 /* GNU_PROPERTY_X86_FEATURE_1_SHSTK.  */
 #define __PROPERTY_SHSTK 0x2
 #   else
diff --git a/gcc/config/i386/i386-c.c b/gcc/config/i386/i386-c.c
index fa8b3682b0c..ae7d678e77e 100644
--- a/gcc/config/i386/i386-c.c
+++ b/gcc/config/i386/i386-c.c
@@ -499,20 +499,8 @@ 

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

[Tsimbalist, Igor V]

> -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 
> >> > 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)
> >&

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

[H.J. Lu]

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

-- 
H.J.

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

[Tsimbalist, Igor V]

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

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

[Jakub Jelinek]

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

Jakub

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

[H.J. Lu]

On Thu, Apr 19, 2018 at 12:25 PM, Jakub Jelinek  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 
> 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.

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

[Tsimbalist, Igor V]

> -Original Message-
> From: Uros Bizjak [mailto:ubiz...@gmail.com]
> Sent: Thursday, April 19, 2018 3:36 PM
> To: H.J. Lu <hjl.to...@gmail.com>
> Cc: Richard Biener <richard.guent...@gmail.com>; gcc-
> patc...@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 3:30 PM, H.J. Lu <hjl.to...@gmail.com> wrote:
> > On Wed, Apr 18, 2018 at 01:35:33PM +0200, Richard Biener wrote:
> >> On Wed, Apr 18, 2018 at 1:24 PM, H.J. Lu <hjl.to...@gmail.com> wrote:
> >> > On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu <hjl.to...@gmail.com>
> wrote:
> >> >> On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu <hjl.to...@gmail.com>
> wrote:
> >> >>> On Tue, Apr 17, 2018 at 12:03 PM, H.J. Lu <hjl.to...@gmail.com>
> wrote:
> >> >>>> On Tue, Apr 17, 2018 at 11:55 AM, Uros Bizjak
> <ubiz...@gmail.com> wrote:
> >> >>>>> On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu
> <hongjiu...@intel.com> wrote:
> >> >>>>>> -fcf-protection -mcet can't be used with IFUNC features, like
> symbol
> >> >>>>>> multiversioning or target clone, since IBT/SHSTK are applied to
> the whole
> >> >>>>>> program and they may be disabled in some functions.  But -
> fcf-protection
> >> >>>>>> is implemented with multi-byte NOPs on all 64-bit processors
> as well as
> >> >>>>>> 32-bit processors starting with Pentium Pro.  If -fcf-protection
> requires
> >> >>>>>> -mcet, IFUNC features can't be used on Linux when -fcf-
> protection is
> >> >>>>>> enabled by default.
> >> >>>>>>
> >> >>>>>> This patch changes -fcf-protection to to enable the NOP
> portion of CET
> >> >>>>>> ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of
> CET
> >> >>>>>> ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.
> >> >>>>>>
> >> >>>>>> OK for trunk?
> >> >>>>>
> >> >>>>> As said in the PR, NOP sequences have non-zero cost in the
> executable
> >> >>>>> (they enlarge the executable), so I don't think this feature should
> be
> >> >>>>> enabled by default.
> >> >>>>>
> >> >>>>> There is always a configure option if someone wants their
> compiler to
> >> >>>>> always emit relevant multi-byte nops.
> >> >>>>
> >> >>>> What we need is an option to enable -fcf-function with multi-byte
> NOPs
> >> >>>> without -mcet which enables the full CET ISAs.  A configure option
> >> >>>> without the corresponding the command-line option makes test
> and
> >> >>>> debug difficult.   I can add
> >> >>>>
> >> >>>> --enable-cf-function-nop or --with-cf-function-nop
> >> >>>>
> >> >>>> with
> >> >>>>
> >> >>>> -fct-function-nop
> >> >>>>
> >> >>>
> >> >>> How about adding -mno-cet, which enables the NOP portion of
> CET
> >> >>
> >> >> I meant -mnop-cet, not -mno-cet.
> >> >>
> >> >
> >> > Here is a patch to add -mnop and use it with -fcf-protection.
> >>
> >> +mnop
> >> +Target Report Var(flag_nop) Init(0)
> >> +Support multi-byte NOP code generation.
> >>
> >> the option name is incredibly bad and the documentation doesn't make it
> >> better either.  The invoke.texi docs refer to duplicate {-mcet}.
> >>
> >> Isn't there a -fcf-protection sub-set that can be used to automatically
> >> enable this?  Or simply do this mode by default when
> >> -fcf-protection is used but neither -mcet nor -mibt is enabled?
> >>
> >
> > Since multi-byte NOPs are used to implement -fcf-protection on x86, we
> > propose a new design for -fcf-protection:
> >
> > 1. -fcf-protection option will report the unsupported error on non-x86
> > platform. On x86 platform it's supported and inserts endbr-nop
> > instructions and properties, depending on its value (full/branch/return)
> > 2. -mcet/-mibt/-mshstk options control intrinsics only.
> > 3. These options are independent and do not influence each other so no
> > need for cross checking between them.
> >
> > OK for trunk?
> 
> This patch touches only CET related code, so Igor's OK should be enough.

I have reviewed the patch and I'm ok with it.

Igor

> Uros.

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

[Richard Biener]

On Thu, Apr 19, 2018 at 3:30 PM, H.J. Lu  wrote:
> On Wed, Apr 18, 2018 at 01:35:33PM +0200, Richard Biener wrote:
>> On Wed, Apr 18, 2018 at 1:24 PM, H.J. Lu  wrote:
>> > On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
>> >> On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
>> >>> On Tue, Apr 17, 2018 at 12:03 PM, H.J. Lu  wrote:
>>  On Tue, Apr 17, 2018 at 11:55 AM, Uros Bizjak  wrote:
>> > On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu  wrote:
>> >> -fcf-protection -mcet can't be used with IFUNC features, like symbol
>> >> multiversioning or target clone, since IBT/SHSTK are applied to the 
>> >> whole
>> >> program and they may be disabled in some functions.  But 
>> >> -fcf-protection
>> >> is implemented with multi-byte NOPs on all 64-bit processors as well 
>> >> as
>> >> 32-bit processors starting with Pentium Pro.  If -fcf-protection 
>> >> requires
>> >> -mcet, IFUNC features can't be used on Linux when -fcf-protection is
>> >> enabled by default.
>> >>
>> >> This patch changes -fcf-protection to to enable the NOP portion of CET
>> >> ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of CET
>> >> ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.
>> >>
>> >> OK for trunk?
>> >
>> > As said in the PR, NOP sequences have non-zero cost in the executable
>> > (they enlarge the executable), so I don't think this feature should be
>> > enabled by default.
>> >
>> > There is always a configure option if someone wants their compiler to
>> > always emit relevant multi-byte nops.
>> 
>>  What we need is an option to enable -fcf-function with multi-byte NOPs
>>  without -mcet which enables the full CET ISAs.  A configure option
>>  without the corresponding the command-line option makes test and
>>  debug difficult.   I can add
>> 
>>  --enable-cf-function-nop or --with-cf-function-nop
>> 
>>  with
>> 
>>  -fct-function-nop
>> 
>> >>>
>> >>> How about adding -mno-cet, which enables the NOP portion of CET
>> >>
>> >> I meant -mnop-cet, not -mno-cet.
>> >>
>> >
>> > Here is a patch to add -mnop and use it with -fcf-protection.
>>
>> +mnop
>> +Target Report Var(flag_nop) Init(0)
>> +Support multi-byte NOP code generation.
>>
>> the option name is incredibly bad and the documentation doesn't make it
>> better either.  The invoke.texi docs refer to duplicate {-mcet}.
>>
>> Isn't there a -fcf-protection sub-set that can be used to automatically
>> enable this?  Or simply do this mode by default when
>> -fcf-protection is used but neither -mcet nor -mibt is enabled?
>>
>
> Since multi-byte NOPs are used to implement -fcf-protection on x86, we
> propose a new design for -fcf-protection:
>
> 1. -fcf-protection option will report the unsupported error on non-x86
> platform. On x86 platform it's supported and inserts endbr-nop
> instructions and properties, depending on its value (full/branch/return)
> 2. -mcet/-mibt/-mshstk options control intrinsics only.
> 3. These options are independent and do not influence each other so no
> need for cross checking between them.
>
> OK for trunk?

I think it makes more sense this way, thanks for doing the change
(this isn't an approval).

Richard.

>
> H.J.
> 
> 
> -fcf-protection -mcet can't be used with IFUNC features, like symbol
> multiversioning or target clone, since IBT/SHSTK are applied to the whole
> program and they may be disabled in some functions.  But -fcf-protection
> is implemented with multi-byte NOPs on all 64-bit processors as well as
> 32-bit processors starting with Pentium Pro.  If -fcf-protection requires
> -mcet, IFUNC features can't be used on Linux when -fcf-protection is
> enabled by default.
>
> This patch changes -fcf-protection to implement indirect branch and
> return address tracking with multi-byte NOPs.  -mibt and -mshstk are
> changed to only CET built-in functions.  CET tests are updated to
> allow -fcf-protection without -mibt, -mshstk and -mcet on x86.
> -fcf-protection=none are also added to tests which fail with
> -fcf-protection so that -fcf-protection can be added to RUNTESTFLAGS
> to verify -fcf-protection implementation.
>
> gcc/
>
> PR target/85417
> * config/i386/cet.c (file_end_indicate_exec_stack_and_cet):
> Check flag_cf_protection instead of TARGET_IBT and TARGET_SHSTK.
> * config/i386/i386-c.c (ix86_target_macros_internal): Also
> define __IBT__ and __SHSTK__ for -fcf-protection.
> * config/i386/i386.c (pass_insert_endbranch::gate): Don't check
> TARGET_IBT.
> (ix86_trampoline_init): Likewise.
> (x86_output_mi_thunk): Likewise.
> (ix86_notrack_prefixed_insn_p): Likewise.
> (ix86_option_override_internal): Don't 

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

[Uros Bizjak]

On Thu, Apr 19, 2018 at 3:30 PM, H.J. Lu  wrote:
> On Wed, Apr 18, 2018 at 01:35:33PM +0200, Richard Biener wrote:
>> On Wed, Apr 18, 2018 at 1:24 PM, H.J. Lu  wrote:
>> > On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
>> >> On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
>> >>> On Tue, Apr 17, 2018 at 12:03 PM, H.J. Lu  wrote:
>>  On Tue, Apr 17, 2018 at 11:55 AM, Uros Bizjak  wrote:
>> > On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu  wrote:
>> >> -fcf-protection -mcet can't be used with IFUNC features, like symbol
>> >> multiversioning or target clone, since IBT/SHSTK are applied to the 
>> >> whole
>> >> program and they may be disabled in some functions.  But 
>> >> -fcf-protection
>> >> is implemented with multi-byte NOPs on all 64-bit processors as well 
>> >> as
>> >> 32-bit processors starting with Pentium Pro.  If -fcf-protection 
>> >> requires
>> >> -mcet, IFUNC features can't be used on Linux when -fcf-protection is
>> >> enabled by default.
>> >>
>> >> This patch changes -fcf-protection to to enable the NOP portion of CET
>> >> ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of CET
>> >> ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.
>> >>
>> >> OK for trunk?
>> >
>> > As said in the PR, NOP sequences have non-zero cost in the executable
>> > (they enlarge the executable), so I don't think this feature should be
>> > enabled by default.
>> >
>> > There is always a configure option if someone wants their compiler to
>> > always emit relevant multi-byte nops.
>> 
>>  What we need is an option to enable -fcf-function with multi-byte NOPs
>>  without -mcet which enables the full CET ISAs.  A configure option
>>  without the corresponding the command-line option makes test and
>>  debug difficult.   I can add
>> 
>>  --enable-cf-function-nop or --with-cf-function-nop
>> 
>>  with
>> 
>>  -fct-function-nop
>> 
>> >>>
>> >>> How about adding -mno-cet, which enables the NOP portion of CET
>> >>
>> >> I meant -mnop-cet, not -mno-cet.
>> >>
>> >
>> > Here is a patch to add -mnop and use it with -fcf-protection.
>>
>> +mnop
>> +Target Report Var(flag_nop) Init(0)
>> +Support multi-byte NOP code generation.
>>
>> the option name is incredibly bad and the documentation doesn't make it
>> better either.  The invoke.texi docs refer to duplicate {-mcet}.
>>
>> Isn't there a -fcf-protection sub-set that can be used to automatically
>> enable this?  Or simply do this mode by default when
>> -fcf-protection is used but neither -mcet nor -mibt is enabled?
>>
>
> Since multi-byte NOPs are used to implement -fcf-protection on x86, we
> propose a new design for -fcf-protection:
>
> 1. -fcf-protection option will report the unsupported error on non-x86
> platform. On x86 platform it's supported and inserts endbr-nop
> instructions and properties, depending on its value (full/branch/return)
> 2. -mcet/-mibt/-mshstk options control intrinsics only.
> 3. These options are independent and do not influence each other so no
> need for cross checking between them.
>
> OK for trunk?

This patch touches only CET related code, so Igor's OK should be enough.

Uros.

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

[H.J. Lu]

On Wed, Apr 18, 2018 at 01:35:33PM +0200, Richard Biener wrote:
> On Wed, Apr 18, 2018 at 1:24 PM, H.J. Lu  wrote:
> > On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
> >> On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
> >>> On Tue, Apr 17, 2018 at 12:03 PM, H.J. Lu  wrote:
>  On Tue, Apr 17, 2018 at 11:55 AM, Uros Bizjak  wrote:
> > On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu  wrote:
> >> -fcf-protection -mcet can't be used with IFUNC features, like symbol
> >> multiversioning or target clone, since IBT/SHSTK are applied to the 
> >> whole
> >> program and they may be disabled in some functions.  But 
> >> -fcf-protection
> >> is implemented with multi-byte NOPs on all 64-bit processors as well as
> >> 32-bit processors starting with Pentium Pro.  If -fcf-protection 
> >> requires
> >> -mcet, IFUNC features can't be used on Linux when -fcf-protection is
> >> enabled by default.
> >>
> >> This patch changes -fcf-protection to to enable the NOP portion of CET
> >> ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of CET
> >> ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.
> >>
> >> OK for trunk?
> >
> > As said in the PR, NOP sequences have non-zero cost in the executable
> > (they enlarge the executable), so I don't think this feature should be
> > enabled by default.
> >
> > There is always a configure option if someone wants their compiler to
> > always emit relevant multi-byte nops.
> 
>  What we need is an option to enable -fcf-function with multi-byte NOPs
>  without -mcet which enables the full CET ISAs.  A configure option
>  without the corresponding the command-line option makes test and
>  debug difficult.   I can add
> 
>  --enable-cf-function-nop or --with-cf-function-nop
> 
>  with
> 
>  -fct-function-nop
> 
> >>>
> >>> How about adding -mno-cet, which enables the NOP portion of CET
> >>
> >> I meant -mnop-cet, not -mno-cet.
> >>
> >
> > Here is a patch to add -mnop and use it with -fcf-protection.
> 
> +mnop
> +Target Report Var(flag_nop) Init(0)
> +Support multi-byte NOP code generation.
> 
> the option name is incredibly bad and the documentation doesn't make it
> better either.  The invoke.texi docs refer to duplicate {-mcet}.
> 
> Isn't there a -fcf-protection sub-set that can be used to automatically
> enable this?  Or simply do this mode by default when
> -fcf-protection is used but neither -mcet nor -mibt is enabled?
> 

Since multi-byte NOPs are used to implement -fcf-protection on x86, we
propose a new design for -fcf-protection:

1. -fcf-protection option will report the unsupported error on non-x86
platform. On x86 platform it's supported and inserts endbr-nop
instructions and properties, depending on its value (full/branch/return)
2. -mcet/-mibt/-mshstk options control intrinsics only. 
3. These options are independent and do not influence each other so no
need for cross checking between them.

OK for trunk?


H.J.


-fcf-protection -mcet can't be used with IFUNC features, like symbol
multiversioning or target clone, since IBT/SHSTK are applied to the whole
program and they may be disabled in some functions.  But -fcf-protection
is implemented with multi-byte NOPs on all 64-bit processors as well as
32-bit processors starting with Pentium Pro.  If -fcf-protection requires
-mcet, IFUNC features can't be used on Linux when -fcf-protection is
enabled by default.

This patch changes -fcf-protection to implement indirect branch and
return address tracking with multi-byte NOPs.  -mibt and -mshstk are
changed to only CET built-in functions.  CET tests are updated to
allow -fcf-protection without -mibt, -mshstk and -mcet on x86.
-fcf-protection=none are also added to tests which fail with
-fcf-protection so that -fcf-protection can be added to RUNTESTFLAGS
to verify -fcf-protection implementation.

gcc/

PR target/85417
* config/i386/cet.c (file_end_indicate_exec_stack_and_cet):
Check flag_cf_protection instead of TARGET_IBT and TARGET_SHSTK.
* config/i386/i386-c.c (ix86_target_macros_internal): Also
define __IBT__ and __SHSTK__ for -fcf-protection.
* config/i386/i386.c (pass_insert_endbranch::gate): Don't check
TARGET_IBT.
(ix86_trampoline_init): Likewise.
(x86_output_mi_thunk): Likewise.
(ix86_notrack_prefixed_insn_p): Likewise.
(ix86_option_override_internal): Don't disallow -fcf-protection.
* config/i386/i386.md (rdssp): Also enable for
-fcf-protection.
(incssp): Likewise.
(nop_endbr): Likewise.
* config/i386/i386.opt (mcet): Change help message to built-in
functions only.
(mibt): Likewise.
(mshstk): Likewise.
* 

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

[H.J. Lu]

On Wed, Apr 18, 2018 at 5:32 AM, Uros Bizjak  wrote:
> On Wed, Apr 18, 2018 at 2:09 PM, Jakub Jelinek  wrote:
>> On Wed, Apr 18, 2018 at 02:04:50PM +0200, Jakub Jelinek wrote:
>>> On Wed, Apr 18, 2018 at 04:57:41AM -0700, H.J. Lu wrote:
>>> > On Wed, Apr 18, 2018 at 4:55 AM, Uros Bizjak  wrote:
>>> > > On Wed, Apr 18, 2018 at 1:39 PM, H.J. Lu  wrote:
>>> > >
>>> >  Here is a patch to add -mnop and use it with -fcf-protection.
>>> > >>>
>>> > >>> +mnop
>>> > >>> +Target Report Var(flag_nop) Init(0)
>>> > >>> +Support multi-byte NOP code generation.
>>> > >>>
>>> > >>> the option name is incredibly bad and the documentation doesn't make 
>>> > >>> it
>>> > >>> better either.  The invoke.texi docs refer to duplicate {-mcet}.
>>> > >>>
>>> > >>> Isn't there a -fcf-protection sub-set that can be used to 
>>> > >>> automatically
>>> > >>> enable this?  Or simply do this mode by default when
>>> > >>> -fcf-protection is used but neither -mcet nor -mibt is enabled?
>>> > >>
>>> > >> Make -fcf-protection default to multi-byte NOPs works.  Uros,
>>> > >> should I prepare a patch?
>>> > >
>>> > > Please make it an opt-in feature, so the compiler won't litter the
>>> > > executable with unnecessary nops without user consent.
>>> > >
>>> >
>>> > -fcf-protection is off by default.  Users need to pass -fcf-protection
>>> > to enable it.  I will work on such a patch.
>>>
>>> That is not true.  When building gcc itself, config/cet.m4 makes
>>> -fcf-protection -mcet the default if assembler supports it.
>>> The request was to change --enable-cet configure option from having
>>> yes,no,default arguments with default autodetection and being a default
>>> if --enable-cet*/--disable-cet is not specified to say
>>> yes,no,auto arguments where no would be the default and auto would be the
>>> current default - enable it if as supports it, disable otherwise.
>>
>> So untested patch would be something like:
>
> Yes, this is what I think should be the most appropriate approach.
>
> Uros.
>
>> 2018-04-18  Jakub Jelinek  
>>
>> * config/cet.m4 (GCC_CET_FLAGS): Default to --disable-cet, replace
>> --enable-cet=default with --enable-cet=auto.
>>
>> * doc/install.texi: Document --disable-cet being the default and
>> --enable-cet=auto.
>>
>> --- gcc/config/cet.m4.jj2018-02-19 19:57:05.221280084 +0100
>> +++ gcc/config/cet.m4   2018-04-18 14:05:31.514859185 +0200
>> @@ -3,14 +3,14 @@ dnl GCC_CET_FLAGS
>>  dnl(SHELL-CODE_HANDLER)
>>  dnl
>>  AC_DEFUN([GCC_CET_FLAGS],[dnl
>> -GCC_ENABLE(cet, default, ,[enable Intel CET in target libraries],
>> -  permit yes|no|default)
>> +GCC_ENABLE(cet, no, ,[enable Intel CET in target libraries],
>> +  permit yes|no|auto)
>>  AC_MSG_CHECKING([for CET support])
>>
>>  case "$host" in
>>i[[34567]]86-*-linux* | x86_64-*-linux*)
>>  case "$enable_cet" in
>> -  default)
>> +  auto)
>> # Check if target supports multi-byte NOPs
>> # and if assembler supports CET insn.
>> AC_COMPILE_IFELSE(
>> --- gcc/doc/install.texi.jj 2018-02-08 12:21:20.791749480 +0100
>> +++ gcc/doc/install.texi2018-04-18 14:07:19.637901528 +0200
>> @@ -2103,10 +2103,11 @@ instrumentation, see @option{-fcf-protec
>>  to add @option{-fcf-protection} and, if needed, other target
>>  specific options to a set of building options.
>>
>> -The option is enabled by default on Linux/x86 if target binutils
>> -supports @code{Intel CET} instructions.  In this case the target
>> -libraries are configured to get additional @option{-fcf-protection}
>> -and @option{-mcet} options.
>> +The option is disabled by default on Linux/x86.  When
>> +@code{--enable-cet=auto} is used, it is enabled if target binutils
>> +supports @code{Intel CET} instructions and disabled otherwise.
>> +In this case the target libraries are configured to get additional
>> +@option{-fcf-protection} and @option{-mcet} options.
>>  @end table
>>
>>  @subheading Cross-Compiler-Specific Options
>>
>>
>> Jakub

Looks good to me.

Thanks.

-- 
H.J.

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

[Jakub Jelinek]

On Wed, Apr 18, 2018 at 04:57:41AM -0700, H.J. Lu wrote:
> On Wed, Apr 18, 2018 at 4:55 AM, Uros Bizjak  wrote:
> > On Wed, Apr 18, 2018 at 1:39 PM, H.J. Lu  wrote:
> >
>  Here is a patch to add -mnop and use it with -fcf-protection.
> >>>
> >>> +mnop
> >>> +Target Report Var(flag_nop) Init(0)
> >>> +Support multi-byte NOP code generation.
> >>>
> >>> the option name is incredibly bad and the documentation doesn't make it
> >>> better either.  The invoke.texi docs refer to duplicate {-mcet}.
> >>>
> >>> Isn't there a -fcf-protection sub-set that can be used to automatically
> >>> enable this?  Or simply do this mode by default when
> >>> -fcf-protection is used but neither -mcet nor -mibt is enabled?
> >>
> >> Make -fcf-protection default to multi-byte NOPs works.  Uros,
> >> should I prepare a patch?
> >
> > Please make it an opt-in feature, so the compiler won't litter the
> > executable with unnecessary nops without user consent.
> >
> 
> -fcf-protection is off by default.  Users need to pass -fcf-protection
> to enable it.  I will work on such a patch.

That is not true.  When building gcc itself, config/cet.m4 makes
-fcf-protection -mcet the default if assembler supports it.
The request was to change --enable-cet configure option from having
yes,no,default arguments with default autodetection and being a default
if --enable-cet*/--disable-cet is not specified to say
yes,no,auto arguments where no would be the default and auto would be the
current default - enable it if as supports it, disable otherwise.

Jakub

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

[Uros Bizjak]

On Wed, Apr 18, 2018 at 2:09 PM, Jakub Jelinek  wrote:
> On Wed, Apr 18, 2018 at 02:04:50PM +0200, Jakub Jelinek wrote:
>> On Wed, Apr 18, 2018 at 04:57:41AM -0700, H.J. Lu wrote:
>> > On Wed, Apr 18, 2018 at 4:55 AM, Uros Bizjak  wrote:
>> > > On Wed, Apr 18, 2018 at 1:39 PM, H.J. Lu  wrote:
>> > >
>> >  Here is a patch to add -mnop and use it with -fcf-protection.
>> > >>>
>> > >>> +mnop
>> > >>> +Target Report Var(flag_nop) Init(0)
>> > >>> +Support multi-byte NOP code generation.
>> > >>>
>> > >>> the option name is incredibly bad and the documentation doesn't make it
>> > >>> better either.  The invoke.texi docs refer to duplicate {-mcet}.
>> > >>>
>> > >>> Isn't there a -fcf-protection sub-set that can be used to automatically
>> > >>> enable this?  Or simply do this mode by default when
>> > >>> -fcf-protection is used but neither -mcet nor -mibt is enabled?
>> > >>
>> > >> Make -fcf-protection default to multi-byte NOPs works.  Uros,
>> > >> should I prepare a patch?
>> > >
>> > > Please make it an opt-in feature, so the compiler won't litter the
>> > > executable with unnecessary nops without user consent.
>> > >
>> >
>> > -fcf-protection is off by default.  Users need to pass -fcf-protection
>> > to enable it.  I will work on such a patch.
>>
>> That is not true.  When building gcc itself, config/cet.m4 makes
>> -fcf-protection -mcet the default if assembler supports it.
>> The request was to change --enable-cet configure option from having
>> yes,no,default arguments with default autodetection and being a default
>> if --enable-cet*/--disable-cet is not specified to say
>> yes,no,auto arguments where no would be the default and auto would be the
>> current default - enable it if as supports it, disable otherwise.
>
> So untested patch would be something like:

Yes, this is what I think should be the most appropriate approach.

Uros.

> 2018-04-18  Jakub Jelinek  
>
> * config/cet.m4 (GCC_CET_FLAGS): Default to --disable-cet, replace
> --enable-cet=default with --enable-cet=auto.
>
> * doc/install.texi: Document --disable-cet being the default and
> --enable-cet=auto.
>
> --- gcc/config/cet.m4.jj2018-02-19 19:57:05.221280084 +0100
> +++ gcc/config/cet.m4   2018-04-18 14:05:31.514859185 +0200
> @@ -3,14 +3,14 @@ dnl GCC_CET_FLAGS
>  dnl(SHELL-CODE_HANDLER)
>  dnl
>  AC_DEFUN([GCC_CET_FLAGS],[dnl
> -GCC_ENABLE(cet, default, ,[enable Intel CET in target libraries],
> -  permit yes|no|default)
> +GCC_ENABLE(cet, no, ,[enable Intel CET in target libraries],
> +  permit yes|no|auto)
>  AC_MSG_CHECKING([for CET support])
>
>  case "$host" in
>i[[34567]]86-*-linux* | x86_64-*-linux*)
>  case "$enable_cet" in
> -  default)
> +  auto)
> # Check if target supports multi-byte NOPs
> # and if assembler supports CET insn.
> AC_COMPILE_IFELSE(
> --- gcc/doc/install.texi.jj 2018-02-08 12:21:20.791749480 +0100
> +++ gcc/doc/install.texi2018-04-18 14:07:19.637901528 +0200
> @@ -2103,10 +2103,11 @@ instrumentation, see @option{-fcf-protec
>  to add @option{-fcf-protection} and, if needed, other target
>  specific options to a set of building options.
>
> -The option is enabled by default on Linux/x86 if target binutils
> -supports @code{Intel CET} instructions.  In this case the target
> -libraries are configured to get additional @option{-fcf-protection}
> -and @option{-mcet} options.
> +The option is disabled by default on Linux/x86.  When
> +@code{--enable-cet=auto} is used, it is enabled if target binutils
> +supports @code{Intel CET} instructions and disabled otherwise.
> +In this case the target libraries are configured to get additional
> +@option{-fcf-protection} and @option{-mcet} options.
>  @end table
>
>  @subheading Cross-Compiler-Specific Options
>
>
> Jakub

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

[Tsimbalist, Igor V]

> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> ow...@gcc.gnu.org] On Behalf Of Jakub Jelinek
> Sent: Wednesday, April 18, 2018 2:10 PM
> To: H.J. Lu <hjl.to...@gmail.com>
> Cc: Uros Bizjak <ubiz...@gmail.com>; Richard Biener
> <richard.guent...@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 Wed, Apr 18, 2018 at 02:04:50PM +0200, Jakub Jelinek wrote:
> > On Wed, Apr 18, 2018 at 04:57:41AM -0700, H.J. Lu wrote:
> > > On Wed, Apr 18, 2018 at 4:55 AM, Uros Bizjak <ubiz...@gmail.com>
> wrote:
> > > > On Wed, Apr 18, 2018 at 1:39 PM, H.J. Lu <hjl.to...@gmail.com>
> wrote:
> > > >
> > > >>>> Here is a patch to add -mnop and use it with -fcf-protection.
> > > >>>
> > > >>> +mnop
> > > >>> +Target Report Var(flag_nop) Init(0)
> > > >>> +Support multi-byte NOP code generation.
> > > >>>
> > > >>> the option name is incredibly bad and the documentation doesn't
> make it
> > > >>> better either.  The invoke.texi docs refer to duplicate {-mcet}.
> > > >>>
> > > >>> Isn't there a -fcf-protection sub-set that can be used to
> automatically
> > > >>> enable this?  Or simply do this mode by default when
> > > >>> -fcf-protection is used but neither -mcet nor -mibt is enabled?
> > > >>
> > > >> Make -fcf-protection default to multi-byte NOPs works.  Uros,
> > > >> should I prepare a patch?
> > > >
> > > > Please make it an opt-in feature, so the compiler won't litter the
> > > > executable with unnecessary nops without user consent.
> > > >
> > >
> > > -fcf-protection is off by default.  Users need to pass -fcf-protection
> > > to enable it.  I will work on such a patch.
> >
> > That is not true.  When building gcc itself, config/cet.m4 makes
> > -fcf-protection -mcet the default if assembler supports it.
> > The request was to change --enable-cet configure option from having
> > yes,no,default arguments with default autodetection and being a default
> > if --enable-cet*/--disable-cet is not specified to say
> > yes,no,auto arguments where no would be the default and auto would be
> the
> > current default - enable it if as supports it, disable otherwise.
> 
> So untested patch would be something like:
> 
> 2018-04-18  Jakub Jelinek  <ja...@redhat.com>
> 
>   * config/cet.m4 (GCC_CET_FLAGS): Default to --disable-cet, replace
>   --enable-cet=default with --enable-cet=auto.
> 
>   * doc/install.texi: Document --disable-cet being the default and
>   --enable-cet=auto.
> 
> --- gcc/config/cet.m4.jj  2018-02-19 19:57:05.221280084 +0100
> +++ gcc/config/cet.m4 2018-04-18 14:05:31.514859185 +0200
> @@ -3,14 +3,14 @@ dnl GCC_CET_FLAGS
>  dnl(SHELL-CODE_HANDLER)
>  dnl
>  AC_DEFUN([GCC_CET_FLAGS],[dnl
> -GCC_ENABLE(cet, default, ,[enable Intel CET in target libraries],
> -permit yes|no|default)
> +GCC_ENABLE(cet, no, ,[enable Intel CET in target libraries],
> +permit yes|no|auto)
>  AC_MSG_CHECKING([for CET support])
> 
>  case "$host" in
>i[[34567]]86-*-linux* | x86_64-*-linux*)
>  case "$enable_cet" in
> -  default)
> +  auto)
>   # Check if target supports multi-byte NOPs
>   # and if assembler supports CET insn.
>   AC_COMPILE_IFELSE(
> --- gcc/doc/install.texi.jj   2018-02-08 12:21:20.791749480 +0100
> +++ gcc/doc/install.texi  2018-04-18 14:07:19.637901528 +0200
> @@ -2103,10 +2103,11 @@ instrumentation, see @option{-fcf-protec
>  to add @option{-fcf-protection} and, if needed, other target
>  specific options to a set of building options.
> 
> -The option is enabled by default on Linux/x86 if target binutils
> -supports @code{Intel CET} instructions.  In this case the target
> -libraries are configured to get additional @option{-fcf-protection}
> -and @option{-mcet} options.
> +The option is disabled by default on Linux/x86.  When
> +@code{--enable-cet=auto} is used, it is enabled if target binutils
> +supports @code{Intel CET} instructions and disabled otherwise.
> +In this case the target libraries are configured to get additional
> +@option{-fcf-protection} and @option{-mcet} options.
>  @end table
> 
>  @subheading Cross-Compiler-Specific Options
> 

Thanks! I will work on this.

>   Jakub

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

[Tsimbalist, Igor V]

> -Original Message-
> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
> ow...@gcc.gnu.org] On Behalf Of H.J. Lu
> Sent: Wednesday, April 18, 2018 1:39 PM
> To: Richard Biener <richard.guent...@gmail.com>
> Cc: 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 Wed, Apr 18, 2018 at 4:35 AM, Richard Biener
> <richard.guent...@gmail.com> wrote:
> > On Wed, Apr 18, 2018 at 1:24 PM, H.J. Lu <hjl.to...@gmail.com> wrote:
> >> On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu <hjl.to...@gmail.com> wrote:
> >>> On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu <hjl.to...@gmail.com>
> wrote:
> >>>> On Tue, Apr 17, 2018 at 12:03 PM, H.J. Lu <hjl.to...@gmail.com>
> wrote:
> >>>>> On Tue, Apr 17, 2018 at 11:55 AM, Uros Bizjak
> <ubiz...@gmail.com> wrote:
> >>>>>> On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu <hongjiu...@intel.com>
> wrote:
> >>>>>>> -fcf-protection -mcet can't be used with IFUNC features, like
> symbol
> >>>>>>> multiversioning or target clone, since IBT/SHSTK are applied to
> the whole
> >>>>>>> program and they may be disabled in some functions.  But -fcf-
> protection
> >>>>>>> is implemented with multi-byte NOPs on all 64-bit processors as
> well as
> >>>>>>> 32-bit processors starting with Pentium Pro.  If -fcf-protection
> requires
> >>>>>>> -mcet, IFUNC features can't be used on Linux when -fcf-
> protection is
> >>>>>>> enabled by default.
> >>>>>>>
> >>>>>>> This patch changes -fcf-protection to to enable the NOP portion
> of CET
> >>>>>>> ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of
> CET
> >>>>>>> ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.
> >>>>>>>
> >>>>>>> OK for trunk?
> >>>>>>
> >>>>>> As said in the PR, NOP sequences have non-zero cost in the
> executable
> >>>>>> (they enlarge the executable), so I don't think this feature should
> be
> >>>>>> enabled by default.
> >>>>>>
> >>>>>> There is always a configure option if someone wants their compiler
> to
> >>>>>> always emit relevant multi-byte nops.
> >>>>>
> >>>>> What we need is an option to enable -fcf-function with multi-byte
> NOPs
> >>>>> without -mcet which enables the full CET ISAs.  A configure option
> >>>>> without the corresponding the command-line option makes test and
> >>>>> debug difficult.   I can add
> >>>>>
> >>>>> --enable-cf-function-nop or --with-cf-function-nop
> >>>>>
> >>>>> with
> >>>>>
> >>>>> -fct-function-nop
> >>>>>
> >>>>
> >>>> How about adding -mno-cet, which enables the NOP portion of CET
> >>>
> >>> I meant -mnop-cet, not -mno-cet.
> >>>
> >>
> >> Here is a patch to add -mnop and use it with -fcf-protection.
> >
> > +mnop
> > +Target Report Var(flag_nop) Init(0)
> > +Support multi-byte NOP code generation.
> >
> > the option name is incredibly bad and the documentation doesn't make it
> > better either.  The invoke.texi docs refer to duplicate {-mcet}.
> >
> > Isn't there a -fcf-protection sub-set that can be used to automatically
> > enable this?  Or simply do this mode by default when
> > -fcf-protection is used but neither -mcet nor -mibt is enabled?
> 
> Make -fcf-protection default to multi-byte NOPs works.  Uros,
> should I prepare a patch?

This is going to change the designed approach and has to be communicated 
to/agreed
with other compilers. And I assume there will be no extra option introduced, 
like -mnop.

Igor

> --
> H.J.

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

[H.J. Lu]

On Wed, Apr 18, 2018 at 5:08 AM, Uros Bizjak  wrote:
> On Wed, Apr 18, 2018 at 1:57 PM, H.J. Lu  wrote:
>> On Wed, Apr 18, 2018 at 4:55 AM, Uros Bizjak  wrote:
>>> On Wed, Apr 18, 2018 at 1:39 PM, H.J. Lu  wrote:
>>>
>> Here is a patch to add -mnop and use it with -fcf-protection.
>
> +mnop
> +Target Report Var(flag_nop) Init(0)
> +Support multi-byte NOP code generation.
>
> the option name is incredibly bad and the documentation doesn't make it
> better either.  The invoke.texi docs refer to duplicate {-mcet}.
>
> Isn't there a -fcf-protection sub-set that can be used to automatically
> enable this?  Or simply do this mode by default when
> -fcf-protection is used but neither -mcet nor -mibt is enabled?

 Make -fcf-protection default to multi-byte NOPs works.  Uros,
 should I prepare a patch?
>>>
>>> Please make it an opt-in feature, so the compiler won't litter the
>>> executable with unnecessary nops without user consent.
>>>
>>
>> -fcf-protection is off by default.  Users need to pass -fcf-protection
>> to enable it.  I will work on such a patch.
>
> Please note that currently all libraries are compiled with
> "-fcf-protection -mcet" by default, even without using --enable-cet
> during configure. The CET instrumentation of libraries should be put
> under strict user control, so please remove the "default" from
> config/cet.m4.
>

Igor, please prepare such a patch to config/cet.m4.


-- 
H.J.

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

[Jakub Jelinek]

On Wed, Apr 18, 2018 at 02:04:50PM +0200, Jakub Jelinek wrote:
> On Wed, Apr 18, 2018 at 04:57:41AM -0700, H.J. Lu wrote:
> > On Wed, Apr 18, 2018 at 4:55 AM, Uros Bizjak  wrote:
> > > On Wed, Apr 18, 2018 at 1:39 PM, H.J. Lu  wrote:
> > >
> >  Here is a patch to add -mnop and use it with -fcf-protection.
> > >>>
> > >>> +mnop
> > >>> +Target Report Var(flag_nop) Init(0)
> > >>> +Support multi-byte NOP code generation.
> > >>>
> > >>> the option name is incredibly bad and the documentation doesn't make it
> > >>> better either.  The invoke.texi docs refer to duplicate {-mcet}.
> > >>>
> > >>> Isn't there a -fcf-protection sub-set that can be used to automatically
> > >>> enable this?  Or simply do this mode by default when
> > >>> -fcf-protection is used but neither -mcet nor -mibt is enabled?
> > >>
> > >> Make -fcf-protection default to multi-byte NOPs works.  Uros,
> > >> should I prepare a patch?
> > >
> > > Please make it an opt-in feature, so the compiler won't litter the
> > > executable with unnecessary nops without user consent.
> > >
> > 
> > -fcf-protection is off by default.  Users need to pass -fcf-protection
> > to enable it.  I will work on such a patch.
> 
> That is not true.  When building gcc itself, config/cet.m4 makes
> -fcf-protection -mcet the default if assembler supports it.
> The request was to change --enable-cet configure option from having
> yes,no,default arguments with default autodetection and being a default
> if --enable-cet*/--disable-cet is not specified to say
> yes,no,auto arguments where no would be the default and auto would be the
> current default - enable it if as supports it, disable otherwise.

So untested patch would be something like:

2018-04-18  Jakub Jelinek  

* config/cet.m4 (GCC_CET_FLAGS): Default to --disable-cet, replace
--enable-cet=default with --enable-cet=auto.

* doc/install.texi: Document --disable-cet being the default and
--enable-cet=auto.

--- gcc/config/cet.m4.jj2018-02-19 19:57:05.221280084 +0100
+++ gcc/config/cet.m4   2018-04-18 14:05:31.514859185 +0200
@@ -3,14 +3,14 @@ dnl GCC_CET_FLAGS
 dnl(SHELL-CODE_HANDLER)
 dnl
 AC_DEFUN([GCC_CET_FLAGS],[dnl
-GCC_ENABLE(cet, default, ,[enable Intel CET in target libraries],
-  permit yes|no|default)
+GCC_ENABLE(cet, no, ,[enable Intel CET in target libraries],
+  permit yes|no|auto)
 AC_MSG_CHECKING([for CET support])
 
 case "$host" in
   i[[34567]]86-*-linux* | x86_64-*-linux*)
 case "$enable_cet" in
-  default)
+  auto)
# Check if target supports multi-byte NOPs
# and if assembler supports CET insn.
AC_COMPILE_IFELSE(
--- gcc/doc/install.texi.jj 2018-02-08 12:21:20.791749480 +0100
+++ gcc/doc/install.texi2018-04-18 14:07:19.637901528 +0200
@@ -2103,10 +2103,11 @@ instrumentation, see @option{-fcf-protec
 to add @option{-fcf-protection} and, if needed, other target
 specific options to a set of building options.
 
-The option is enabled by default on Linux/x86 if target binutils
-supports @code{Intel CET} instructions.  In this case the target
-libraries are configured to get additional @option{-fcf-protection}
-and @option{-mcet} options.
+The option is disabled by default on Linux/x86.  When
+@code{--enable-cet=auto} is used, it is enabled if target binutils
+supports @code{Intel CET} instructions and disabled otherwise.
+In this case the target libraries are configured to get additional
+@option{-fcf-protection} and @option{-mcet} options.
 @end table
 
 @subheading Cross-Compiler-Specific Options


Jakub

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

[Uros Bizjak]

On Wed, Apr 18, 2018 at 1:57 PM, H.J. Lu  wrote:
> On Wed, Apr 18, 2018 at 4:55 AM, Uros Bizjak  wrote:
>> On Wed, Apr 18, 2018 at 1:39 PM, H.J. Lu  wrote:
>>
> Here is a patch to add -mnop and use it with -fcf-protection.

 +mnop
 +Target Report Var(flag_nop) Init(0)
 +Support multi-byte NOP code generation.

 the option name is incredibly bad and the documentation doesn't make it
 better either.  The invoke.texi docs refer to duplicate {-mcet}.

 Isn't there a -fcf-protection sub-set that can be used to automatically
 enable this?  Or simply do this mode by default when
 -fcf-protection is used but neither -mcet nor -mibt is enabled?
>>>
>>> Make -fcf-protection default to multi-byte NOPs works.  Uros,
>>> should I prepare a patch?
>>
>> Please make it an opt-in feature, so the compiler won't litter the
>> executable with unnecessary nops without user consent.
>>
>
> -fcf-protection is off by default.  Users need to pass -fcf-protection
> to enable it.  I will work on such a patch.

Please note that currently all libraries are compiled with
"-fcf-protection -mcet" by default, even without using --enable-cet
during configure. The CET instrumentation of libraries should be put
under strict user control, so please remove the "default" from
config/cet.m4.

Uros.

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

[H.J. Lu]

On Wed, Apr 18, 2018 at 4:55 AM, Uros Bizjak  wrote:
> On Wed, Apr 18, 2018 at 1:39 PM, H.J. Lu  wrote:
>
 Here is a patch to add -mnop and use it with -fcf-protection.
>>>
>>> +mnop
>>> +Target Report Var(flag_nop) Init(0)
>>> +Support multi-byte NOP code generation.
>>>
>>> the option name is incredibly bad and the documentation doesn't make it
>>> better either.  The invoke.texi docs refer to duplicate {-mcet}.
>>>
>>> Isn't there a -fcf-protection sub-set that can be used to automatically
>>> enable this?  Or simply do this mode by default when
>>> -fcf-protection is used but neither -mcet nor -mibt is enabled?
>>
>> Make -fcf-protection default to multi-byte NOPs works.  Uros,
>> should I prepare a patch?
>
> Please make it an opt-in feature, so the compiler won't litter the
> executable with unnecessary nops without user consent.
>

-fcf-protection is off by default.  Users need to pass -fcf-protection
to enable it.  I will work on such a patch.

Thanks.

-- 
H.J.

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

[Uros Bizjak]

On Wed, Apr 18, 2018 at 1:39 PM, H.J. Lu  wrote:

>>> Here is a patch to add -mnop and use it with -fcf-protection.
>>
>> +mnop
>> +Target Report Var(flag_nop) Init(0)
>> +Support multi-byte NOP code generation.
>>
>> the option name is incredibly bad and the documentation doesn't make it
>> better either.  The invoke.texi docs refer to duplicate {-mcet}.
>>
>> Isn't there a -fcf-protection sub-set that can be used to automatically
>> enable this?  Or simply do this mode by default when
>> -fcf-protection is used but neither -mcet nor -mibt is enabled?
>
> Make -fcf-protection default to multi-byte NOPs works.  Uros,
> should I prepare a patch?

Please make it an opt-in feature, so the compiler won't litter the
executable with unnecessary nops without user consent.

Uros.

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

[H.J. Lu]

On Wed, Apr 18, 2018 at 4:35 AM, Richard Biener
 wrote:
> On Wed, Apr 18, 2018 at 1:24 PM, H.J. Lu  wrote:
>> On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
>>> On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
 On Tue, Apr 17, 2018 at 12:03 PM, H.J. Lu  wrote:
> On Tue, Apr 17, 2018 at 11:55 AM, Uros Bizjak  wrote:
>> On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu  wrote:
>>> -fcf-protection -mcet can't be used with IFUNC features, like symbol
>>> multiversioning or target clone, since IBT/SHSTK are applied to the 
>>> whole
>>> program and they may be disabled in some functions.  But -fcf-protection
>>> is implemented with multi-byte NOPs on all 64-bit processors as well as
>>> 32-bit processors starting with Pentium Pro.  If -fcf-protection 
>>> requires
>>> -mcet, IFUNC features can't be used on Linux when -fcf-protection is
>>> enabled by default.
>>>
>>> This patch changes -fcf-protection to to enable the NOP portion of CET
>>> ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of CET
>>> ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.
>>>
>>> OK for trunk?
>>
>> As said in the PR, NOP sequences have non-zero cost in the executable
>> (they enlarge the executable), so I don't think this feature should be
>> enabled by default.
>>
>> There is always a configure option if someone wants their compiler to
>> always emit relevant multi-byte nops.
>
> What we need is an option to enable -fcf-function with multi-byte NOPs
> without -mcet which enables the full CET ISAs.  A configure option
> without the corresponding the command-line option makes test and
> debug difficult.   I can add
>
> --enable-cf-function-nop or --with-cf-function-nop
>
> with
>
> -fct-function-nop
>

 How about adding -mno-cet, which enables the NOP portion of CET
>>>
>>> I meant -mnop-cet, not -mno-cet.
>>>
>>
>> Here is a patch to add -mnop and use it with -fcf-protection.
>
> +mnop
> +Target Report Var(flag_nop) Init(0)
> +Support multi-byte NOP code generation.
>
> the option name is incredibly bad and the documentation doesn't make it
> better either.  The invoke.texi docs refer to duplicate {-mcet}.
>
> Isn't there a -fcf-protection sub-set that can be used to automatically
> enable this?  Or simply do this mode by default when
> -fcf-protection is used but neither -mcet nor -mibt is enabled?

Make -fcf-protection default to multi-byte NOPs works.  Uros,
should I prepare a patch?

-- 
H.J.

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

[Richard Biener]

On Wed, Apr 18, 2018 at 1:24 PM, H.J. Lu  wrote:
> On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
>> On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
>>> On Tue, Apr 17, 2018 at 12:03 PM, H.J. Lu  wrote:
 On Tue, Apr 17, 2018 at 11:55 AM, Uros Bizjak  wrote:
> On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu  wrote:
>> -fcf-protection -mcet can't be used with IFUNC features, like symbol
>> multiversioning or target clone, since IBT/SHSTK are applied to the whole
>> program and they may be disabled in some functions.  But -fcf-protection
>> is implemented with multi-byte NOPs on all 64-bit processors as well as
>> 32-bit processors starting with Pentium Pro.  If -fcf-protection requires
>> -mcet, IFUNC features can't be used on Linux when -fcf-protection is
>> enabled by default.
>>
>> This patch changes -fcf-protection to to enable the NOP portion of CET
>> ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of CET
>> ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.
>>
>> OK for trunk?
>
> As said in the PR, NOP sequences have non-zero cost in the executable
> (they enlarge the executable), so I don't think this feature should be
> enabled by default.
>
> There is always a configure option if someone wants their compiler to
> always emit relevant multi-byte nops.

 What we need is an option to enable -fcf-function with multi-byte NOPs
 without -mcet which enables the full CET ISAs.  A configure option
 without the corresponding the command-line option makes test and
 debug difficult.   I can add

 --enable-cf-function-nop or --with-cf-function-nop

 with

 -fct-function-nop

>>>
>>> How about adding -mno-cet, which enables the NOP portion of CET
>>
>> I meant -mnop-cet, not -mno-cet.
>>
>
> Here is a patch to add -mnop and use it with -fcf-protection.

+mnop
+Target Report Var(flag_nop) Init(0)
+Support multi-byte NOP code generation.

the option name is incredibly bad and the documentation doesn't make it
better either.  The invoke.texi docs refer to duplicate {-mcet}.

Isn't there a -fcf-protection sub-set that can be used to automatically
enable this?  Or simply do this mode by default when
-fcf-protection is used but neither -mcet nor -mibt is enabled?

>
> --
> H.J.

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

[H.J. Lu]

On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
> On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
>> On Tue, Apr 17, 2018 at 12:03 PM, H.J. Lu  wrote:
>>> On Tue, Apr 17, 2018 at 11:55 AM, Uros Bizjak  wrote:
 On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu  wrote:
> -fcf-protection -mcet can't be used with IFUNC features, like symbol
> multiversioning or target clone, since IBT/SHSTK are applied to the whole
> program and they may be disabled in some functions.  But -fcf-protection
> is implemented with multi-byte NOPs on all 64-bit processors as well as
> 32-bit processors starting with Pentium Pro.  If -fcf-protection requires
> -mcet, IFUNC features can't be used on Linux when -fcf-protection is
> enabled by default.
>
> This patch changes -fcf-protection to to enable the NOP portion of CET
> ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of CET
> ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.
>
> OK for trunk?

 As said in the PR, NOP sequences have non-zero cost in the executable
 (they enlarge the executable), so I don't think this feature should be
 enabled by default.

 There is always a configure option if someone wants their compiler to
 always emit relevant multi-byte nops.
>>>
>>> What we need is an option to enable -fcf-function with multi-byte NOPs
>>> without -mcet which enables the full CET ISAs.  A configure option
>>> without the corresponding the command-line option makes test and
>>> debug difficult.   I can add
>>>
>>> --enable-cf-function-nop or --with-cf-function-nop
>>>
>>> with
>>>
>>> -fct-function-nop
>>>
>>
>> How about adding -mno-cet, which enables the NOP portion of CET
>
> I meant -mnop-cet, not -mno-cet.
>

Here is a patch to add -mnop and use it with -fcf-protection.


-- 
H.J.
From cb4446aa122dfb7711240612c013e4cdbd73971f Mon Sep 17 00:00:00 2001
From: "H.J. Lu" 
Date: Mon, 16 Apr 2018 14:13:10 -0700
Subject: [PATCH] x86: Allow -fcf-protection with multi-byte NOPs

-fcf-protection -mcet can't be used with IFUNC features, like symbol
multiversioning or target clone, since IBT/SHSTK are applied to the whole
program and they may be disabled in some functions.  But -fcf-protection
is implemented with multi-byte NOPs on all 64-bit processors as well as
32-bit processors starting with Pentium Pro.  If -fcf-protection requires
-mcet, IFUNC features can't be used on Linux when -fcf-protection is
enabled by default.

This patch adds -mnop to enable multi-byte NOP code generation which
can be used with -fcf-protection to implement indirect branch and return
address tracking without -mcet.

gcc/

	PR target/85417
	* config/i386/cet.c (file_end_indicate_exec_stack_and_cet):
	Check flag_cf_protection instead of TARGET_IBT and TARGET_SHSTK.
	* config/i386/i386.c (pass_insert_endbranch::gate): Don't check
	TARGET_IBT.
	(ix86_option_override_internal): Allow  -fcf-protection with
	multi-byte NOPs unless CET is disabled explicitly.
	(ix86_trampoline_init): Don't check TARGET_IBT.
	(x86_output_mi_thunk): Likewise.
	(ix86_notrack_prefixed_insn_p): Likewise.
	* config/i386/i386.md (rdssp): Also enable for flag_nop.
	(incssp): Likewise.
	(nop_endbr): Likewise.
	* config/i386/i386.opt: Add -mnop.
	* doc/invoke.texi: Document -mnop.

gcc/testsuite/

	PR target/85417
	* c-c++-common/attr-nocf-check-1.c: Compile with
	-fcf-protection=none.
	* c-c++-common/attr-nocf-check-3.c: Likewise.
	* gcc.dg/march-generic.c: Likewise.
	* gcc.target/i386/align-limit.c: Likewise.
	* c-c++-common/fcf-protection-1.c: Compile with -mno-nop for
	x86 targets.
	* c-c++-common/fcf-protection-2.c: Likewise.
	* c-c++-common/fcf-protection-3.c: Likewise.
	* c-c++-common/fcf-protection-5.c: Likewise.
	* c-c++-common/fcf-protection-6.c: Likewise.
	* c-c++-common/fcf-protection-7.c: Likewise.
	* gcc.target/i386/cet-notrack-icf-1.c: Compile with
	-fcf-protection=none.
	* gcc.target/i386/cet-notrack-icf-3.c: Likewise.
	* gcc.target/i386/cet-property-2.c: Likewise.
	* gcc.target/i386/indirect-thunk-attr-7.c: Likewise.
	* gcc.target/i386/indirect-thunk-extern-7.c: Likewise.
	* gcc.target/i386/ret-thunk-26.c: Likewise.
	* gcc.target/i386/cet-label-3.c: New test.
	* gcc.target/i386/cet-property-3.c: Likewise.
	* gcc.target/i386/cet-sjlj-7.c: Likewise.
	* gcc.target/i386/pr85417-1.c: Likewise.
	* gcc.target/i386/pr85417-2.c: Likewise.
	* gcc.target/i386/pr85403.c: Compile with -mno-nop.
---
 gcc/config/i386/cet.c  |  4 +-
 gcc/config/i386/i386.c | 23 +++
 gcc/config/i386/i386.md|  6 +--
 gcc/config/i386/i386.opt   |  4 ++
 gcc/doc/invoke.texi| 14 ++-
 gcc/testsuite/c-c++-common/attr-nocf-check-1.c |  1 +
 

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

[H.J. Lu]

On Tue, Apr 17, 2018 at 12:03 PM, H.J. Lu  wrote:
> On Tue, Apr 17, 2018 at 11:55 AM, Uros Bizjak  wrote:
>> On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu  wrote:
>>> -fcf-protection -mcet can't be used with IFUNC features, like symbol
>>> multiversioning or target clone, since IBT/SHSTK are applied to the whole
>>> program and they may be disabled in some functions.  But -fcf-protection
>>> is implemented with multi-byte NOPs on all 64-bit processors as well as
>>> 32-bit processors starting with Pentium Pro.  If -fcf-protection requires
>>> -mcet, IFUNC features can't be used on Linux when -fcf-protection is
>>> enabled by default.
>>>
>>> This patch changes -fcf-protection to to enable the NOP portion of CET
>>> ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of CET
>>> ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.
>>>
>>> OK for trunk?
>>
>> As said in the PR, NOP sequences have non-zero cost in the executable
>> (they enlarge the executable), so I don't think this feature should be
>> enabled by default.
>>
>> There is always a configure option if someone wants their compiler to
>> always emit relevant multi-byte nops.
>
> What we need is an option to enable -fcf-function with multi-byte NOPs
> without -mcet which enables the full CET ISAs.  A configure option
> without the corresponding the command-line option makes test and
> debug difficult.   I can add
>
> --enable-cf-function-nop or --with-cf-function-nop
>
> with
>
> -fct-function-nop
>

How about adding -mno-cet, which enables the NOP portion of CET
ISAs?

-- 
H.J.

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

[H.J. Lu]

On Tue, Apr 17, 2018 at 12:25 PM, H.J. Lu  wrote:
> On Tue, Apr 17, 2018 at 12:03 PM, H.J. Lu  wrote:
>> On Tue, Apr 17, 2018 at 11:55 AM, Uros Bizjak  wrote:
>>> On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu  wrote:
 -fcf-protection -mcet can't be used with IFUNC features, like symbol
 multiversioning or target clone, since IBT/SHSTK are applied to the whole
 program and they may be disabled in some functions.  But -fcf-protection
 is implemented with multi-byte NOPs on all 64-bit processors as well as
 32-bit processors starting with Pentium Pro.  If -fcf-protection requires
 -mcet, IFUNC features can't be used on Linux when -fcf-protection is
 enabled by default.

 This patch changes -fcf-protection to to enable the NOP portion of CET
 ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of CET
 ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.

 OK for trunk?
>>>
>>> As said in the PR, NOP sequences have non-zero cost in the executable
>>> (they enlarge the executable), so I don't think this feature should be
>>> enabled by default.
>>>
>>> There is always a configure option if someone wants their compiler to
>>> always emit relevant multi-byte nops.
>>
>> What we need is an option to enable -fcf-function with multi-byte NOPs
>> without -mcet which enables the full CET ISAs.  A configure option
>> without the corresponding the command-line option makes test and
>> debug difficult.   I can add
>>
>> --enable-cf-function-nop or --with-cf-function-nop
>>
>> with
>>
>> -fct-function-nop
>>
>
> How about adding -mno-cet, which enables the NOP portion of CET

I meant -mnop-cet, not -mno-cet.

> ISAs?
>
> --
> H.J.



-- 
H.J.

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

[H.J. Lu]

On Tue, Apr 17, 2018 at 11:55 AM, Uros Bizjak  wrote:
> On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu  wrote:
>> -fcf-protection -mcet can't be used with IFUNC features, like symbol
>> multiversioning or target clone, since IBT/SHSTK are applied to the whole
>> program and they may be disabled in some functions.  But -fcf-protection
>> is implemented with multi-byte NOPs on all 64-bit processors as well as
>> 32-bit processors starting with Pentium Pro.  If -fcf-protection requires
>> -mcet, IFUNC features can't be used on Linux when -fcf-protection is
>> enabled by default.
>>
>> This patch changes -fcf-protection to to enable the NOP portion of CET
>> ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of CET
>> ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.
>>
>> OK for trunk?
>
> As said in the PR, NOP sequences have non-zero cost in the executable
> (they enlarge the executable), so I don't think this feature should be
> enabled by default.
>
> There is always a configure option if someone wants their compiler to
> always emit relevant multi-byte nops.

What we need is an option to enable -fcf-function with multi-byte NOPs
without -mcet which enables the full CET ISAs.  A configure option
without the corresponding the command-line option makes test and
debug difficult.   I can add

--enable-cf-function-nop or --with-cf-function-nop

with

-fct-function-nop

-- 
H.J.

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

[Uros Bizjak]

On Tue, Apr 17, 2018 at 8:42 PM, H.J. Lu  wrote:
> -fcf-protection -mcet can't be used with IFUNC features, like symbol
> multiversioning or target clone, since IBT/SHSTK are applied to the whole
> program and they may be disabled in some functions.  But -fcf-protection
> is implemented with multi-byte NOPs on all 64-bit processors as well as
> 32-bit processors starting with Pentium Pro.  If -fcf-protection requires
> -mcet, IFUNC features can't be used on Linux when -fcf-protection is
> enabled by default.
>
> This patch changes -fcf-protection to to enable the NOP portion of CET
> ISAs unless IBT and/or SHSTK are disabled explicitly.  The rest of CET
> ISAs, including intrinsics, still requires -mcet, -mibt or -mshstk.
>
> OK for trunk?

As said in the PR, NOP sequences have non-zero cost in the executable
(they enlarge the executable), so I don't think this feature should be
enabled by default.

There is always a configure option if someone wants their compiler to
always emit relevant multi-byte nops.

Uros.