diff mbox

[Android] Runtime stack protector enabling for Android target

Message ID CAKdSQZm6v1m5oDhtY2YAyz46ZEoqOqFjhDSJ3i7UK5BT-ayBAg@mail.gmail.com
State New
Headers show

Commit Message

Igor Zamyatin July 9, 2012, 8:51 p.m. UTC 
Let's try with this patch then. Is it ok for trunk after all
appropriate testing is done?

ChangeLog:

2012-07-09 Sergey Melnikov <sergey.melnikov@intel.com>

         * config/i386/i386.md (stack_protect_set): Disable the pattern
         for Android since Android libc (bionic) does not provide random
         value for stack protection guard at gs:0x14. Guard value
         will be provided from external symbol (default implementation).
         (stack_protect_set_<mode>): Likewise.
         (stack_protect_test): Likewise.
         (stack_protect_test_<mode>): Likewise.



@@ -17997,7 +17997,7 @@
   [(match_operand 0 "memory_operand" "")
    (match_operand 1 "memory_operand" "")
    (match_operand 2 "" "")]
-  ""
+  "!(flag_android && OPTION_BIONIC)"
 {
   rtx flags = gen_rtx_REG (CCZmode, FLAGS_REG);

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

Thanks,
Igor


On Fri, Jul 6, 2012 at 4:54 PM, Igor Zamyatin <izamyatin@gmail.com> wrote:
> Right, flag_android looks better, thanks.
> Probably it's even better to check also bionic (OPTION_BIONIC)
>
> On Fri, Jul 6, 2012 at 12:13 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>> On Fri, Jul 6, 2012 at 12:49 AM, Igor Zamyatin <izamyatin@gmail.com> wrote:
>>> Hi!
>>>
>>> For runtime stack protector enabling on x86 for Android we have to
>>> disable current glibc-based implementation and turn on default one
>>> since bionic doesn't use value from gs:0x14.
>>>
>>> Tested in android environment(x86_64-*-linux-android), also
>>> bootstrapped and regtested on x86_64-unknown-linux-gnu and i686-linux.
>>> Ok for trunk?
>>
>>
>>
>> I think you want flag_android and not ANDROID_DEFAULT since you could
>> use -mno-android .
>>
>> Thanks,
>> Andrew
>>
>>
>>
>>>
>>> Thanks,
>>> Igor
>>>
>>>

Comments

Maxim Kuvyrkov July 9, 2012, 10:40 p.m. UTC | #1 
On 10/07/2012, at 8:51 AM, Igor Zamyatin wrote:

> Let's try with this patch then. Is it ok for trunk after all
> appropriate testing is done?

Sorry for the late reply, I've been down with a virus last week.

Uros,

Would you please approve this patch for i386 provided my review below?

> 
> ChangeLog:
> 
> 2012-07-09 Sergey Melnikov <sergey.melnikov@intel.com>
> 
>         * config/i386/i386.md (stack_protect_set): Disable the pattern
>         for Android since Android libc (bionic) does not provide random
>         value for stack protection guard at gs:0x14. Guard value
>         will be provided from external symbol (default implementation).
>         (stack_protect_set_<mode>): Likewise.
>         (stack_protect_test): Likewise.
>         (stack_protect_test_<mode>): Likewise.
> 
> 
> diff --git a/gcc-4.6/gcc/config/i386/i386.md b/gcc-4.6/gcc/config/i386/i386.md
> index b1d7e5e..c4dd6e3 100644
> --- a/gcc-4.6/gcc/config/i386/i386.md
> +++ b/gcc-4.6/gcc/config/i386/i386.md
> @@ -17955,7 +17955,7 @@
> (define_expand "stack_protect_set"
>   [(match_operand 0 "memory_operand" "")
>    (match_operand 1 "memory_operand" "")]
> -  ""
> +  "!(flag_android && OPTION_BIONIC)"

This should be just !OPTION_BIONIC, no flag_android.  Same applies below.

OK with that change.

-mandroid, which sets flag_android, tells the compiler to use compilation mode suitable for Android, like don't use C++ exceptions.  The particular implementation of stack protector, on the other hand, is a feature related to the C library.

> {
>   rtx (*insn)(rtx, rtx);
> 
> @@ -17979,7 +17979,7 @@
> 	(unspec:P [(match_operand:P 1 "memory_operand" "m")] UNSPEC_SP_SET))
>    (set (match_scratch:P 2 "=&r") (const_int 0))
>    (clobber (reg:CC FLAGS_REG))]
> -  ""
> +  "!(flag_android && OPTION_BIONIC)"
>   "mov{<imodesuffix>}\t{%1, %2|%2, %1}\;mov{<imodesuffix>}\t{%2,
> %0|%0, %2}\;xor{l}\t%k2, %k2"
>   [(set_attr "type" "multi")])
> 
> @@ -17997,7 +17997,7 @@
>   [(match_operand 0 "memory_operand" "")
>    (match_operand 1 "memory_operand" "")
>    (match_operand 2 "" "")]
> -  ""
> +  "!(flag_android && OPTION_BIONIC)"
> {
>   rtx flags = gen_rtx_REG (CCZmode, FLAGS_REG);
> 
> @@ -18027,7 +18027,7 @@
> 		     (match_operand:P 2 "memory_operand" "m")]
> 		    UNSPEC_SP_TEST))
>    (clobber (match_scratch:P 3 "=&r"))]
> -  ""
> +  "!(flag_android && OPTION_BIONIC)"
>   "mov{<imodesuffix>}\t{%1, %3|%3, %1}\;xor{<imodesuffix>}\t{%2, %3|%3, %2}"
>   [(set_attr "type" "multi")])

