RFA: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

RFA: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Nick Clifton]

Hi H.J.

  Attached is a potential patch for PR 84145:
  
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84145

  The problem was that the code to check that the --mibt and/or -mshstk
  options have been correctly enabled for control flow protection did
  not take into account that the wrong option might have been enabled.

  So the patch inverts the test, looking at the value of
  flag_cf_protection first and then checking to see if the needed x86
  specific options have been enabled.  This gives results like this:


   % gcc -c main.c
   % gcc -c main.c -fcf-protection=full
cc1: error: '-fcf-protection=full' requires CET support on this target. Use -mcet or both of -mibt and -mshstk options to enable CET
   % gcc -c main.c -fcf-protection=full -mcet
   % gcc -c main.c -fcf-protection=full -mibt
cc1: error: '-fcf-protection=full' requires CET support on this target. Use -mcet or both of -mibt and -mshstk options to enable CET
   % gcc -c main.c -fcf-protection=full -mibt -mshstk
   % gcc -c main.c -fcf-protection=branch
cc1: error: '-fcf-protection=branch' requires CET support on this target. Use -mcet or -mibt to enable CET
   % gcc -c main.c -fcf-protection=branch -mcet
   % gcc -c main.c -fcf-protection=branch -mibt
   % gcc -c main.c -fcf-protection=branch -mshstk
cc1: error: '-fcf-protection=branch' requires CET support on this target. Use -mcet or -mibt to enable CET
   % gcc -c main.c -fcf-protection=return
cc1: error: '-fcf-protection=return' requires CET support on this target. Use -mcet or -mshstk to enable CET
   % gcc -c main.c -fcf-protection=return -mcet
   % gcc -c main.c -fcf-protection=return -mibt
cc1: error: '-fcf-protection=return' requires CET support on this target. Use -mcet or -mshstk to enable CET
   % gcc -c main.c -fcf-protection=return -mshstk
   %
   
  What do you think ?  Is the patch OK for the mainline ?

Cheers
  Nick

gcc/ChangeLog
2018-02-05  Nick Clifton  <nickc@redhat.com>

	PR 84145
	* config/i386/i386.c (ix86_option_override_internal): Rework
	checks for -fcf-protection and -mibt/-mshstk.

Index: gcc/config/i386/i386.c
===================================================================
--- gcc/config/i386/i386.c	(revision 257389)
+++ gcc/config/i386/i386.c	(working copy)
@@ -4915,30 +4915,43 @@
   /* Do not support control flow instrumentation if CET is not enabled.  */
   if (opts->x_flag_cf_protection != CF_NONE)
     {
-      if (!(TARGET_IBT_P (opts->x_ix86_isa_flags2)
-	    || TARGET_SHSTK_P (opts->x_ix86_isa_flags)))
+      switch (flag_cf_protection)
 	{
-	  if (flag_cf_protection == CF_FULL)
+	case CF_NONE:
+	  break;
+	case CF_BRANCH:
+	  if (! TARGET_IBT_P (opts->x_ix86_isa_flags2))
 	    {
-	      error ("%<-fcf-protection=full%> requires CET support "
-		     "on this target. Use -mcet or one of -mibt, "
-		     "-mshstk options to enable CET");
+	      error ("%<-fcf-protection=branch%> requires CET support "
+		     "on this target. Use -mcet or -mibt to enable CET");
+	      flag_cf_protection = CF_NONE;
+	      return false;
 	    }
-	  else if (flag_cf_protection == CF_BRANCH)
+	  break;
+	case CF_RETURN:
+	  if (! TARGET_SHSTK_P (opts->x_ix86_isa_flags))
 	    {
-	      error ("%<-fcf-protection=branch%> requires CET support "
-		     "on this target. Use -mcet or one of -mibt, "
-		     "-mshstk options to enable CET");
+	      error ("%<-fcf-protection=return%> requires CET support "
+		     "on this target. Use -mcet or -mshstk to enable CET");
+	      flag_cf_protection = CF_NONE;
+	      return false;
 	    }
-	  else if (flag_cf_protection == CF_RETURN)
+	  break;
+	case CF_FULL:
+	  if (   ! TARGET_IBT_P (opts->x_ix86_isa_flags2)
+		 || ! TARGET_SHSTK_P (opts->x_ix86_isa_flags))
 	    {
-	      error ("%<-fcf-protection=return%> requires CET support "
-		     "on this target. Use -mcet or one of -mibt, "
+	      error ("%<-fcf-protection=full%> requires CET support "
+		     "on this target. Use -mcet or both of -mibt and "
 		     "-mshstk options to enable CET");
+	      flag_cf_protection = CF_NONE;
+	      return false;
 	    }
-	  flag_cf_protection = CF_NONE;
-	  return false;
+	  break;
+	default:
+	  gcc_unreachable ();
 	}
+
       opts->x_flag_cf_protection =
 	(cf_protection_level) (opts->x_flag_cf_protection | CF_SET);
     }

RE: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Tsimbalist, Igor V]

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Nick Clifton
> Sent: Monday, February 5, 2018 4:15 PM
> To: hjl.tools@gmail.com
> Cc: gcc-patches@gcc.gnu.org
> Subject: RFA: PR 84154: Fix checking -mibt and -mshstk options for control
> flow protection
> 
> Hi H.J.
> 
>   Attached is a potential patch for PR 84145:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84145
> 
>   The problem was that the code to check that the --mibt and/or -mshstk
>   options have been correctly enabled for control flow protection did
>   not take into account that the wrong option might have been enabled.
> 
>   So the patch inverts the test, looking at the value of
>   flag_cf_protection first and then checking to see if the needed x86
>   specific options have been enabled.  This gives results like this:
> 
> 
>    % gcc -c main.c
>    % gcc -c main.c -fcf-protection=full
> cc1: error: '-fcf-protection=full' requires CET support on this target. Use -
> mcet or both of -mibt and -mshstk options to enable CET
>    % gcc -c main.c -fcf-protection=full -mcet
>    % gcc -c main.c -fcf-protection=full -mibt
> cc1: error: '-fcf-protection=full' requires CET support on this target. Use -
> mcet or both of -mibt and -mshstk options to enable CET
>    % gcc -c main.c -fcf-protection=full -mibt -mshstk
>    % gcc -c main.c -fcf-protection=branch
> cc1: error: '-fcf-protection=branch' requires CET support on this target. Use -
> mcet or -mibt to enable CET
>    % gcc -c main.c -fcf-protection=branch -mcet
>    % gcc -c main.c -fcf-protection=branch -mibt
>    % gcc -c main.c -fcf-protection=branch -mshstk
> cc1: error: '-fcf-protection=branch' requires CET support on this target. Use -
> mcet or -mibt to enable CET
>    % gcc -c main.c -fcf-protection=return
> cc1: error: '-fcf-protection=return' requires CET support on this target. Use -
> mcet or -mshstk to enable CET
>    % gcc -c main.c -fcf-protection=return -mcet
>    % gcc -c main.c -fcf-protection=return -mibt
> cc1: error: '-fcf-protection=return' requires CET support on this target. Use -
> mcet or -mshstk to enable CET
>    % gcc -c main.c -fcf-protection=return -mshstk
>    %
> 
>   What do you think ?  Is the patch OK for the mainline ?

