diff mbox

Fix PR43404, PR48470, PR64744 ICE on naked functions

Message ID CAHpy5QqPZLnO5oJJ8v9yoVQ5i4DqN45NOMc7EGrSV00c+eYSkA@mail.gmail.com
State New
Headers show

Commit Message

coopht@gmail.com June 29, 2015, 1:32 p.m. UTC
I've updated patch with attributes lookup.
is it OK?

Comments

coopht@gmail.com July 12, 2015, 7:02 p.m. UTC | #1
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
Jeff Law Aug. 3, 2015, 7:35 p.m. UTC | #2
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
diff mbox

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.

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" } */
+}