diff mbox

[i386] Add -mstack-protector-guard= for i386

Message ID CABySjRuYS4aBPHNiHrQMfQgZM=sK7FVcaCCohHULK8O_urRXCQ@mail.gmail.com
State New
Headers show

Commit Message

Andrew Hsieh April 12, 2013, 8:43 a.m. UTC 
Introduced TARGET_SSP_GLOBAL_GUARD and TARGET_SSP_TLS_GUARD as suggested.
As for handling default in ix86_option_override_internal, is it
possible to achieve the same "default to SSP_GLOBAL if bionic"
entirely in i386.opt?


On Fri, Apr 12, 2013 at 2:45 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Hello!
>
>> Bionic in Android 4.2 starts to support stack-protector canary at TLS for x86.
>>
>> Android prior to 4.2 still looks at a global variable for stack
>> canary.  To maintain backward compatibility, I propose to add a new
>> option -mstack-protector-guard={global,tls} for i386 back-end to use
>> canary at global or per-thread at %gs:20, respectively.
>>
>> "global" is the default for bionic (*1); otherwise "tls" is the default
>
>  (define_expand "stack_protect_set"
>    [(match_operand 0 "memory_operand")
>     (match_operand 1 "memory_operand")]
> -  "!TARGET_HAS_BIONIC"
> +  "ix86_stack_protector_guard == SSP_TLS"
>
> Please introduce TARGET_SSP_GLOBAL_GUARD and TARGET_SSP_TLS_GUARD
> defines to i386.h (please follow an example of TARGET_GNU_TLS).
>
>      target_option_default_node = target_option_current_node
>        = build_target_option_node ();
> +
> +  /* Handle stack protector */
> +  if (!global_options_set.x_ix86_stack_protector_guard)
> +    {
> +      ix86_stack_protector_guard = TARGET_HAS_BIONIC? SSP_GLOBAL : SSP_TLS;
> +    }
>  }
>
> You should not need this part, JoinedVar directive should handle this
> functionality by itself. Again, please see ix86_tls_dialect handling.
>
> Uros.

Comments

Jakub Jelinek April 12, 2013, 9:05 a.m. UTC | #1 
On Fri, Apr 12, 2013 at 04:43:51PM +0800, Andrew Hsieh wrote:
> --- gcc/config/i386/i386-opts.h (revision 197837)
> +++ gcc/config/i386/i386-opts.h (working copy)
> @@ -85,4 +85,9 @@
>    ix86_veclibabi_type_acml
>  };
> 
> +enum stack_protector_guard {
> +  SSP_TLS,      /* per-thread canary at %gs:20 */

This comment is wrong, the TLS canary is {%fs,%gs}:{0x14,0x18,0x28}
depending on command line options.

> --- gcc/config/i386/i386.c (revision 197837)
> +++ gcc/config/i386/i386.c (working copy)
> @@ -3922,6 +3922,12 @@
>    if (main_args_p)
>      target_option_default_node = target_option_current_node
>        = build_target_option_node ();
> +
> +  /* Handle stack protector */
> +  if (!global_options_set.x_ix86_stack_protector_guard)
> +    {
> +      ix86_stack_protector_guard = TARGET_HAS_BIONIC? SSP_GLOBAL : SSP_TLS;
> +    }

Wrong formatting.  No {} around single line statement then body,
and missing space before ?.

	Jakub
Uros Bizjak April 12, 2013, 9:49 a.m. UTC | #2 
On Fri, Apr 12, 2013 at 10:43 AM, Andrew Hsieh <andrewhsieh@google.com> wrote:
> Introduced TARGET_SSP_GLOBAL_GUARD and TARGET_SSP_TLS_GUARD as suggested.
> As for handling default in ix86_option_override_internal, is it
> possible to achieve the same "default to SSP_GLOBAL if bionic"
> entirely in i386.opt?

No, AFAIK option system won't dynamically initialize a variable.

+mstack-protector-guard=
+Target RejectNegative Joined Enum(stack_protector_guard)
Var(ix86_stack_protector_
guard) Init(SSP_TLS)
+Use given stack-protector guard

So, you don't need Init in the code above ...

> +
> +  /* Handle stack protector */
> +  if (!global_options_set.x_ix86_stack_protector_guard)
> +    {
> +      ix86_stack_protector_guard = TARGET_HAS_BIONIC? SSP_GLOBAL : SSP_TLS;
> +    }

... since you initialize it here.

Please, remove the braces here.

Also, as Jakub said, please also update the comment for 32bit case.

The patch is OK with these changes.

Thanks,
Uros.
diff mbox

Patch

===
2013-04-12  Andrew Hsieh  <andrewhsieh.google.com>

        * config/i386/i386.opt: New option mstack-protector-guard=.
        * config/i386/i386-opts.h: Add enum stack_protector_guard.
        * config/i386/i386.h: Introduce TARGET_SSP_GLOBAL_GUARD and
        TARGET_SSP_TLS_GUARD.
        * config/i386/i386.c (ix86_option_override_internal): Default
        to SSP_TLS unless it's bionic
        * config/i386/i386.md: define_expand/insn "stack_protect_set/test..."
        if TARGET_SSP_TLS_GUARD
        * doc/invoke.texi (i386 Option): Document.

Index: gcc/config/i386/i386.opt
===================================================================
--- gcc/config/i386/i386.opt (revision 197837)
+++ gcc/config/i386/i386.opt (working copy)
@@ -626,3 +626,17 @@ 
 mrtm
 Target Report Mask(ISA_RTM) Var(ix86_isa_flags) Save
 Support RTM built-in functions and code generation
+
+mstack-protector-guard=
+Target RejectNegative Joined Enum(stack_protector_guard)
Var(ix86_stack_protector_guard) Init(SSP_TLS)
+Use given stack-protector guard
+
+Enum
+Name(stack_protector_guard) Type(enum stack_protector_guard)
+Known stack protector guard (for use with the -mstack-protector-guard= option):
+
+EnumValue
+Enum(stack_protector_guard) String(tls) Value(SSP_TLS)
+
+EnumValue
+Enum(stack_protector_guard) String(global) Value(SSP_GLOBAL)
Index: gcc/config/i386/i386.md
===================================================================
--- gcc/config/i386/i386.md (revision 197837)
+++ gcc/config/i386/i386.md (working copy)
@@ -17058,7 +17058,7 @@ 
 (define_expand "stack_protect_set"
   [(match_operand 0 "memory_operand")
    (match_operand 1 "memory_operand")]
-  "!TARGET_HAS_BIONIC"
+  "TARGET_SSP_TLS_GUARD"
 {
   rtx (*insn)(rtx, rtx);

@@ -17083,7 +17083,7 @@ 
     UNSPEC_SP_SET))
    (set (match_scratch:PTR 2 "=&r") (const_int 0))
    (clobber (reg:CC FLAGS_REG))]