Coincidentally, I have worked on the same patch. Please look at the patch, I uploaded it to the bug. The main differences are

- updated the output messages to be more informative;
- updated  the tests and add couple of new tests to check the messages;
- fixed a typo in the doc file related to fcf-protection;

I am ok with the changes in i386.c but would like to update the messages. Could you incorporate my changes and proceed? Or would you like me to finish the fix?

Thanks,
Igor

> Cheers
>   Nick
> 
> gcc/ChangeLog
> 2018-02-05  Nick Clifton  <nickc@redhat.com>
> 
> 	PR 84145
> 	* config/i386/i386.c (ix86_option_override_internal): Rework
> 	checks for -fcf-protection and -mibt/-mshstk.
> 
> Index: gcc/config/i386/i386.c
> ===============================================
> ====================
> --- gcc/config/i386/i386.c	(revision 257389)
> +++ gcc/config/i386/i386.c	(working copy)
> @@ -4915,30 +4915,43 @@
>    /* Do not support control flow instrumentation if CET is not enabled.  */
>    if (opts->x_flag_cf_protection != CF_NONE)
>      {
> -      if (!(TARGET_IBT_P (opts->x_ix86_isa_flags2)
> -	    || TARGET_SHSTK_P (opts->x_ix86_isa_flags)))
> +      switch (flag_cf_protection)
>  	{
> -	  if (flag_cf_protection == CF_FULL)
> +	case CF_NONE:
> +	  break;
> +	case CF_BRANCH:
> +	  if (! TARGET_IBT_P (opts->x_ix86_isa_flags2))
>  	    {
> -	      error ("%<-fcf-protection=full%> requires CET support "
> -		     "on this target. Use -mcet or one of -mibt, "
> -		     "-mshstk options to enable CET");
> +	      error ("%<-fcf-protection=branch%> requires CET support "
> +		     "on this target. Use -mcet or -mibt to enable CET");
> +	      flag_cf_protection = CF_NONE;
> +	      return false;
>  	    }
> -	  else if (flag_cf_protection == CF_BRANCH)
> +	  break;
> +	case CF_RETURN:
> +	  if (! TARGET_SHSTK_P (opts->x_ix86_isa_flags))
>  	    {
> -	      error ("%<-fcf-protection=branch%> requires CET support "
> -		     "on this target. Use -mcet or one of -mibt, "
> -		     "-mshstk options to enable CET");
> +	      error ("%<-fcf-protection=return%> requires CET support "
> +		     "on this target. Use -mcet or -mshstk to enable CET");
> +	      flag_cf_protection = CF_NONE;
> +	      return false;
>  	    }
> -	  else if (flag_cf_protection == CF_RETURN)
> +	  break;
> +	case CF_FULL:
> +	  if (   ! TARGET_IBT_P (opts->x_ix86_isa_flags2)
> +		 || ! TARGET_SHSTK_P (opts->x_ix86_isa_flags))
>  	    {
> -	      error ("%<-fcf-protection=return%> requires CET support "
> -		     "on this target. Use -mcet or one of -mibt, "
> +	      error ("%<-fcf-protection=full%> requires CET support "
> +		     "on this target. Use -mcet or both of -mibt and "
>  		     "-mshstk options to enable CET");
> +	      flag_cf_protection = CF_NONE;
> +	      return false;
>  	    }
> -	  flag_cf_protection = CF_NONE;
> -	  return false;
> +	  break;
> +	default:
> +	  gcc_unreachable ();
>  	}
> +
>        opts->x_flag_cf_protection =
>  	(cf_protection_level) (opts->x_flag_cf_protection | CF_SET);
>      }

Re: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Nick Clifton]

