diff mbox series

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

Message ID 87k1vra2oh.fsf@redhat.com
State New
Headers show
Series RFA: PR 84154: Fix checking -mibt and -mshstk options for control flow protection | expand

Commit Message

Nick Clifton Feb. 5, 2018, 3:14 p.m. UTC 
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.

Comments

Tsimbalist, Igor V Feb. 6, 2018, 11:14 a.m. UTC | #1 
> -----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);
>      }
Nick Clifton Feb. 6, 2018, 12:15 p.m. UTC | #2 
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
Tsimbalist, Igor V Feb. 6, 2018, 2:08 p.m. UTC | #3 
> -----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

>
Uros Bizjak Feb. 6, 2018, 2:35 p.m. UTC | #4 
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.
Rainer Orth Feb. 6, 2018, 10:50 p.m. UTC | #5 
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
Tsimbalist, Igor V Feb. 6, 2018, 11:27 p.m. UTC | #6 
> -----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.
Paolo Carlini Feb. 6, 2018, 11:46 p.m. UTC | #7 
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.
Tsimbalist, Igor V Feb. 7, 2018, 12:02 a.m. UTC | #8 
> -----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.
Paolo Carlini Feb. 7, 2018, 12:07 a.m. UTC | #9 
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.
Jakub Jelinek Feb. 7, 2018, 8:54 a.m. UTC | #10 
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
diff mbox series

Patch

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