| Message ID | 20200210162241.988175-1-gofmanp@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix stack pointer handling in ms_hook_prologue functions for i386 target. | expand |
Hello, ping for patch https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00554.html. Thanks, Paul. On 2/10/20 19:22, Paul Gofman wrote: > ChangeLog: > PR target/91489 > * config/i386/i386.md (simple_return): Also check > for ms_hook_prologue function attribute. > * config/i386/i386.c (ix86_can_use_return_insn_p): > Also check for ms_hook_prologue function attribute. > > testsuite/ChangeLog: > PR target/91489 > * gcc.target/i386/ms_hook_prologue.c: Expand testcase > to reproduce PR target/91489 issue. > > Signed-off-by: Paul Gofman <gofmanp@gmail.com> > --- > gcc/config/i386/i386-protos.h | 1 + > gcc/config/i386/i386.c | 3 +++ > gcc/config/i386/i386.md | 5 ++++- > gcc/testsuite/gcc.target/i386/ms_hook_prologue.c | 13 ++++++++++++- > 4 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/gcc/config/i386/i386-protos.h b/gcc/config/i386/i386-protos.h > index 266381ca5a6..966ce426a18 100644 > --- a/gcc/config/i386/i386-protos.h > +++ b/gcc/config/i386/i386-protos.h > @@ -26,6 +26,7 @@ extern bool ix86_handle_option (struct gcc_options *opts, > /* Functions in i386.c */ > extern bool ix86_target_stack_probe (void); > extern bool ix86_can_use_return_insn_p (void); > +extern bool ix86_function_ms_hook_prologue (const_tree fn); > extern void ix86_setup_frame_addresses (void); > extern bool ix86_rip_relative_addr_p (struct ix86_address *parts); > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > index 44bc0e0176a..68e2a7519f4 100644 > --- a/gcc/config/i386/i386.c > +++ b/gcc/config/i386/i386.c > @@ -4954,6 +4954,9 @@ symbolic_reference_mentioned_p (rtx op) > bool > ix86_can_use_return_insn_p (void) > { > + if (ix86_function_ms_hook_prologue (current_function_decl)) > + return false; > + > if (ix86_function_naked (current_function_decl)) > return false; > > diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md > index f14683cd14f..a7302b886c6 100644 > --- a/gcc/config/i386/i386.md > +++ b/gcc/config/i386/i386.md > @@ -13445,10 +13445,13 @@ > ;; static chain pointer - the first instruction has to be pushl %esi > ;; and it can't be moved around, as we use alternate entry points > ;; in that case. > +;; Also disallow for ms_hook_prologue functions which have frame > +;; pointer set up in function label which is correctly handled in > +;; ix86_expand_{prologue|epligoue}() only. > > (define_expand "simple_return" > [(simple_return)] > - "!TARGET_SEH && !ix86_static_chain_on_stack" > + "!TARGET_SEH && !ix86_static_chain_on_stack && !ix86_function_ms_hook_prologue (cfun->decl)" > { > if (crtl->args.pops_args) > { > diff --git a/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c b/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c > index e11bcc049cb..12e54c0e4ad 100644 > --- a/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c > +++ b/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c > @@ -4,6 +4,8 @@ > /* { dg-require-effective-target ms_hook_prologue } */ > /* { dg-options "-O2 -fomit-frame-pointer" } */ > > +#include <stdio.h> > + > int __attribute__ ((__ms_hook_prologue__)) foo () > { > unsigned char *ptr = (unsigned char *) foo; > @@ -32,7 +34,16 @@ int __attribute__ ((__ms_hook_prologue__)) foo () > return 0; > } > > +unsigned int __attribute__ ((noinline, __ms_hook_prologue__)) test_func() > +{ > + static int value; > + > + if (value++) puts(""); > + > + return 0; > +} > + > int main () > { > - return foo(); > + return foo() || test_func(); > }
Hello, ping for patch https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00554.html Regards, Paul. On 2/21/20 12:10, Paul Gofman wrote: > Hello, > > ping for patch https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00554.html. > > Thanks, > Paul. > > On 2/10/20 19:22, Paul Gofman wrote: >> ChangeLog: >> PR target/91489 >> * config/i386/i386.md (simple_return): Also check >> for ms_hook_prologue function attribute. >> * config/i386/i386.c (ix86_can_use_return_insn_p): >> Also check for ms_hook_prologue function attribute. >> >> testsuite/ChangeLog: >> PR target/91489 >> * gcc.target/i386/ms_hook_prologue.c: Expand testcase >> to reproduce PR target/91489 issue. >> >> Signed-off-by: Paul Gofman <gofmanp@gmail.com> >> --- >> gcc/config/i386/i386-protos.h | 1 + >> gcc/config/i386/i386.c | 3 +++ >> gcc/config/i386/i386.md | 5 ++++- >> gcc/testsuite/gcc.target/i386/ms_hook_prologue.c | 13 ++++++++++++- >> 4 files changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/gcc/config/i386/i386-protos.h b/gcc/config/i386/i386-protos.h >> index 266381ca5a6..966ce426a18 100644 >> --- a/gcc/config/i386/i386-protos.h >> +++ b/gcc/config/i386/i386-protos.h >> @@ -26,6 +26,7 @@ extern bool ix86_handle_option (struct gcc_options *opts, >> /* Functions in i386.c */ >> extern bool ix86_target_stack_probe (void); >> extern bool ix86_can_use_return_insn_p (void); >> +extern bool ix86_function_ms_hook_prologue (const_tree fn); >> extern void ix86_setup_frame_addresses (void); >> extern bool ix86_rip_relative_addr_p (struct ix86_address *parts); >> >> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c >> index 44bc0e0176a..68e2a7519f4 100644 >> --- a/gcc/config/i386/i386.c >> +++ b/gcc/config/i386/i386.c >> @@ -4954,6 +4954,9 @@ symbolic_reference_mentioned_p (rtx op) >> bool >> ix86_can_use_return_insn_p (void) >> { >> + if (ix86_function_ms_hook_prologue (current_function_decl)) >> + return false; >> + >> if (ix86_function_naked (current_function_decl)) >> return false; >> >> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md >> index f14683cd14f..a7302b886c6 100644 >> --- a/gcc/config/i386/i386.md >> +++ b/gcc/config/i386/i386.md >> @@ -13445,10 +13445,13 @@ >> ;; static chain pointer - the first instruction has to be pushl %esi >> ;; and it can't be moved around, as we use alternate entry points >> ;; in that case. >> +;; Also disallow for ms_hook_prologue functions which have frame >> +;; pointer set up in function label which is correctly handled in >> +;; ix86_expand_{prologue|epligoue}() only. >> >> (define_expand "simple_return" >> [(simple_return)] >> - "!TARGET_SEH && !ix86_static_chain_on_stack" >> + "!TARGET_SEH && !ix86_static_chain_on_stack && !ix86_function_ms_hook_prologue (cfun->decl)" >> { >> if (crtl->args.pops_args) >> { >> diff --git a/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c b/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c >> index e11bcc049cb..12e54c0e4ad 100644 >> --- a/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c >> +++ b/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c >> @@ -4,6 +4,8 @@ >> /* { dg-require-effective-target ms_hook_prologue } */ >> /* { dg-options "-O2 -fomit-frame-pointer" } */ >> >> +#include <stdio.h> >> + >> int __attribute__ ((__ms_hook_prologue__)) foo () >> { >> unsigned char *ptr = (unsigned char *) foo; >> @@ -32,7 +34,16 @@ int __attribute__ ((__ms_hook_prologue__)) foo () >> return 0; >> } >> >> +unsigned int __attribute__ ((noinline, __ms_hook_prologue__)) test_func() >> +{ >> + static int value; >> + >> + if (value++) puts(""); >> + >> + return 0; >> +} >> + >> int main () >> { >> - return foo(); >> + return foo() || test_func(); >> } >
On Mon, 2020-02-10 at 19:22 +0300, Paul Gofman wrote: > ChangeLog: > PR target/91489 > * config/i386/i386.md (simple_return): Also check > for ms_hook_prologue function attribute. > * config/i386/i386.c (ix86_can_use_return_insn_p): > Also check for ms_hook_prologue function attribute. > > testsuite/ChangeLog: > PR target/91489 > * gcc.target/i386/ms_hook_prologue.c: Expand testcase > to reproduce PR target/91489 issue. I'm going to queue this for gcc-11. While I realize this is a codegen issue, I'm not familiar enough with the stdcall conventions to give this the review it would need at this stage in gcc-10 development. jeff >
Hello Jeff, pinging the patch. Regards, Paul. On 3/26/20 01:49, Jeff Law wrote: > On Mon, 2020-02-10 at 19:22 +0300, Paul Gofman wrote: >> ChangeLog: >> PR target/91489 >> * config/i386/i386.md (simple_return): Also check >> for ms_hook_prologue function attribute. >> * config/i386/i386.c (ix86_can_use_return_insn_p): >> Also check for ms_hook_prologue function attribute. >> >> testsuite/ChangeLog: >> PR target/91489 >> * gcc.target/i386/ms_hook_prologue.c: Expand testcase >> to reproduce PR target/91489 issue. > I'm going to queue this for gcc-11. While I realize this is a codegen issue, I'm > not familiar enough with the stdcall conventions to give this the review it would > need at this stage in gcc-10 development. > > jeff
On 2/10/20 9:22 AM, Paul Gofman wrote: > ChangeLog: > PR target/91489 > * config/i386/i386.md (simple_return): Also check > for ms_hook_prologue function attribute. > * config/i386/i386.c (ix86_can_use_return_insn_p): > Also check for ms_hook_prologue function attribute. > > testsuite/ChangeLog: > PR target/91489 > * gcc.target/i386/ms_hook_prologue.c: Expand testcase > to reproduce PR target/91489 issue. Thanks. I'm sorry it took so long, but this has been committed to the trunk. jeff
diff --git a/gcc/config/i386/i386-protos.h b/gcc/config/i386/i386-protos.h index 266381ca5a6..966ce426a18 100644 --- a/gcc/config/i386/i386-protos.h +++ b/gcc/config/i386/i386-protos.h @@ -26,6 +26,7 @@ extern bool ix86_handle_option (struct gcc_options *opts, /* Functions in i386.c */ extern bool ix86_target_stack_probe (void); extern bool ix86_can_use_return_insn_p (void); +extern bool ix86_function_ms_hook_prologue (const_tree fn); extern void ix86_setup_frame_addresses (void); extern bool ix86_rip_relative_addr_p (struct ix86_address *parts); diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 44bc0e0176a..68e2a7519f4 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -4954,6 +4954,9 @@ symbolic_reference_mentioned_p (rtx op) bool ix86_can_use_return_insn_p (void) { + if (ix86_function_ms_hook_prologue (current_function_decl)) + return false; + if (ix86_function_naked (current_function_decl)) return false; diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index f14683cd14f..a7302b886c6 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -13445,10 +13445,13 @@ ;; static chain pointer - the first instruction has to be pushl %esi ;; and it can't be moved around, as we use alternate entry points ;; in that case. +;; Also disallow for ms_hook_prologue functions which have frame +;; pointer set up in function label which is correctly handled in +;; ix86_expand_{prologue|epligoue}() only. (define_expand "simple_return" [(simple_return)] - "!TARGET_SEH && !ix86_static_chain_on_stack" + "!TARGET_SEH && !ix86_static_chain_on_stack && !ix86_function_ms_hook_prologue (cfun->decl)" { if (crtl->args.pops_args) { diff --git a/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c b/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c index e11bcc049cb..12e54c0e4ad 100644 --- a/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c +++ b/gcc/testsuite/gcc.target/i386/ms_hook_prologue.c @@ -4,6 +4,8 @@ /* { dg-require-effective-target ms_hook_prologue } */ /* { dg-options "-O2 -fomit-frame-pointer" } */ +#include <stdio.h> + int __attribute__ ((__ms_hook_prologue__)) foo () { unsigned char *ptr = (unsigned char *) foo; @@ -32,7 +34,16 @@ int __attribute__ ((__ms_hook_prologue__)) foo () return 0; } +unsigned int __attribute__ ((noinline, __ms_hook_prologue__)) test_func() +{ + static int value; + + if (value++) puts(""); + + return 0; +} + int main () { - return foo(); + return foo() || test_func(); }
ChangeLog: PR target/91489 * config/i386/i386.md (simple_return): Also check for ms_hook_prologue function attribute. * config/i386/i386.c (ix86_can_use_return_insn_p): Also check for ms_hook_prologue function attribute. testsuite/ChangeLog: PR target/91489 * gcc.target/i386/ms_hook_prologue.c: Expand testcase to reproduce PR target/91489 issue. Signed-off-by: Paul Gofman <gofmanp@gmail.com> --- gcc/config/i386/i386-protos.h | 1 + gcc/config/i386/i386.c | 3 +++ gcc/config/i386/i386.md | 5 ++++- gcc/testsuite/gcc.target/i386/ms_hook_prologue.c | 13 ++++++++++++- 4 files changed, 20 insertions(+), 2 deletions(-)