Hi Igor,

>>   Attached is a potential patch for PR 84145:
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84145

> Coincidentally, I have worked on the same patch.

Great minds, etc :-)

> Please look at the patch, I uploaded it to the bug. The main differences are
> 
> - updated the output messages to be more informative;
> - updated  the tests and add couple of new tests to check the messages;
> - fixed a typo in the doc file related to fcf-protection;
> 
> I am ok with the changes in i386.c but would like to update the messages. Could you incorporate my changes and proceed? Or would you like me to finish the fix?

If you are happy to finish the fix then please do so.  Your fix is
more thorough than mine, so I am happy to see it go on.  Although
I should say that I am not an x86 maintainer, so I cannot approve
it.

Cheers
  Nick

RE: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Tsimbalist, Igor V]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

> -----Original Message-----
> From: Nick Clifton [mailto:nickc@redhat.com]
> Sent: Tuesday, February 6, 2018 1:16 PM
> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; hjl.tools@gmail.com
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: PR 84154: Fix checking -mibt and -mshstk options for control
> flow protection
> 
> Hi Igor,
> 
> >>   Attached is a potential patch for PR 84145:
> >>
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84145
> 
> > Coincidentally, I have worked on the same patch.
> 
> Great minds, etc :-)
> 
> > Please look at the patch, I uploaded it to the bug. The main differences are
> >
> > - updated the output messages to be more informative;
> > - updated  the tests and add couple of new tests to check the messages;
> > - fixed a typo in the doc file related to fcf-protection;
> >
> > I am ok with the changes in i386.c but would like to update the messages.
> Could you incorporate my changes and proceed? Or would you like me to
> finish the fix?
> 
> If you are happy to finish the fix then please do so.  Your fix is
> more thorough than mine, so I am happy to see it go on.  Although
> I should say that I am not an x86 maintainer, so I cannot approve
> it.

Here is the updated patch. Please note the subject should say PR 84145.

Ok for trunk?

> Cheers
>   Nick
> 