-  "!TARGET_HAS_BIONIC"
+  "TARGET_SSP_TLS_GUARD"
   "mov{<imodesuffix>}\t{%1, %2|%2, %1}\;mov{<imodesuffix>}\t{%2,
%0|%0, %2}\;xor{l}\t%k2, %k2"
   [(set_attr "type" "multi")])

@@ -17101,7 +17101,7 @@ 
   [(match_operand 0 "memory_operand")
    (match_operand 1 "memory_operand")
    (match_operand 2)]
-  "!TARGET_HAS_BIONIC"
+  "TARGET_SSP_TLS_GUARD"
 {
   rtx flags = gen_rtx_REG (CCZmode, FLAGS_REG);

@@ -17131,7 +17131,7 @@ 
      (match_operand:PTR 2 "memory_operand" "m")]
     UNSPEC_SP_TEST))
    (clobber (match_scratch:PTR 3 "=&r"))]
-  "!TARGET_HAS_BIONIC"
+  "TARGET_SSP_TLS_GUARD"
   "mov{<imodesuffix>}\t{%1, %3|%3, %1}\;xor{<imodesuffix>}\t{%2, %3|%3, %2}"
   [(set_attr "type" "multi")])

Index: gcc/config/i386/i386-opts.h
===================================================================
--- gcc/config/i386/i386-opts.h (revision 197837)
+++ gcc/config/i386/i386-opts.h (working copy)
@@ -85,4 +85,9 @@ 
   ix86_veclibabi_type_acml
 };

+enum stack_protector_guard {
+  SSP_TLS,      /* per-thread canary at %gs:20 */
+  SSP_GLOBAL    /* global canary */
+};
+
 #endif
Index: gcc/config/i386/i386.c
===================================================================
--- gcc/config/i386/i386.c (revision 197837)
+++ gcc/config/i386/i386.c (working copy)
@@ -3922,6 +3922,12 @@ 
   if (main_args_p)
     target_option_default_node = target_option_current_node
       = build_target_option_node ();
+
+  /* Handle stack protector */
+  if (!global_options_set.x_ix86_stack_protector_guard)
+    {
+      ix86_stack_protector_guard = TARGET_HAS_BIONIC? SSP_GLOBAL : SSP_TLS;
+    }
 }

 /* Implement the TARGET_OPTION_OVERRIDE hook.  */
Index: gcc/config/i386/i386.h
===================================================================
--- gcc/config/i386/i386.h (revision 197837)
+++ gcc/config/i386/i386.h (working copy)
@@ -486,6 +486,9 @@ 
 #define TARGET_TLS_DIRECT_SEG_REFS_DEFAULT 0
 #endif

+#define TARGET_SSP_GLOBAL_GUARD (ix86_stack_protector_guard == SSP_GLOBAL)
+#define TARGET_SSP_TLS_GUARD    (ix86_stack_protector_guard == SSP_TLS)
+
 /* Fence to use after loop using storent.  */

 extern tree x86_mfence;
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi (revision 197837)
+++ gcc/doc/invoke.texi (working copy)
@@ -656,7 +656,8 @@ 
 -mcmodel=@var{code-model} -mabi=@var{name} -maddress-mode=@var{mode} @gol
 -m32 -m64 -mx32 -mlarge-data-threshold=@var{num} @gol
 -msse2avx -mfentry -m8bit-idiv @gol
--mavx256-split-unaligned-load -mavx256-split-unaligned-store}
+-mavx256-split-unaligned-load -mavx256-split-unaligned-store @gol
+-mstack-protector-guard=@var{guard}}

 @emph{i386 and x86-64 Windows Options}
 @gccoptlist{-mconsole -mcygwin -mno-cygwin -mdll @gol
@@ -14521,6 +14522,13 @@ 
 @opindex avx256-split-unaligned-store
 Split 32-byte AVX unaligned load and store.

+@item -mstack-protector-guard=@var{guard}
+@opindex mstack-protector-guard=@var{guard}
+Generate stack protection code using canary at @var{guard}.  Supported
+locations are @samp{global} or @samp{tls} per thread at %gs:20 (the default).
+This option has effect only when @option{-fstack-protector}
+or @option{-fstack-protector-all} is also specified.
+
 @end table

 These @samp{-m} switches are supported in addition to the above