Fix PR43404, PR48470, PR64744 ICE on naked functions

Fix PR43404, PR48470, PR64744 ICE on naked functions

[Alexander Basov]

[-- Attachment #1: Type: text/plain, Size: 249 bytes --]

Hi,
this patch fixes ICE when compiling naked functions for arm targets.
It prevents register allocation for non-register things like volatile,
float, BLKMode vars.

Tested on trunk with arm-v7ar-linux-gnueabi on qemu vexpress board.

-- 
Alexander

[-- Attachment #2: pr64744.patch --]
[-- Type: text/x-patch, Size: 5598 bytes --]

2015-06-01 Alexander Basov <coohpt@gmail.com>

    	PR middle-end/64744
    	PR middle-end/48470
    	PR middle-end/43404

    	* gcc/cfgexpand.c (expand_one_var): Add check if stack is going to
    	be used in naked function.
    	* gcc/expr.c (expand_expr_addr_expr_1): Remove exscess checking
    	whether expression should not reside in MEM.
        * gcc/function.c (use_register_for_decl): Do not use registers for
    	non-register things (volatile, float, BLKMode) in naked functions.

    	* gcc/testsuite/gcc.target/arm/pr43404.c : New testcase.
    	* gcc/testsuite/gcc.target/arm/pr48470.c : New testcase.
    	* gcc/testsuite/gcc.target/arm/pr64744-1.c : New testcase.
        * gcc/testsuite/gcc.target/arm/pr64744-2.c : New testcase.

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index b190f91..c6db8a9 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1382,7 +1382,15 @@ expand_one_var (tree var, bool toplevel, bool really_expand)
   else
     {
       if (really_expand)
-        expand_one_stack_var (origvar);
+        {
+          if (!targetm.calls.allocate_stack_slots_for_args ())
+            error ("cannot allocate stack for variable %q+D, naked function.",
+                   var);
+
+          expand_one_stack_var (origvar);
+        }
+
+
       return tree_to_uhwi (DECL_SIZE_UNIT (var));
     }
   return 0;
diff --git a/gcc/expr.c b/gcc/expr.c
index 5a931dc..4d728bc 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -7646,15 +7646,7 @@ expand_expr_addr_expr_1 (tree exp, rtx target, machine_mode tmode,
 	     marked TREE_ADDRESSABLE, which will be either a front-end
 	     or a tree optimizer bug.  */

-	  if (TREE_ADDRESSABLE (exp)
-	      && ! MEM_P (result)
-	      && ! targetm.calls.allocate_stack_slots_for_args ())
-	    {
-	      error ("local frame unavailable (naked function?)");
-	      return result;
-	    }
-	  else
-	    gcc_assert (MEM_P (result));
+	  gcc_assert (MEM_P (result));
 	  result = XEXP (result, 0);

 	  /* ??? Is this needed anymore?  */
diff --git a/gcc/function.c b/gcc/function.c
index 7d2d7e4..0cafe12 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -2121,9 +2121,6 @@ aggregate_value_p (const_tree exp, const_tree fntype)
 bool
 use_register_for_decl (const_tree decl)
 {
-  if (!targetm.calls.allocate_stack_slots_for_args ())
-    return true;
-
   /* Honor volatile.  */
   if (TREE_SIDE_EFFECTS (decl))
     return false;
@@ -2151,6 +2148,9 @@ use_register_for_decl (const_tree decl)
   if (flag_float_store && FLOAT_TYPE_P (TREE_TYPE (decl)))
     return false;

+  if (!targetm.calls.allocate_stack_slots_for_args ())
+    return true;
+
   /* If we're not interested in tracking debugging information for
      this decl, then we can certainly put it in a register.  */
   if (DECL_IGNORED_P (decl))
diff --git a/gcc/testsuite/gcc.target/arm/pr43404.c b/gcc/testsuite/gcc.target/arm/pr43404.c
new file mode 100644
index 0000000..4f2291d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr43404.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+__attribute__ ((naked))
+void __data_abort(void)
+{
+  long foo; /* { dg-error "cannot allocate stack for variable" } */
+  long* bar = &foo;
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr48470.c b/gcc/testsuite/gcc.target/arm/pr48470.c
new file mode 100644
index 0000000..20343e7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr48470.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+extern void g(int *x);
+
+void __attribute__((naked)) f(void)
+{
+    int x = 0; /* { dg-error "cannot allocate stack for variable" } */
+    g(&x);
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr64744-1.c b/gcc/testsuite/gcc.target/arm/pr64744-1.c
new file mode 100644
index 0000000..4029303
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr64744-1.c
@@ -0,0 +1,40 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+__attribute__((naked))
+void foo1 ()
+{
+  int aa = 0;
+  int ab = {0};
+}
+
+__attribute__((naked))
+void foo2() {
+  char aa [ ] = {}; /* { dg-error "cannot allocate stack for variable" } */
+  char ab [1] = {};
+  char ac [2] = {}; /* { dg-error "cannot allocate stack for variable" } */
+  char ad [3] = {}; /* { dg-error "cannot allocate stack for variable" } */
+}
+
+__attribute__((naked))
+void foo3() {
+  char aa [1] = {0};
+  char ab [2] = {0}; /* { dg-error "cannot allocate stack for variable" } */
+  char ac [3] = {0}; /* { dg-error "cannot allocate stack for variable" } */
+  char ad [4] = {0}; /* { dg-error "cannot allocate stack for variable" } */
+}
+
+__attribute__((naked))
+void foo4() {
+  char aa [2] = {0,0}; /* { dg-error "cannot allocate stack for variable" } */
+}
+__attribute__((naked))
+void foo5() {
+  char aa [3] = {0,0,0}; /* { dg-error "cannot allocate stack for variable" } */
+}
+
+__attribute__((naked))
+void foo6() {
+  char aa [4] = {0,0,0,0}; /* { dg-error "cannot allocate stack for variable" } */
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr64744-2.c b/gcc/testsuite/gcc.target/arm/pr64744-2.c
new file mode 100644
index 0000000..d33ea7b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr64744-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+struct s {
+  char a;
+    int b;
+};
+
+__attribute__((naked))
+void foo () {
+  struct s x = {}; /* { dg-error "cannot allocate stack for variable" } */
+}

Re: Fix PR43404, PR48470, PR64744 ICE on naked functions

[Jeff Law]

On 06/01/2015 04:12 AM, Alexander Basov wrote:
> Hi,
> this patch fixes ICE when compiling naked functions for arm targets.
> It prevents register allocation for non-register things like volatile,
> float, BLKMode vars.
>
> Tested on trunk with arm-v7ar-linux-gnueabi on qemu vexpress board.
>
> -- Alexander
>
>
> pr64744.patch
>
>
> 2015-06-01 Alexander Basov<coohpt@gmail.com>
>
>      	PR middle-end/64744
>      	PR middle-end/48470
>      	PR middle-end/43404
>
>      	* gcc/cfgexpand.c (expand_one_var): Add check if stack is going to
>      	be used in naked function.
>      	* gcc/expr.c (expand_expr_addr_expr_1): Remove exscess checking
>      	whether expression should not reside in MEM.
>          * gcc/function.c (use_register_for_decl): Do not use registers for
>      	non-register things (volatile, float, BLKMode) in naked functions.
Lose the "gcc/" prefix on these entries as there's a ChangeLog in the 
gcc/ subdirectory.

>
>      	* gcc/testsuite/gcc.target/arm/pr43404.c : New testcase.
>      	* gcc/testsuite/gcc.target/arm/pr48470.c : New testcase.
>      	* gcc/testsuite/gcc.target/arm/pr64744-1.c : New testcase.
>          * gcc/testsuite/gcc.target/arm/pr64744-2.c : New testcase.
Similarly, lose the gcc/testsuite prefix on these entries as this will 
be installed in gcc/testsuite/ChangeLog.  However, do copy the PR markers.


>
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index b190f91..c6db8a9 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -1382,7 +1382,15 @@ expand_one_var (tree var, bool toplevel, bool really_expand)
>     else
>       {
>         if (really_expand)
> -        expand_one_stack_var (origvar);
> +        {
> +          if (!targetm.calls.allocate_stack_slots_for_args ())
> +            error ("cannot allocate stack for variable %q+D, naked function.",
> +                   var);
> +
> +          expand_one_stack_var (origvar);
> +        }
So how do you know ORIGVAR is an argument here before issuing the error? 
  ie, shouldn't you verify that the underlying object is a PARM_DECL? 
If there's some way we already know we're dealing with a PARM_DECL, then 
just say so.

I'd rewrite the user_register_for_decl ChangeLog entry to
	* function.c (use_register_for_decl): Correct location
	of allocate_stack_slot_for_args test.


With those changes this should be ready to go, please make the updates 
and repost.

Thanks,
jeff

Re: Fix PR43404, PR48470, PR64744 ICE on naked functions

[Alexander Basov]

[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]

Hello Jeff,
please find updated patch attached

03.06.2015 00:18, Jeff Law wrote:
> On 06/01/2015 04:12 AM, Alexander Basov wrote:
>> Hi,
>> this patch fixes ICE when compiling naked functions for arm targets.
>> It prevents register allocation for non-register things like volatile,
>> float, BLKMode vars.
>>
>> Tested on trunk with arm-v7ar-linux-gnueabi on qemu vexpress board.
>>
>> -- Alexander
>>
>>
>> pr64744.patch
>>
>>
>> 2015-06-01 Alexander Basov<coohpt@gmail.com>
>>
>>          PR middle-end/64744
>>          PR middle-end/48470
>>          PR middle-end/43404
>>
>>          * gcc/cfgexpand.c (expand_one_var): Add check if stack is
>> going to
>>          be used in naked function.
>>          * gcc/expr.c (expand_expr_addr_expr_1): Remove exscess checking
>>          whether expression should not reside in MEM.
>>          * gcc/function.c (use_register_for_decl): Do not use
>> registers for
>>          non-register things (volatile, float, BLKMode) in naked
>> functions.
> Lose the "gcc/" prefix on these entries as there's a ChangeLog in the
> gcc/ subdirectory.
>
>>
>>          * gcc/testsuite/gcc.target/arm/pr43404.c : New testcase.
>>          * gcc/testsuite/gcc.target/arm/pr48470.c : New testcase.
>>          * gcc/testsuite/gcc.target/arm/pr64744-1.c : New testcase.
>>          * gcc/testsuite/gcc.target/arm/pr64744-2.c : New testcase.
> Similarly, lose the gcc/testsuite prefix on these entries as this will
> be installed in gcc/testsuite/ChangeLog.  However, do copy the PR
> markers.
>
>
>>
>> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
>> index b190f91..c6db8a9 100644
>> --- a/gcc/cfgexpand.c
>> +++ b/gcc/cfgexpand.c
>> @@ -1382,7 +1382,15 @@ expand_one_var (tree var, bool toplevel, bool
>> really_expand)
>>     else
>>       {
>>         if (really_expand)
>> -        expand_one_stack_var (origvar);
>> +        {
>> +          if (!targetm.calls.allocate_stack_slots_for_args ())
>> +            error ("cannot allocate stack for variable %q+D, naked
>> function.",
>> +                   var);
>> +
>> +          expand_one_stack_var (origvar);
>> +        }
> So how do you know ORIGVAR is an argument here before issuing the
> error?  ie, shouldn't you verify that the underlying object is a
> PARM_DECL? If there's some way we already know we're dealing with a
> PARM_DECL, then just say so.
In case of naked function stack should not be used not only for function
args, but also for any local variables.
So, i think we don't need to check if underlying object is a PARM_DECL.
>
> I'd rewrite the user_register_for_decl ChangeLog entry to
>     * function.c (use_register_for_decl): Correct location
>     of allocate_stack_slot_for_args test.
>
>
> With those changes this should be ready to go, please make the updates
> and repost.
>
> Thanks,
> jeff

--
Alexander


[-- Attachment #2: pr64744.patch --]
[-- Type: text/x-patch, Size: 5865 bytes --]

commit 09f6c3e35a2e2ac87486b546b139ba9b8b8e52a1
Author: Alexander <coopht@gmail.com>
Date:   Wed Jun 3 22:41:58 2015 +0300

    2015-06-01 Alexander Basov <coohpt@gmail.com>
    
        	PR middle-end/64744
        	PR middle-end/48470
        	PR middle-end/43404
    
        	* cfgexpand.c (expand_one_var): Add check if stack is going to
        	be used in naked function.
        	* expr.c (expand_expr_addr_expr_1): Remove exscess checking
        	whether expression should not reside in MEM.
        	* function.c (use_register_for_decl): Correct location
        	of allocate_stack_slot_for_args test.
    
    2015-06-01 Alexander Basov <coohpt@gmail.com>
    
        	PR middle-end/64744
        	PR middle-end/48470
        	PR middle-end/43404
    
        	* testsuite/gcc.target/arm/pr43404.c: New testcase.
        	* testsuite/gcc.target/arm/pr48470.c: New testcase.
        	* testsuite/gcc.target/arm/pr64744-1.c: New testcase.
        	* testsuite/gcc.target/arm/pr64744-2.c: New testcase.

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index b190f91..c6db8a9 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1382,7 +1382,15 @@ expand_one_var (tree var, bool toplevel, bool really_expand)
   else
     {
       if (really_expand)
-        expand_one_stack_var (origvar);
+        {
+          if (!targetm.calls.allocate_stack_slots_for_args ())
+            error ("cannot allocate stack for variable %q+D, naked function.",
+                   var);
+
+          expand_one_stack_var (origvar);
+        }
+
+
       return tree_to_uhwi (DECL_SIZE_UNIT (var));
     }
   return 0;
diff --git a/gcc/expr.c b/gcc/expr.c
index 5a931dc..4d728bc 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -7646,15 +7646,7 @@ expand_expr_addr_expr_1 (tree exp, rtx target, machine_mode tmode,
 	     marked TREE_ADDRESSABLE, which will be either a front-end
 	     or a tree optimizer bug.  */
 
-	  if (TREE_ADDRESSABLE (exp)
-	      && ! MEM_P (result)
-	      && ! targetm.calls.allocate_stack_slots_for_args ())
-	    {
-	      error ("local frame unavailable (naked function?)");
-	      return result;
-	    }
-	  else
-	    gcc_assert (MEM_P (result));
+	  gcc_assert (MEM_P (result));
 	  result = XEXP (result, 0);
 
 	  /* ??? Is this needed anymore?  */
diff --git a/gcc/function.c b/gcc/function.c
index 7d2d7e4..0cafe12 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -2121,9 +2121,6 @@ aggregate_value_p (const_tree exp, const_tree fntype)
 bool
 use_register_for_decl (const_tree decl)
 {
-  if (!targetm.calls.allocate_stack_slots_for_args ())
-    return true;
-
   /* Honor volatile.  */
   if (TREE_SIDE_EFFECTS (decl))
     return false;
@@ -2151,6 +2148,9 @@ use_register_for_decl (const_tree decl)
   if (flag_float_store && FLOAT_TYPE_P (TREE_TYPE (decl)))
     return false;
 
+  if (!targetm.calls.allocate_stack_slots_for_args ())
+    return true;
+
   /* If we're not interested in tracking debugging information for
      this decl, then we can certainly put it in a register.  */
   if (DECL_IGNORED_P (decl))
diff --git a/gcc/testsuite/gcc.target/arm/pr43404.c b/gcc/testsuite/gcc.target/arm/pr43404.c
new file mode 100644
index 0000000..4f2291d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr43404.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+__attribute__ ((naked))
+void __data_abort(void)
+{
+  long foo; /* { dg-error "cannot allocate stack for variable" } */
+  long* bar = &foo;
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr48470.c b/gcc/testsuite/gcc.target/arm/pr48470.c
new file mode 100644
index 0000000..20343e7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr48470.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+extern void g(int *x);
+
+void __attribute__((naked)) f(void)
+{
+    int x = 0; /* { dg-error "cannot allocate stack for variable" } */
+    g(&x);
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr64744-1.c b/gcc/testsuite/gcc.target/arm/pr64744-1.c
new file mode 100644
index 0000000..4029303
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr64744-1.c
@@ -0,0 +1,40 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+__attribute__((naked))
+void foo1 ()
+{
+  int aa = 0;
+  int ab = {0};
+}
+
+__attribute__((naked))
+void foo2() {
+  char aa [ ] = {}; /* { dg-error "cannot allocate stack for variable" } */
+  char ab [1] = {};
+  char ac [2] = {}; /* { dg-error "cannot allocate stack for variable" } */
+  char ad [3] = {}; /* { dg-error "cannot allocate stack for variable" } */
+}
+
+__attribute__((naked))
+void foo3() {
+  char aa [1] = {0};
+  char ab [2] = {0}; /* { dg-error "cannot allocate stack for variable" } */
+  char ac [3] = {0}; /* { dg-error "cannot allocate stack for variable" } */
+  char ad [4] = {0}; /* { dg-error "cannot allocate stack for variable" } */
+}
+
+__attribute__((naked))
+void foo4() {
+  char aa [2] = {0,0}; /* { dg-error "cannot allocate stack for variable" } */
+}
+__attribute__((naked))
+void foo5() {
+  char aa [3] = {0,0,0}; /* { dg-error "cannot allocate stack for variable" } */
+}
+
+__attribute__((naked))
+void foo6() {
+  char aa [4] = {0,0,0,0}; /* { dg-error "cannot allocate stack for variable" } */
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr64744-2.c b/gcc/testsuite/gcc.target/arm/pr64744-2.c
new file mode 100644
index 0000000..d33ea7b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr64744-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+struct s {
+  char a;
+    int b;
+};
+
+__attribute__((naked))
+void foo () {
+  struct s x = {}; /* { dg-error "cannot allocate stack for variable" } */
+}

Re: Fix PR43404, PR48470, PR64744 ICE on naked functions

[Jeff Law]

On 06/03/2015 02:15 PM, Alexander Basov wrote:
> Hello Jeff,
> please find updated patch attached
>
>>> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
>>> index b190f91..c6db8a9 100644
>>> --- a/gcc/cfgexpand.c
>>> +++ b/gcc/cfgexpand.c
>>> @@ -1382,7 +1382,15 @@ expand_one_var (tree var, bool toplevel, bool
>>> really_expand)
>>>      else
>>>        {
>>>          if (really_expand)
>>> -        expand_one_stack_var (origvar);
>>> +        {
>>> +          if (!targetm.calls.allocate_stack_slots_for_args ())
>>> +            error ("cannot allocate stack for variable %q+D, naked
>>> function.",
>>> +                   var);
>>> +
>>> +          expand_one_stack_var (origvar);
>>> +        }
>> So how do you know ORIGVAR is an argument here before issuing the
>> error?  ie, shouldn't you verify that the underlying object is a
>> PARM_DECL? If there's some way we already know we're dealing with a
>> PARM_DECL, then just say so.
> In case of naked function stack should not be used not only for function
> args, but also for any local variables.
> So, i think we don't need to check if underlying object is a PARM_DECL.
Then that would indicate that we're using the wrong test 
(allocate_stack_slot_for_args).  That hook is for whether or not 
arguments should have stack slots allocated.  Yet you're issuing an 
error for more than just PARM_DECLs.

Shouldn't you instead be checking if the current function is a naked 
function or not by checking the attributes of the current function?

Jeff

Re: Fix PR43404, PR48470, PR64744 ICE on naked functions

[Alexander Basov]

2015-06-25 21:47 GMT+03:00 Jeff Law <law@redhat.com>:
> On 06/03/2015 02:15 PM, Alexander Basov wrote:
>>
>> Hello Jeff,
>> please find updated patch attached
>>
>>>> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
>>>> index b190f91..c6db8a9 100644
>>>> --- a/gcc/cfgexpand.c
>>>> +++ b/gcc/cfgexpand.c
>>>> @@ -1382,7 +1382,15 @@ expand_one_var (tree var, bool toplevel, bool
>>>> really_expand)
>>>>      else
>>>>        {
>>>>          if (really_expand)
>>>> -        expand_one_stack_var (origvar);
>>>> +        {
>>>> +          if (!targetm.calls.allocate_stack_slots_for_args ())
>>>> +            error ("cannot allocate stack for variable %q+D, naked
>>>> function.",
>>>> +                   var);
>>>> +
>>>> +          expand_one_stack_var (origvar);
>>>> +        }
>>>
>>> So how do you know ORIGVAR is an argument here before issuing the
>>> error?  ie, shouldn't you verify that the underlying object is a
>>> PARM_DECL? If there's some way we already know we're dealing with a
>>> PARM_DECL, then just say so.
>>
>> In case of naked function stack should not be used not only for function
>> args, but also for any local variables.
>> So, i think we don't need to check if underlying object is a PARM_DECL.
>
> Then that would indicate that we're using the wrong test
> (allocate_stack_slot_for_args).  That hook is for whether or not arguments
> should have stack slots allocated.  Yet you're issuing an error for more
> than just PARM_DECLs.
>
> Shouldn't you instead be checking if the current function is a naked
> function or not by checking the attributes of the current function?
>
> Jeff

What allocate_stack_slots_for_args  does, it only checks if current
function is naked or not.
May be it will be better to remove allocate_stack_slots_for_args and
replace if with explicit checking of naked attribute?

-- 
Alexander

Re: Fix PR43404, PR48470, PR64744 ICE on naked functions

[Alexander Basov]

[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]

I've updated patch with attributes lookup.
is it OK?

-- 
Alexander

2015-06-26 9:33 GMT+03:00 Alexander Basov <coopht@gmail.com>:
> 2015-06-25 21:47 GMT+03:00 Jeff Law <law@redhat.com>:
>> On 06/03/2015 02:15 PM, Alexander Basov wrote:
>>>
>>> Hello Jeff,
>>> please find updated patch attached
>>>
>>>>> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
>>>>> index b190f91..c6db8a9 100644
>>>>> --- a/gcc/cfgexpand.c
>>>>> +++ b/gcc/cfgexpand.c
>>>>> @@ -1382,7 +1382,15 @@ expand_one_var (tree var, bool toplevel, bool
>>>>> really_expand)
>>>>>      else
>>>>>        {
>>>>>          if (really_expand)
>>>>> -        expand_one_stack_var (origvar);
>>>>> +        {
>>>>> +          if (!targetm.calls.allocate_stack_slots_for_args ())
>>>>> +            error ("cannot allocate stack for variable %q+D, naked
>>>>> function.",
>>>>> +                   var);
>>>>> +
>>>>> +          expand_one_stack_var (origvar);
>>>>> +        }
>>>>
>>>> So how do you know ORIGVAR is an argument here before issuing the
>>>> error?  ie, shouldn't you verify that the underlying object is a
>>>> PARM_DECL? If there's some way we already know we're dealing with a
>>>> PARM_DECL, then just say so.
>>>
>>> In case of naked function stack should not be used not only for function
>>> args, but also for any local variables.
>>> So, i think we don't need to check if underlying object is a PARM_DECL.
>>
>> Then that would indicate that we're using the wrong test
>> (allocate_stack_slot_for_args).  That hook is for whether or not arguments
>> should have stack slots allocated.  Yet you're issuing an error for more
>> than just PARM_DECLs.
>>
>> Shouldn't you instead be checking if the current function is a naked
>> function or not by checking the attributes of the current function?
>>
>> Jeff
>
> What allocate_stack_slots_for_args  does, it only checks if current
> function is naked or not.
> May be it will be better to remove allocate_stack_slots_for_args and
> replace if with explicit checking of naked attribute?
>
> --
> Alexander

[-- Attachment #2: pr64744.patch --]
[-- Type: text/x-patch, Size: 5746 bytes --]

commit 3a72dac72beb713ab6a566728b77c4da6d297755
Author: Alexander Basov <coohpt@gmail.com>
Date:   Tue Mar 10 14:15:24 2015 +0300

    	PR middle-end/64744
    	PR middle-end/48470
    	PR middle-end/43404
    
    	* gcc/cfgexpand.c (expand_one_var): Add check if stack is going to
    	be used in naked function.
    	* gcc/expr.c (expand_expr_addr_expr_1): Remove exscess checking
    	whether expression should not reside in MEM.
        * gcc/function.c (use_register_for_decl): Do not use registers for
    	non-register things (volatile, float, BLKMode) in naked functions.
    
    	* gcc/testsuite/gcc.target/arm/pr43404.c : New testcase.
    	* gcc/testsuite/gcc.target/arm/pr48470.c : New testcase.
    	* gcc/testsuite/gcc.target/arm/pr64744-1.c : New testcase.
        * gcc/testsuite/gcc.target/arm/pr64744-2.c : New testcase.

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 05eb2ad..b7b4804 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1349,7 +1349,16 @@ expand_one_var (tree var, bool toplevel, bool really_expand)
   else
     {
       if (really_expand)
-        expand_one_stack_var (origvar);
+        {
+          if (lookup_attribute ("naked",
+                                DECL_ATTRIBUTES (current_function_decl)))
+            error ("cannot allocate stack for variable %q+D, naked function.",
+                   var);
+
+          expand_one_stack_var (origvar);
+        }
+
+
       return tree_to_uhwi (DECL_SIZE_UNIT (var));
     }
   return 0;
diff --git a/gcc/expr.c b/gcc/expr.c
index 408ae1a..34cd7de 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -7631,15 +7631,7 @@ expand_expr_addr_expr_1 (tree exp, rtx target, machine_mode tmode,
 	     marked TREE_ADDRESSABLE, which will be either a front-end
 	     or a tree optimizer bug.  */
 
-	  if (TREE_ADDRESSABLE (exp)
-	      && ! MEM_P (result)
-	      && ! targetm.calls.allocate_stack_slots_for_args ())
-	    {
-	      error ("local frame unavailable (naked function?)");
-	      return result;
-	    }
-	  else
-	    gcc_assert (MEM_P (result));
+	  gcc_assert (MEM_P (result));
 	  result = XEXP (result, 0);
 
 	  /* ??? Is this needed anymore?  */
diff --git a/gcc/function.c b/gcc/function.c
index cffe323..0866c49 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -2110,9 +2110,6 @@ aggregate_value_p (const_tree exp, const_tree fntype)
 bool
 use_register_for_decl (const_tree decl)
 {
-  if (!targetm.calls.allocate_stack_slots_for_args ())
-    return true;
-
   /* Honor volatile.  */
   if (TREE_SIDE_EFFECTS (decl))
     return false;
@@ -2140,6 +2137,9 @@ use_register_for_decl (const_tree decl)
   if (flag_float_store && FLOAT_TYPE_P (TREE_TYPE (decl)))
     return false;
 
+  if (!targetm.calls.allocate_stack_slots_for_args ())
+    return true;
+
   /* If we're not interested in tracking debugging information for
      this decl, then we can certainly put it in a register.  */
   if (DECL_IGNORED_P (decl))
diff --git a/gcc/testsuite/gcc.target/arm/pr43404.c b/gcc/testsuite/gcc.target/arm/pr43404.c
new file mode 100644
index 0000000..4f2291d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr43404.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+__attribute__ ((naked))
+void __data_abort(void)
+{
+  long foo; /* { dg-error "cannot allocate stack for variable" } */
+  long* bar = &foo;
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr48470.c b/gcc/testsuite/gcc.target/arm/pr48470.c
new file mode 100644
index 0000000..20343e7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr48470.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+extern void g(int *x);
+
+void __attribute__((naked)) f(void)
+{
+    int x = 0; /* { dg-error "cannot allocate stack for variable" } */
+    g(&x);
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr64744-1.c b/gcc/testsuite/gcc.target/arm/pr64744-1.c
new file mode 100644
index 0000000..4029303
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr64744-1.c
@@ -0,0 +1,40 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+__attribute__((naked))
+void foo1 ()
+{
+  int aa = 0;
+  int ab = {0};
+}
+
+__attribute__((naked))
+void foo2() {
+  char aa [ ] = {}; /* { dg-error "cannot allocate stack for variable" } */
+  char ab [1] = {};
+  char ac [2] = {}; /* { dg-error "cannot allocate stack for variable" } */
+  char ad [3] = {}; /* { dg-error "cannot allocate stack for variable" } */
+}
+
+__attribute__((naked))
+void foo3() {
+  char aa [1] = {0};
+  char ab [2] = {0}; /* { dg-error "cannot allocate stack for variable" } */
+  char ac [3] = {0}; /* { dg-error "cannot allocate stack for variable" } */
+  char ad [4] = {0}; /* { dg-error "cannot allocate stack for variable" } */
+}
+
+__attribute__((naked))
+void foo4() {
+  char aa [2] = {0,0}; /* { dg-error "cannot allocate stack for variable" } */
+}
+__attribute__((naked))
+void foo5() {
+  char aa [3] = {0,0,0}; /* { dg-error "cannot allocate stack for variable" } */
+}
+
+__attribute__((naked))
+void foo6() {
+  char aa [4] = {0,0,0,0}; /* { dg-error "cannot allocate stack for variable" } */
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr64744-2.c b/gcc/testsuite/gcc.target/arm/pr64744-2.c
new file mode 100644
index 0000000..d33ea7b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr64744-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target naked_functions } */
+/* { dg-options "-O0" } */
+
+struct s {
+  char a;
+    int b;
+};
+
+__attribute__((naked))
+void foo () {
+  struct s x = {}; /* { dg-error "cannot allocate stack for variable" } */
+}

Re: Fix PR43404, PR48470, PR64744 ICE on naked functions

[Alexander Basov]

ping

2015-06-29 16:32 GMT+03:00 Alexander Basov <coopht@gmail.com>:
> I've updated patch with attributes lookup.
> is it OK?
>
> --
> Alexander
>
> 2015-06-26 9:33 GMT+03:00 Alexander Basov <coopht@gmail.com>:
>> 2015-06-25 21:47 GMT+03:00 Jeff Law <law@redhat.com>:
>>> On 06/03/2015 02:15 PM, Alexander Basov wrote:
>>>>
>>>> Hello Jeff,
>>>> please find updated patch attached
>>>>
>>>>>> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
>>>>>> index b190f91..c6db8a9 100644
>>>>>> --- a/gcc/cfgexpand.c
>>>>>> +++ b/gcc/cfgexpand.c
>>>>>> @@ -1382,7 +1382,15 @@ expand_one_var (tree var, bool toplevel, bool
>>>>>> really_expand)
>>>>>>      else
>>>>>>        {
>>>>>>          if (really_expand)
>>>>>> -        expand_one_stack_var (origvar);
>>>>>> +        {
>>>>>> +          if (!targetm.calls.allocate_stack_slots_for_args ())
>>>>>> +            error ("cannot allocate stack for variable %q+D, naked
>>>>>> function.",
>>>>>> +                   var);
>>>>>> +
>>>>>> +          expand_one_stack_var (origvar);
>>>>>> +        }
>>>>>
>>>>> So how do you know ORIGVAR is an argument here before issuing the
>>>>> error?  ie, shouldn't you verify that the underlying object is a
>>>>> PARM_DECL? If there's some way we already know we're dealing with a
>>>>> PARM_DECL, then just say so.
>>>>
>>>> In case of naked function stack should not be used not only for function
>>>> args, but also for any local variables.
>>>> So, i think we don't need to check if underlying object is a PARM_DECL.
>>>
>>> Then that would indicate that we're using the wrong test
>>> (allocate_stack_slot_for_args).  That hook is for whether or not arguments
>>> should have stack slots allocated.  Yet you're issuing an error for more
>>> than just PARM_DECLs.
>>>
>>> Shouldn't you instead be checking if the current function is a naked
>>> function or not by checking the attributes of the current function?
>>>
>>> Jeff
>>
>> What allocate_stack_slots_for_args  does, it only checks if current
>> function is naked or not.
>> May be it will be better to remove allocate_stack_slots_for_args and
>> replace if with explicit checking of naked attribute?
>>
>> --
>> Alexander



-- 
Alexander

Re: Fix PR43404, PR48470, PR64744 ICE on naked functions

[Jeff Law]

On 06/26/2015 12:33 AM, Alexander Basov wrote:
> 2015-06-25 21:47 GMT+03:00 Jeff Law <law@redhat.com>:
>> On 06/03/2015 02:15 PM, Alexander Basov wrote:
>>>
>>> Hello Jeff,
>>> please find updated patch attached
>>>
>>>>> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
>>>>> index b190f91..c6db8a9 100644
>>>>> --- a/gcc/cfgexpand.c
>>>>> +++ b/gcc/cfgexpand.c
>>>>> @@ -1382,7 +1382,15 @@ expand_one_var (tree var, bool toplevel, bool
>>>>> really_expand)
>>>>>       else
>>>>>         {
>>>>>           if (really_expand)
>>>>> -        expand_one_stack_var (origvar);
>>>>> +        {
>>>>> +          if (!targetm.calls.allocate_stack_slots_for_args ())
>>>>> +            error ("cannot allocate stack for variable %q+D, naked
>>>>> function.",
>>>>> +                   var);
>>>>> +
>>>>> +          expand_one_stack_var (origvar);
>>>>> +        }
>>>>
>>>> So how do you know ORIGVAR is an argument here before issuing the
>>>> error?  ie, shouldn't you verify that the underlying object is a
>>>> PARM_DECL? If there's some way we already know we're dealing with a
>>>> PARM_DECL, then just say so.
>>>
>>> In case of naked function stack should not be used not only for function
>>> args, but also for any local variables.
>>> So, i think we don't need to check if underlying object is a PARM_DECL.
>>
>> Then that would indicate that we're using the wrong test
>> (allocate_stack_slot_for_args).  That hook is for whether or not arguments
>> should have stack slots allocated.  Yet you're issuing an error for more
>> than just PARM_DECLs.
>>
>> Shouldn't you instead be checking if the current function is a naked
>> function or not by checking the attributes of the current function?
>>
>> Jeff
>
> What allocate_stack_slots_for_args  does, it only checks if current
> function is naked or not.
> May be it will be better to remove allocate_stack_slots_for_args and
> replace if with explicit checking of naked attribute?
allocate_stack_slots_for_args is used for more than just naked 
functions.  It indicates whether or not the compiler should try to keep 
arguments in registers or in stack slots when optimization is disabled.

The implementation of allocate_stack_slots_for_args has special 
constraints in a naked function (should always return false).

One could easily argue that if we have a naked function, then having the 
generic parts of the compiler do the right thing rather than having each 
port that defines allocate_stack_slot_for_args handle naked functions 
specially would be an interface improvement -- and I would agree.   That 
would probably be a useful follow-up.

Anyway, I still need to look at your follow-up patch...  It's in the queue.

Jeff

Re: Fix PR43404, PR48470, PR64744 ICE on naked functions

[Jeff Law]

On 06/29/2015 07:32 AM, Alexander Basov wrote:
> I've updated patch with attributes lookup.
> is it OK?
>
> -- Alexander 2015-06-26 9:33 GMT+03:00 Alexander Basov <coopht@gmail.com>:
>> >2015-06-25 21:47 GMT+03:00 Jeff Law<law@redhat.com>:
>>> >>On 06/03/2015 02:15 PM, Alexander Basov wrote:
>>>> >>>
>>>> >>>Hello Jeff,
>>>> >>>please find updated patch attached
>>>> >>>
>>>>>> >>>>>diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
>>>>>> >>>>>index b190f91..c6db8a9 100644
>>>>>> >>>>>--- a/gcc/cfgexpand.c
>>>>>> >>>>>+++ b/gcc/cfgexpand.c
>>>>>> >>>>>@@ -1382,7 +1382,15 @@ expand_one_var (tree var, bool toplevel, bool
>>>>>> >>>>>really_expand)
>>>>>> >>>>>      else
>>>>>> >>>>>        {
>>>>>> >>>>>          if (really_expand)
>>>>>> >>>>>-        expand_one_stack_var (origvar);
>>>>>> >>>>>+        {
>>>>>> >>>>>+          if (!targetm.calls.allocate_stack_slots_for_args ())
>>>>>> >>>>>+            error ("cannot allocate stack for variable %q+D, naked
>>>>>> >>>>>function.",
>>>>>> >>>>>+                   var);
>>>>>> >>>>>+
>>>>>> >>>>>+          expand_one_stack_var (origvar);
>>>>>> >>>>>+        }
>>>>> >>>>
>>>>> >>>>So how do you know ORIGVAR is an argument here before issuing the
>>>>> >>>>error?  ie, shouldn't you verify that the underlying object is a
>>>>> >>>>PARM_DECL? If there's some way we already know we're dealing with a
>>>>> >>>>PARM_DECL, then just say so.
>>>> >>>
>>>> >>>In case of naked function stack should not be used not only for function
>>>> >>>args, but also for any local variables.
>>>> >>>So, i think we don't need to check if underlying object is a PARM_DECL.
>>> >>
>>> >>Then that would indicate that we're using the wrong test
>>> >>(allocate_stack_slot_for_args).  That hook is for whether or not arguments
>>> >>should have stack slots allocated.  Yet you're issuing an error for more
>>> >>than just PARM_DECLs.
>>> >>
>>> >>Shouldn't you instead be checking if the current function is a naked
>>> >>function or not by checking the attributes of the current function?
>>> >>
>>> >>Jeff
>> >
>> >What allocate_stack_slots_for_args  does, it only checks if current
>> >function is naked or not.
>> >May be it will be better to remove allocate_stack_slots_for_args and
>> >replace if with explicit checking of naked attribute?
>> >
>> >--
>> >Alexander
>
> pr64744.patch
>
>
> commit 3a72dac72beb713ab6a566728b77c4da6d297755
> Author: Alexander Basov<coohpt@gmail.com>
> Date:   Tue Mar 10 14:15:24 2015 +0300
>
>      	PR middle-end/64744
>      	PR middle-end/48470
>      	PR middle-end/43404
>
>      	* gcc/cfgexpand.c (expand_one_var): Add check if stack is going to
>      	be used in naked function.
>      	* gcc/expr.c (expand_expr_addr_expr_1): Remove exscess checking
>      	whether expression should not reside in MEM.
>          * gcc/function.c (use_register_for_decl): Do not use registers for
>      	non-register things (volatile, float, BLKMode) in naked functions.
>
>      	* gcc/testsuite/gcc.target/arm/pr43404.c : New testcase.
>      	* gcc/testsuite/gcc.target/arm/pr48470.c : New testcase.
>      	* gcc/testsuite/gcc.target/arm/pr64744-1.c : New testcase.
>          * gcc/testsuite/gcc.target/arm/pr64744-2.c : New testcase.
Sorry for the long delay.  I've fixed up minor nits in the ChangeLog and 
committed your fixes.

Thanks,
jeff