[-- Attachment #2: 0001-Fix-checking-mibt-and-mshstk-options-for-control-flo.patch --]
[-- Type: application/octet-stream, Size: 10002 bytes --]

From 216ba23840c16c57db93d738293574c3e80c2017 Mon Sep 17 00:00:00 2001
From: Igor Tsimbalist <igor.v.tsimbalist@intel.com>
Date: Tue, 6 Feb 2018 16:39:31 +0300
Subject: [PATCH] Fix checking -mibt and -mshstk options for control flow
 protection

	PR target/84145
	* config/i386/i386.c: Reimplement the check of possible options
	-mibt/-mshstk conbination. Change error messages.
	* doc/invoke.texi: Fix a typo: remove extra '='.
	* c-c++-common/fcf-protection-1.c: Change a compared message.
	* 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: New test.
	* c-c++-common/fcf-protection-7.c: Likewise.
---
 gcc/ChangeLog                                 |  7 +++++
 gcc/config/i386/i386.c                        | 43 +++++++++++++++++----------
 gcc/doc/invoke.texi                           |  4 +--
 gcc/testsuite/ChangeLog                       | 10 +++++++
 gcc/testsuite/c-c++-common/fcf-protection-1.c |  2 +-
 gcc/testsuite/c-c++-common/fcf-protection-2.c |  2 +-
 gcc/testsuite/c-c++-common/fcf-protection-3.c |  2 +-
 gcc/testsuite/c-c++-common/fcf-protection-5.c |  2 +-
 gcc/testsuite/c-c++-common/fcf-protection-6.c |  4 +++
 gcc/testsuite/c-c++-common/fcf-protection-7.c |  4 +++
 10 files changed, 59 insertions(+), 21 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/fcf-protection-6.c
 create mode 100644 gcc/testsuite/c-c++-common/fcf-protection-7.c

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index e5a8c11..c73260e 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@
+2018-02-06  Igor Tsimbalist  <igor.v.tsimbalist@intel.com>
+
+	PR target/84145
+	* config/i386/i386.c: Reimplement the check of possible options
+	-mibt/-mshstk conbination. Change error messages.
+	* doc/invoke.texi: Fix a typo: remove extra '='.
+
 2018-02-05  Martin Liska  <mliska@suse.cz>
 
 	* doc/invoke.texi: Cherry-pick upstream r323995.
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index b07f581..d6ca15e 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -4915,30 +4915,43 @@ ix86_option_override_internal (bool main_args_p,
   /* Do not support control flow instrumentation if CET is not enabled.  */
   if (opts->x_flag_cf_protection != CF_NONE)
     {
-      if (!(TARGET_IBT_P (opts->x_ix86_isa_flags2)
-	    || TARGET_SHSTK_P (opts->x_ix86_isa_flags)))
+      switch (flag_cf_protection)
 	{
-	  if (flag_cf_protection == CF_FULL)
+	case CF_NONE:
+	  break;
+	case CF_BRANCH:
+	  if (! TARGET_IBT_P (opts->x_ix86_isa_flags2))
 	    {
-	      error ("%<-fcf-protection=full%> requires CET support "
-		     "on this target. Use -mcet or one of -mibt, "
-		     "-mshstk options to enable CET");
+	      error ("%<-fcf-protection=branch%> requires Intel CET "
+		     "support. Use -mcet or -mibt option to enable CET");
+	      flag_cf_protection = CF_NONE;
+	      return false;
 	    }
-	  else if (flag_cf_protection == CF_BRANCH)
+	  break;
+	case CF_RETURN:
+	  if (! TARGET_SHSTK_P (opts->x_ix86_isa_flags))
 	    {
-	      error ("%<-fcf-protection=branch%> requires CET support "
-		     "on this target. Use -mcet or one of -mibt, "
-		     "-mshstk options to enable CET");
+	      error ("%<-fcf-protection=return%> requires Intel CET "
+		     "support. Use -mcet or -mshstk option to enable CET");
+	      flag_cf_protection = CF_NONE;
+	      return false;
 	    }
-	  else if (flag_cf_protection == CF_RETURN)
+	  break;
+	case CF_FULL:
+	  if (   ! TARGET_IBT_P (opts->x_ix86_isa_flags2)
+		 || ! TARGET_SHSTK_P (opts->x_ix86_isa_flags))
 	    {
-	      error ("%<-fcf-protection=return%> requires CET support "
-		     "on this target. Use -mcet or one of -mibt, "
+	      error ("%<-fcf-protection=full%> requires Intel CET "
+		     "support. Use -mcet or both of -mibt and "
 		     "-mshstk options to enable CET");
+	      flag_cf_protection = CF_NONE;
+	      return false;
 	    }
-	  flag_cf_protection = CF_NONE;
-	  return false;
+	  break;
+	default:
+	  gcc_unreachable ();
 	}
+
       opts->x_flag_cf_protection =
 	(cf_protection_level) (opts->x_flag_cf_protection | CF_SET);
     }
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index cf6d3ae..befebed 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -463,7 +463,7 @@ Objective-C and Objective-C++ Dialects}.
 -fchkp-check-read  -fchkp-check-write  -fchkp-store-bounds @gol
 -fchkp-instrument-calls  -fchkp-instrument-marked-only @gol
 -fchkp-use-wrappers  -fchkp-flexible-struct-trailing-arrays@gol
--fcf-protection==@r{[}full@r{|}branch@r{|}return@r{|}none@r{]} @gol
+-fcf-protection=@r{[}full@r{|}branch@r{|}return@r{|}none@r{]} @gol
 -fstack-protector  -fstack-protector-all  -fstack-protector-strong @gol
 -fstack-protector-explicit  -fstack-check @gol
 -fstack-limit-register=@var{reg}  -fstack-limit-symbol=@var{sym} @gol
@@ -11631,7 +11631,7 @@ is used to link a program, the GCC driver automatically links
 against @file{libmpxwrappers}.  See also @option{-static-libmpxwrappers}.
 Enabled by default.
 
-@item -fcf-protection==@r{[}full@r{|}branch@r{|}return@r{|}none@r{]}
+@item -fcf-protection=@r{[}full@r{|}branch@r{|}return@r{|}none@r{]}
 @opindex fcf-protection
 Enable code instrumentation of control-flow transfers to increase
 program security by checking that target addresses of control-flow
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index bed5534..1ef4ee3 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,13 @@
+2018-02-06  Igor Tsimbalist  <igor.v.tsimbalist@intel.com>
+
+	PR target/84145
+	* c-c++-common/fcf-protection-1.c: Change a compared message.
+	* 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: New test.
+	* c-c++-common/fcf-protection-7.c: Likewise.
+
 2018-02-05  Paolo Carlini  <paolo.carlini@oracle.com>
 
 	PR c++/82782
diff --git a/gcc/testsuite/c-c++-common/fcf-protection-1.c b/gcc/testsuite/c-c++-common/fcf-protection-1.c
index 2e9337c..8e71f47 100644
--- a/gcc/testsuite/c-c++-common/fcf-protection-1.c
+++ b/gcc/testsuite/c-c++-common/fcf-protection-1.c
@@ -1,4 +1,4 @@
 /* { dg-do compile } */
 /* { dg-options "-fcf-protection=full" } */
-/* { dg-error "'-fcf-protection=full' requires CET support on this target" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
+/* { dg-error "'-fcf-protection=full' requires Intel CET.*-mcet.*-mibt and -mshstk option" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
 /* { dg-error "'-fcf-protection=full' is not supported for this target" "" { target { ! "i?86-*-* x86_64-*-*" } } 0 } */
diff --git a/gcc/testsuite/c-c++-common/fcf-protection-2.c b/gcc/testsuite/c-c++-common/fcf-protection-2.c
index aa0d2a0..d7d6db0 100644
--- a/gcc/testsuite/c-c++-common/fcf-protection-2.c
+++ b/gcc/testsuite/c-c++-common/fcf-protection-2.c
@@ -1,4 +1,4 @@
 /* { dg-do compile } */
 /* { dg-options "-fcf-protection=branch" } */
-/* { dg-error "'-fcf-protection=branch' requires CET support on this target" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
+/* { dg-error "'-fcf-protection=branch' requires Intel CET.*-mcet or -mibt option" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
 /* { dg-error "'-fcf-protection=branch' is not supported for this target" "" { target { ! "i?86-*-* x86_64-*-*" } } 0 } */
diff --git a/gcc/testsuite/c-c++-common/fcf-protection-3.c b/gcc/testsuite/c-c++-common/fcf-protection-3.c
index 028775a..5b903c5 100644
--- a/gcc/testsuite/c-c++-common/fcf-protection-3.c
+++ b/gcc/testsuite/c-c++-common/fcf-protection-3.c
@@ -1,4 +1,4 @@
 /* { dg-do compile } */
 /* { dg-options "-fcf-protection=return" } */
-/* { dg-error "'-fcf-protection=return' requires CET support on this target" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
+/* { dg-error "'-fcf-protection=return' requires Intel CET.*-mcet or -mshstk option" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
 /* { dg-error "'-fcf-protection=return' is not supported for this target" "" { target { ! "i?86-*-* x86_64-*-*" } } 0 } */
diff --git a/gcc/testsuite/c-c++-common/fcf-protection-5.c b/gcc/testsuite/c-c++-common/fcf-protection-5.c
index a5f8e11..d7a6780 100644
--- a/gcc/testsuite/c-c++-common/fcf-protection-5.c
+++ b/gcc/testsuite/c-c++-common/fcf-protection-5.c
@@ -1,4 +1,4 @@
 /* { dg-do compile } */
 /* { dg-options "-fcf-protection" } */
-/* { dg-error "'-fcf-protection=full' requires CET support on this target" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
+/* { dg-error "'-fcf-protection=full' requires Intel CET.*-mcet.*-mibt and -mshstk option" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
 /* { dg-error "'-fcf-protection=full' is not supported for this target" "" { target { ! "i?86-*-* x86_64-*-*" } } 0 } */
diff --git a/gcc/testsuite/c-c++-common/fcf-protection-6.c b/gcc/testsuite/c-c++-common/fcf-protection-6.c
new file mode 100644
index 0000000..084983e
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/fcf-protection-6.c
@@ -0,0 +1,4 @@
+/* { dg-do compile } */
+/* { dg-options "-fcf-protection=branch -mshstk" } */
+/* { dg-error "'-fcf-protection=branch' requires Intel CET.*-mcet or -mibt option" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
+/* { dg-error "'-fcf-protection=branch' is not supported for this target" "" { target { ! "i?86-*-* x86_64-*-*" } } 0 } */
diff --git a/gcc/testsuite/c-c++-common/fcf-protection-7.c b/gcc/testsuite/c-c++-common/fcf-protection-7.c
new file mode 100644
index 0000000..9baedf5
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/fcf-protection-7.c
@@ -0,0 +1,4 @@
+/* { dg-do compile } */
+/* { dg-options "-fcf-protection=return -mibt" } */
+/* { dg-error "'-fcf-protection=return' requires Intel CET.*-mcet or -mshstk option" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
+/* { dg-error "'-fcf-protection=return' is not supported for this target" "" { target { ! "i?86-*-* x86_64-*-*" } } 0 } */
-- 
1.8.3.1

Re: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Uros Bizjak]

On Tue, Feb 6, 2018 at 3:08 PM, Tsimbalist, Igor V
<igor.v.tsimbalist@intel.com> wrote:
>> -----Original Message-----
>> From: Nick Clifton [mailto:nickc@redhat.com]
>> Sent: Tuesday, February 6, 2018 1:16 PM
>> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; hjl.tools@gmail.com
>> Cc: gcc-patches@gcc.gnu.org
>> Subject: Re: PR 84154: Fix checking -mibt and -mshstk options for control
>> flow protection
>>
>> Hi Igor,
>>
>> >>   Attached is a potential patch for PR 84145:
>> >>
>> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84145
>>
>> > Coincidentally, I have worked on the same patch.
>>
>> Great minds, etc :-)
>>
>> > Please look at the patch, I uploaded it to the bug. The main differences are
>> >
>> > - updated the output messages to be more informative;
>> > - updated  the tests and add couple of new tests to check the messages;
>> > - fixed a typo in the doc file related to fcf-protection;
>> >
>> > I am ok with the changes in i386.c but would like to update the messages.
>> Could you incorporate my changes and proceed? Or would you like me to
>> finish the fix?
>>
>> If you are happy to finish the fix then please do so.  Your fix is
>> more thorough than mine, so I am happy to see it go on.  Although
>> I should say that I am not an x86 maintainer, so I cannot approve
>> it.
>
> Here is the updated patch. Please note the subject should say PR 84145.
>
> Ok for trunk?

