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

Changed comment, corrected formatting, and removed Init(SSP_TLS).  Thanks!


On Fri, Apr 12, 2013 at 5:49 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> 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.

One question: If I drop Init(SSP_TLS), I got the following error
during compilation:
   options.c:1122:1: error: invalid conversion from 'int' to
'stack_protector_guard' [-fpermissive]
It's because w/o Init(SSP_TLS), '0' is placed in global_options_init
where enum stack_protector_guard is expected.
Used to be okay with C but no good in 4.8+ since C++ becomes
implementation language.

What is the best way to deal with it?

On Fri, Apr 12, 2013 at 6:08 PM, Andrew Hsieh <andrewhsieh@google.com> wrote:
> Changed comment, corrected formatting, and removed Init(SSP_TLS).  Thanks!
>
> =====
> 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)
> +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 in TLS block */
> +  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,10 @@
>    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
>
> On Fri, Apr 12, 2013 at 5:49 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> 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.
>
>
>
> --
> Thanks,
> Andrew

On Fri, Apr 12, 2013 at 12:28 PM, Andrew Hsieh <andrewhsieh@google.com> wrote:
> One question: If I drop Init(SSP_TLS), I got the following error
> during compilation:
>    options.c:1122:1: error: invalid conversion from 'int' to
> 'stack_protector_guard' [-fpermissive]
> It's because w/o Init(SSP_TLS), '0' is placed in global_options_init
> where enum stack_protector_guard is expected.
> Used to be okay with C but no good in 4.8+ since C++ becomes
> implementation language.
>
> What is the best way to deal with it?

Just leave Init(SSP_TLS) for now, I have CC'd Joseph for the problem
with option system.

Uros.