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

["Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>]

> -----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);
>      }