LGTM.

Thanks,
Uros.

Re: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Rainer Orth]

[-- Attachment #1: Type: text/plain, Size: 1981 bytes --]

Hi Igor,

> Here is the updated patch. Please note the subject should say PR 84145.

the two new testcases FAIL on all non-x86 targets (I've seen that on
sparc-sun-solaris2.11, there's a gcc-testresults posting for
powerpc64le-unknown-linux-gnu, and PR testsuite/84243 reports it for
aarch64-none-linux-gnu:

+FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++11  (test for errors, line )
+FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++11 (test for excess errors)
+FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++14  (test for errors, line )
+FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++14 (test for excess errors)
+FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++98  (test for errors, line )
+FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++98 (test for excess errors)

Excess errors:
xg++: error: unrecognized command line option '-mshstk'

+FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++11  (test for errors, line )
+FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++11 (test for excess errors)
+FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++14  (test for errors, line )
+FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++14 (test for excess errors)
+FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++98  (test for errors, line )
+FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++98 (test for excess errors)

Excess errors:
xg++: error: unrecognized command line option '-mibt'

I think the right way to handle that is to pass -mshstk resp. -mibt on
x86 only.  The following patch does this; tested with the appropriate
runtest invocation on i386-pc-solaris2.11 and sparc-sun-solaris2.11.

Ok for mainline?

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


2018-02-06  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	PR testsuite/84243
	* c-c++-common/fcf-protection-6.c: Only pass -mshstk on x86
	targets.
	* c-c++-common/fcf-protection-7.c: Likewise for -mibt.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: testsuite-i386-fcf-protection-67.patch --]
[-- Type: text/x-patch, Size: 1530 bytes --]

# HG changeset patch
# Parent  bc1af87b4176f6f5ddc900980a18df826cbf4553
Don't pass x86-only options on non-x86 targets in c-c++-common/fcf-protection-[67].c

diff --git a/gcc/testsuite/c-c++-common/fcf-protection-6.c b/gcc/testsuite/c-c++-common/fcf-protection-6.c
--- a/gcc/testsuite/c-c++-common/fcf-protection-6.c
+++ b/gcc/testsuite/c-c++-common/fcf-protection-6.c
@@ -1,4 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-fcf-protection=branch -mshstk" } */
+/* { dg-options "-fcf-protection=branch" } */
+/* { dg-additional-options "-mshstk" { target { i?86-*-* x86_64-*-* } } } */
 /* { dg-error "'-fcf-protection=branch' requires Intel CET.*-mcet or -mibt option" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
 /* { dg-error "'-fcf-protection=branch' is not supported for this target" "" { target { ! "i?86-*-* x86_64-*-*" } } 0 } */
diff --git a/gcc/testsuite/c-c++-common/fcf-protection-7.c b/gcc/testsuite/c-c++-common/fcf-protection-7.c
--- a/gcc/testsuite/c-c++-common/fcf-protection-7.c
+++ b/gcc/testsuite/c-c++-common/fcf-protection-7.c
@@ -1,4 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-fcf-protection=return -mibt" } */
+/* { dg-options "-fcf-protection=return" } */
+/* { dg-additional-options "-mibt" { target { i?86-*-* x86_64-*-* } } } */
 /* { dg-error "'-fcf-protection=return' requires Intel CET.*-mcet or -mshstk option" "" { target { "i?86-*-* x86_64-*-*" } } 0 } */
 /* { dg-error "'-fcf-protection=return' is not supported for this target" "" { target { ! "i?86-*-* x86_64-*-*" } } 0 } */

RE: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Tsimbalist, Igor V]

