Message ID | CAHpy5QqPZLnO5oJJ8v9yoVQ5i4DqN45NOMc7EGrSV00c+eYSkA@mail.gmail.com |
---|---|
State | New |
Headers | show |
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
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
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" } */ +}