Thanks,

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics
Uros Bizjak July 10, 2012, 5:56 a.m. UTC | #2 
On Tue, Jul 10, 2012 at 12:40 AM, Maxim Kuvyrkov <maxim@codesourcery.com> wrote:
> On 10/07/2012, at 8:51 AM, Igor Zamyatin wrote:
>
>> Let's try with this patch then. Is it ok for trunk after all
>> appropriate testing is done?
>
> Sorry for the late reply, I've been down with a virus last week.
>
> Uros,
>
> Would you please approve this patch for i386 provided my review below?
>
>>
>> ChangeLog:
>>
>> 2012-07-09 Sergey Melnikov <sergey.melnikov@intel.com>
>>
>>         * config/i386/i386.md (stack_protect_set): Disable the pattern
>>         for Android since Android libc (bionic) does not provide random
>>         value for stack protection guard at gs:0x14. Guard value
>>         will be provided from external symbol (default implementation).
>>         (stack_protect_set_<mode>): Likewise.
>>         (stack_protect_test): Likewise.
>>         (stack_protect_test_<mode>): Likewise.

This is  OK.

BTW: Since the patch touches only Android specific functionality (and
doesn't affect generic functionality), your approval should be enough.

Thanks,
Uros.
Igor Zamyatin July 23, 2012, 8 p.m. UTC | #3 
Hi!

Since this change for stack-protector enabling hurts mingw compiler
(see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53980 and
http://gcc.gnu.org/ml/gcc-patches/2012-07/msg00638.html) we'd like to
make change more general.
Please see new patch in attachment.
Tested in android environment(x86_64-*-linux-android), also
bootstrapped and regtested on x86_64-unknown-linux-gnu and i686-linux.
Also mingw bootstrap was checked.
Ok for trunk?

Thanks,
Igor


Changelog:

2012-07-23 Sergey Melnikov <sergey.melnikov@intel.com>

        * config/i386/i386.md (stack_protect_set): Disable the pattern
        for Android since Android libc (bionic) does not provide random
        value for stack protection guard at gs:0x14. Guard value
        will be provided from external symbol (default implementation).
        (stack_protect_set_<mode>): Likewise.
        (stack_protect_test): Likewise.
        (stack_protect_test_<mode>): Likewise.
        * gcc/defaults.h: Define macro TARGET_HAS_BIONIC to 0 - target does
        not have Bionic by default
        * config/linux.h: Redefine macro TARGET_HAS_BIONIC to (OPTION_BIONIC)
        Macro OPTION_BIONIC is defined in this file and provides Bionic
        accessibility status
Uros Bizjak July 24, 2012, 10:08 a.m. UTC | #4 
On Mon, Jul 23, 2012 at 10:00 PM, Igor Zamyatin <izamyatin@gmail.com> wrote:

> 2012-07-23 Sergey Melnikov <sergey.melnikov@intel.com>
>
>         * config/i386/i386.md (stack_protect_set): Disable the pattern
>         for Android since Android libc (bionic) does not provide random
>         value for stack protection guard at gs:0x14. Guard value
>         will be provided from external symbol (default implementation).
>         (stack_protect_set_<mode>): Likewise.
>         (stack_protect_test): Likewise.
>         (stack_protect_test_<mode>): Likewise.
>         * gcc/defaults.h: Define macro TARGET_HAS_BIONIC to 0 - target does
>         not have Bionic by default
>         * config/linux.h: Redefine macro TARGET_HAS_BIONIC to (OPTION_BIONIC)
>         Macro OPTION_BIONIC is defined in this file and provides Bionic
>         accessibility status

Looks OK to me, patch needs approval from Maxim.

Thanks,
Uros.
Maxim Kuvyrkov July 24, 2012, 10:08 p.m. UTC | #5 
On 24/07/2012, at 10:08 PM, Uros Bizjak wrote:

> On Mon, Jul 23, 2012 at 10:00 PM, Igor Zamyatin <izamyatin@gmail.com> wrote:
> 
>> 2012-07-23 Sergey Melnikov <sergey.melnikov@intel.com>
>> 
>>        * config/i386/i386.md (stack_protect_set): Disable the pattern
>>        for Android since Android libc (bionic) does not provide random
>>        value for stack protection guard at gs:0x14. Guard value
>>        will be provided from external symbol (default implementation).
>>        (stack_protect_set_<mode>): Likewise.
>>        (stack_protect_test): Likewise.
>>        (stack_protect_test_<mode>): Likewise.
>>        * gcc/defaults.h: Define macro TARGET_HAS_BIONIC to 0 - target does
>>        not have Bionic by default
>>        * config/linux.h: Redefine macro TARGET_HAS_BIONIC to (OPTION_BIONIC)
>>        Macro OPTION_BIONIC is defined in this file and provides Bionic
>>        accessibility status
> 
> Looks OK to me, patch needs approval from Maxim.

OK.  Thanks for fixing this.

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics
Igor Zamyatin Aug. 8, 2012, 7:54 a.m. UTC | #6 
Hi all!

I'd like to ask whether stack-protector changes for Android could go to 4.7?

Pathes are:

http://gcc.gnu.org/ml/gcc-patches/2012-05/msg01089.html
http://gcc.gnu.org/ml/gcc-patches/2012-07/msg01157.html

Thanks,
Igor

On Wed, Jul 25, 2012 at 2:08 AM, Maxim Kuvyrkov <maxim@codesourcery.com> wrote:
> On 24/07/2012, at 10:08 PM, Uros Bizjak wrote:
>
>> On Mon, Jul 23, 2012 at 10:00 PM, Igor Zamyatin <izamyatin@gmail.com> wrote:
>>
>>> 2012-07-23 Sergey Melnikov <sergey.melnikov@intel.com>
>>>
>>>        * config/i386/i386.md (stack_protect_set): Disable the pattern
>>>        for Android since Android libc (bionic) does not provide random
>>>        value for stack protection guard at gs:0x14. Guard value
>>>        will be provided from external symbol (default implementation).
>>>        (stack_protect_set_<mode>): Likewise.
>>>        (stack_protect_test): Likewise.
>>>        (stack_protect_test_<mode>): Likewise.
>>>        * gcc/defaults.h: Define macro TARGET_HAS_BIONIC to 0 - target does
>>>        not have Bionic by default
>>>        * config/linux.h: Redefine macro TARGET_HAS_BIONIC to (OPTION_BIONIC)
>>>        Macro OPTION_BIONIC is defined in this file and provides Bionic
>>>        accessibility status
>>
>> Looks OK to me, patch needs approval from Maxim.
>
> OK.  Thanks for fixing this.
>
> --
> Maxim Kuvyrkov
> CodeSourcery / Mentor Graphics
>
Uros Bizjak Aug. 8, 2012, 9:46 a.m. UTC | #7 
On Wed, Aug 8, 2012 at 9:54 AM, Igor Zamyatin <izamyatin@gmail.com> wrote:

> I'd like to ask whether stack-protector changes for Android could go to 4.7?
>
> Pathes are:
>
> http://gcc.gnu.org/ml/gcc-patches/2012-05/msg01089.html
> http://gcc.gnu.org/ml/gcc-patches/2012-07/msg01157.html

OK, as far as x86 is concerned.

Thanks,
Uros.
Maxim Kuvyrkov Aug. 8, 2012, 10:25 a.m. UTC | #8 
On 8/08/2012, at 9:46 PM, Uros Bizjak wrote:

> On Wed, Aug 8, 2012 at 9:54 AM, Igor Zamyatin <izamyatin@gmail.com> wrote:
> 
>> I'd like to ask whether stack-protector changes for Android could go to 4.7?
>> 
>> Pathes are:
>> 
>> http://gcc.gnu.org/ml/gcc-patches/2012-05/msg01089.html
>> http://gcc.gnu.org/ml/gcc-patches/2012-07/msg01157.html
> 
> OK, as far as x86 is concerned.

Strictly speaking, these fixes are not for a regression, so do not readily qualify for a release branch.  However, since the patches are no-ops on targets other than x86 and Uros OK'd them for x86 ...

OK, provided that the patches in the above threads apply without conflicts.  If there are conflicts, please repost for review.

Thanks,

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics
Igor Zamyatin Aug. 10, 2012, 1:05 p.m. UTC | #9 
Hi!

There were only trivial merge conflicts like

<<<
  [(match_operand 0 "memory_operand" "")
   (match_operand 1 "memory_operand" "")]
  ""
---
  [(match_operand 0 "memory_operand")
   (match_operand 1 "memory_operand")]
  "!TARGET_HAS_BIONIC"
>>>

Patch attached.

All necessary testing passed. Ok for 4.7?

Changelog:


2012-08-08 Pavel Chupin <pavel.v.chupin@intel.com>

	Backport from mainline r189840 and r187586:

	2012-07-25 Sergey Melnikov <sergey.melnikov@intel.com>

	* config/i386/i386.md (stack_protect_set): Disable the pattern
	for Android since Android libc (bionic) does not provide random
	value for stack protection guard at gs:0x14. Guard value
	will be provided from external symbol (default implementation).
	(stack_protect_set_<mode>): Likewise.
	(stack_protect_test): Likewise.
	(stack_protect_test_<mode>): Likewise.
	* gcc/defaults.h: Define macro TARGET_HAS_BIONIC to 0 - target does
	not have Bionic by default
	* config/linux.h: Redefine macro TARGET_HAS_BIONIC to (OPTION_BIONIC)
	Macro OPTION_BIONIC is defined in this file and provides Bionic
	accessibility status

	2012-05-16  Igor Zamyatin  <igor.zamyatin@intel.com>

	* configure.ac: Stack protector enabling for Android targets.
	* configure: Regenerate.


On Wed, Aug 8, 2012 at 2:25 PM, Maxim Kuvyrkov <maxim@codesourcery.com> wrote:
> On 8/08/2012, at 9:46 PM, Uros Bizjak wrote:
>
>> On Wed, Aug 8, 2012 at 9:54 AM, Igor Zamyatin <izamyatin@gmail.com> wrote:
>>
>>> I'd like to ask whether stack-protector changes for Android could go to 4.7?
>>>
>>> Pathes are:
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2012-05/msg01089.html
>>> http://gcc.gnu.org/ml/gcc-patches/2012-07/msg01157.html
>>
>> OK, as far as x86 is concerned.
>
> Strictly speaking, these fixes are not for a regression, so do not readily qualify for a release branch.  However, since the patches are no-ops on targets other than x86 and Uros OK'd them for x86 ...
>
> OK, provided that the patches in the above threads apply without conflicts.  If there are conflicts, please repost for review.
>
> Thanks,
>
> --
> Maxim Kuvyrkov
> CodeSourcery / Mentor Graphics
>
Kirill Yukhin Aug. 14, 2012, 2:01 p.m. UTC | #10 
> OK, provided that the patches in the above threads apply without conflicts.  If there are conflicts, please repost for review.

Comitted to 4.7 branch: http://gcc.gnu.org/ml/gcc-cvs/2012-08/msg00360.html

Thanks, K
diff mbox

Patch

diff --git a/gcc-4.6/gcc/config/i386/i386.md b/gcc-4.6/gcc/config/i386/i386.md
index b1d7e5e..c4dd6e3 100644
--- a/gcc-4.6/gcc/config/i386/i386.md
+++ b/gcc-4.6/gcc/config/i386/i386.md
@@ -17955,7 +17955,7 @@ 
 (define_expand "stack_protect_set"
   [(match_operand 0 "memory_operand" "")
    (match_operand 1 "memory_operand" "")]
-  ""
+  "!(flag_android && OPTION_BIONIC)"
 {
   rtx (*insn)(rtx, rtx);

@@ -17979,7 +17979,7 @@ 
 	(unspec:P [(match_operand:P 1 "memory_operand" "m")] UNSPEC_SP_SET))
    (set (match_scratch:P 2 "=&r") (const_int 0))
    (clobber (reg:CC FLAGS_REG))]
-  ""
+  "!(flag_android && OPTION_BIONIC)"
   "mov{<imodesuffix>}\t{%1, %2|%2, %1}\;mov{<imodesuffix>}\t{%2,
%0|%0, %2}\;xor{l}\t%k2, %k2"
   [(set_attr "type" "multi")])