> -----Original Message-----
> From: Rainer Orth [mailto:ro@CeBiTec.Uni-Bielefeld.DE]
> Sent: Tuesday, February 6, 2018 11:50 PM
> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>
> Cc: gcc-patches@gcc.gnu.org; Nick Clifton <nickc@redhat.com>;
> hjl.tools@gmail.com; Uros Bizjak <ubizjak@gmail.com>
> Subject: Re: PR 84154: Fix checking -mibt and -mshstk options for control
> flow protection
> 
> Hi Igor,
> 
> > Here is the updated patch. Please note the subject should say PR 84145.
> 
> the two new testcases FAIL on all non-x86 targets (I've seen that on
> sparc-sun-solaris2.11, there's a gcc-testresults posting for
> powerpc64le-unknown-linux-gnu, and PR testsuite/84243 reports it for
> aarch64-none-linux-gnu:
> 
> +FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++11  (test for errors,
> line )
> +FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++11 (test for excess
> errors)
> +FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++14  (test for errors,
> line )
> +FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++14 (test for excess
> errors)
> +FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++98  (test for errors,
> line )
> +FAIL: c-c++-common/fcf-protection-6.c  -std=gnu++98 (test for excess
> errors)
> 
> Excess errors:
> xg++: error: unrecognized command line option '-mshstk'
> 
> +FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++11  (test for errors,
> line )
> +FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++11 (test for excess
> errors)
> +FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++14  (test for errors,
> line )
> +FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++14 (test for excess
> errors)
> +FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++98  (test for errors,
> line )
> +FAIL: c-c++-common/fcf-protection-7.c  -std=gnu++98 (test for excess
> errors)
> 
> Excess errors:
> xg++: error: unrecognized command line option '-mibt'
> 
> I think the right way to handle that is to pass -mshstk resp. -mibt on
> x86 only.  The following patch does this; tested with the appropriate
> runtest invocation on i386-pc-solaris2.11 and sparc-sun-solaris2.11.
> 
> Ok for mainline?

Agree with the fix. Thanks for taking care of this issue.

Igor

> 	Rainer
> 
> --
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
> 
> 
> 2018-02-06  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
> 
> 	PR testsuite/84243
> 	* c-c++-common/fcf-protection-6.c: Only pass -mshstk on x86
> 	targets.
> 	* c-c++-common/fcf-protection-7.c: Likewise for -mibt.

Re: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Paolo Carlini]

Hi,

on a rather old x86_64-linux machine GCC doesn't build anymore with r257414:

libtool: compile:  /xxx/Gcc/svn-dirs/trunk-build/./gcc/xg++ 
-B/xxx/Gcc/svn-dirs/trunk-build/./gcc/ -nostdinc++ -nostdinc++ 
-I/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu 
-I/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-v3/include 
-I/xxx/Gcc/svn-dirs/trunk/libstdc++-v3/libsupc++ 
-I/xxx/Gcc/svn-dirs/trunk/libstdc++-v3/include/backward 
-I/xxx/Gcc/svn-dirs/trunk/libstdc++-v3/testsuite/util 
-L/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-v3/src 
-L/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs 
-L/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs 
-B/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs 
-B/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs 
-B/xxx/Gcc/svn-dirs/trunk-install/x86_64-pc-linux-gnu/bin/ 
-B/xxx/Gcc/svn-dirs/trunk-install/x86_64-pc-linux-gnu/lib/ -isystem 
/xxx/Gcc/svn-dirs/trunk-install/x86_64-pc-linux-gnu/include -isystem 
/xxx/Gcc/svn-dirs/trunk-install/x86_64-pc-linux-gnu/sys-include 
-DHAVE_CONFIG_H -I. -I../../../trunk/libitm 
-I../../../trunk/libitm/config/linux/x86 
-I../../../trunk/libitm/config/linux -I../../../trunk/libitm/config/x86 
-I../../../trunk/libitm/config/posix 
-I../../../trunk/libitm/config/generic -I../../../trunk/libitm -mrtm 
-Wall -pthread -Werror -fcf-protection -mcet -std=gnu++0x 
-funwind-tables -fno-exceptions -fno-rtti -fabi-version=4 -g -O2 
-D_GNU_SOURCE -MT beginend.lo -MD -MP -MF .deps/beginend.Tpo -c 
../../../trunk/libitm/beginend.cc  -fPIC -DPIC -o .libs/beginend.o
In file included from 
/xxx/Gcc/svn-dirs/trunk-build/gcc/include/x86intrin.h:27,
                  from ../../../trunk/libitm/config/x86/target.h:26,
                  from ../../../trunk/libitm/libitm_i.h:74,
                  from ../../../trunk/libitm/barrier.cc:25:
/xxx/Gcc/svn-dirs/trunk-build/gcc/include/ia32intrin.h:56:28: internal 
compiler error: in ix86_option_override_internal, at config/i386/i386.c:4952
  #pragma GCC target("sse4.2")

Paolo.

RE: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Tsimbalist, Igor V]

> -----Original Message-----
> From: Paolo Carlini [mailto:paolo.carlini@oracle.com]
> Sent: Wednesday, February 7, 2018 12:46 AM
> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; gcc-
> patches@gcc.gnu.org
> Cc: Nick Clifton <nickc@redhat.com>; hjl.tools@gmail.com; Uros Bizjak
> <ubizjak@gmail.com>
> Subject: Re: PR 84154: Fix checking -mibt and -mshstk options for control
> flow protection
> 
> Hi,
> 
> on a rather old x86_64-linux machine GCC doesn't build anymore with
> r257414:
> 
> libtool: compile:  /xxx/Gcc/svn-dirs/trunk-build/./gcc/xg++
> -B/xxx/Gcc/svn-dirs/trunk-build/./gcc/ -nostdinc++ -nostdinc++
> -I/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-
> v3/include/x86_64-pc-linux-gnu
> -I/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-v3/include
> -I/xxx/Gcc/svn-dirs/trunk/libstdc++-v3/libsupc++
> -I/xxx/Gcc/svn-dirs/trunk/libstdc++-v3/include/backward
> -I/xxx/Gcc/svn-dirs/trunk/libstdc++-v3/testsuite/util
> -L/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-v3/src
> -L/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs
> -L/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-
> v3/libsupc++/.libs
> -B/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs
> -B/xxx/Gcc/svn-dirs/trunk-build/x86_64-pc-linux-gnu/libstdc++-
> v3/libsupc++/.libs
> -B/xxx/Gcc/svn-dirs/trunk-install/x86_64-pc-linux-gnu/bin/
> -B/xxx/Gcc/svn-dirs/trunk-install/x86_64-pc-linux-gnu/lib/ -isystem
> /xxx/Gcc/svn-dirs/trunk-install/x86_64-pc-linux-gnu/include -isystem
> /xxx/Gcc/svn-dirs/trunk-install/x86_64-pc-linux-gnu/sys-include
> -DHAVE_CONFIG_H -I. -I../../../trunk/libitm
> -I../../../trunk/libitm/config/linux/x86
> -I../../../trunk/libitm/config/linux -I../../../trunk/libitm/config/x86
> -I../../../trunk/libitm/config/posix
> -I../../../trunk/libitm/config/generic -I../../../trunk/libitm -mrtm
> -Wall -pthread -Werror -fcf-protection -mcet -std=gnu++0x
> -funwind-tables -fno-exceptions -fno-rtti -fabi-version=4 -g -O2
> -D_GNU_SOURCE -MT beginend.lo -MD -MP -MF .deps/beginend.Tpo -c
> ../../../trunk/libitm/beginend.cc  -fPIC -DPIC -o .libs/beginend.o
> In file included from
> /xxx/Gcc/svn-dirs/trunk-build/gcc/include/x86intrin.h:27,
>                   from ../../../trunk/libitm/config/x86/target.h:26,
>                   from ../../../trunk/libitm/libitm_i.h:74,
>                   from ../../../trunk/libitm/barrier.cc:25:
> /xxx/Gcc/svn-dirs/trunk-build/gcc/include/ia32intrin.h:56:28: internal
> compiler error: in ix86_option_override_internal, at config/i386/i386.c:4952
>   #pragma GCC target("sse4.2")

The issue is known and is covered by https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84248. The patch has been posted

https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00276.html

Igor

> Paolo.

Re: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Paolo Carlini]

Hi,

On 07/02/2018 01:02, Tsimbalist, Igor V wrote:
> The issue is known and is covered by 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84248. The patch has been 
> posted
> https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00276.html
Ok, thanks, I missed the above.

Paolo.

Re: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Jakub Jelinek]

On Tue, Feb 06, 2018 at 11:14:08AM +0000, Tsimbalist, Igor V wrote:
> Coincidentally, I have worked on the same patch. Please look at the patch, I uploaded it to the bug. The main differences are
> 
> - updated the output messages to be more informative;
> - updated  the tests and add couple of new tests to check the messages;
> - fixed a typo in the doc file related to fcf-protection;
> 
> I am ok with the changes in i386.c but would like to update the messages. Could you incorporate my changes and proceed? Or would you like me to finish the fix?

The r257414 change broke bootstrap on both x86_64-linux and i686-linux,
in both the bootstrap ICEs during libitm build of aatree.cc:
/.../gcc/obj70/./gcc/xg++ -B/.../gcc/obj70/./gcc/ -nostdinc++ -nostdinc++ -I/.../gcc/obj70/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu -I/.../gcc/obj70/x86_64-pc-linux-gnu/libstdc++-v3/include -I/.../gcc/libstdc++-v3/libsupc++ -I/.../gcc/libstdc++-v3/include/backward -I/.../gcc/libstdc++-v3/testsuite/util -L/.../gcc/obj70/x86_64-pc-linux-gnu/libstdc++-v3/src -L/.../gcc/obj70/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs -L/.../gcc/obj70/x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs -B/.../gcc/obj70/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs -B/.../gcc/obj70/x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs -B/usr/local/x86_64-pc-linux-gnu/bin/ -B/usr/local/x86_64-pc-linux-gnu/lib/ -isystem /usr/local/x86_64-pc-linux-gnu/include -isystem /usr/local/x86_64-pc-linux-gnu/sys-include -DHAVE_CONFIG_H -I. -I../../../libitm -I../../../libitm/config/linux/x86 -I../../../libitm/config/linux -I../../../libitm/config/x86 -I../../../libitm/config/posix -I../../../libitm/config/generic -I../../../libitm -mrtm -Wall -pthread -Werror -fcf-protection -mcet -std=gnu++0x -funwind-tables -fno-exceptions -fno-rtti -fabi-version=4 -g -O2 -D_GNU_SOURCE -MT aatree.lo -MD -MP -MF .deps/aatree.Tpo -c ../../../libitm/aatree.cc  -fPIC -DPIC -o .libs/aatree.o
/.../gcc/obj70/gcc/include/ia32intrin.h:56:28: internal compiler error: in ix86_option_override_internal, at config/i386/i386.c:4948

The problem is that the enum has
{CF_NONE, CF_BRANCH, CF_RETURN, CF_FULL, CF_SET}
values and the hook does:
      opts->x_flag_cf_protection =
	(cf_protection_level) (opts->x_flag_cf_protection | CF_SET);
after the switch (note, bad formatting), and if called again, we end up with
e.g. CF_FULL | CF_SET value which the switch doesn't handle.

It isn't clear what you want to do, either ignore the CF_SET | ...
values in the switch instead of gcc_unreachable on them, or
switch (flag_cf_protection & ~CF_SET)
or something similar.

	Jakub