Message ID | 20230713164756.3558785-2-mikpelinux@gmail.com |
---|---|
State | New |
Headers | show |
Series | fix pdp11_expand_epilogue (PR target/107841) | expand |
> On Jul 13, 2023, at 12:47 PM, Mikael Pettersson <mikpelinux@gmail.com> wrote: > > If the stack frame only contains an alloca area, then > pdp11_expand_epilogue fails to deallocate it, resulting > in callee-saved registers and the return address being > restored from the wrong stack slots. Fixed by adding > || cfun->calls_alloca to the condition for deallocating > the frame. > > Tested with a cross to pdp11-unknown-aout. > > Ok for master? (Note: I don't have commit rights.) Yes, thank you! Question for the experts: how is this handled? Do I need to apply this change to my workspace and commit it, with Mikael as the change author? paul > > gcc/ > > PR target/107841 > * config/pdp11/pdp11.c (pdp11_expand_epilogue): Also > deallocate alloca-only frame. > > gcc/testsuite/ > > PR target/107841 > * gcc.target/pdp11/pr107841.c: New test. > --- > gcc/config/pdp11/pdp11.cc | 2 +- > gcc/testsuite/gcc.target/pdp11/pr107841.c | 12 ++++++++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.target/pdp11/pr107841.c > > diff --git a/gcc/config/pdp11/pdp11.cc b/gcc/config/pdp11/pdp11.cc > index f6dd841f184..311a1d225e0 100644 > --- a/gcc/config/pdp11/pdp11.cc > +++ b/gcc/config/pdp11/pdp11.cc > @@ -393,7 +393,7 @@ pdp11_expand_epilogue (void) > rtx x, reg, via_ac = NULL; > > /* Deallocate the local variables. */ > - if (fsize) > + if (fsize || cfun->calls_alloca) > { > if (frame_pointer_needed) > { > diff --git a/gcc/testsuite/gcc.target/pdp11/pr107841.c b/gcc/testsuite/gcc.target/pdp11/pr107841.c > new file mode 100644 > index 00000000000..a363c468b0b > --- /dev/null > +++ b/gcc/testsuite/gcc.target/pdp11/pr107841.c > @@ -0,0 +1,12 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +/* Verify that the stack frame is deallocated using the frame pointer. */ > + > +void qq (int a) > +{ > + char *s = __builtin_alloca (128); > + __builtin_sprintf (s, "qq %d", 3); > +} > + > +/* { dg-final { scan-assembler "mov\tr5,sp" } } */ > -- > 2.41.0 >
On 7/13/23 11:46, Paul Koning via Gcc-patches wrote: > > >> On Jul 13, 2023, at 12:47 PM, Mikael Pettersson <mikpelinux@gmail.com> wrote: >> >> If the stack frame only contains an alloca area, then >> pdp11_expand_epilogue fails to deallocate it, resulting >> in callee-saved registers and the return address being >> restored from the wrong stack slots. Fixed by adding >> || cfun->calls_alloca to the condition for deallocating >> the frame. >> >> Tested with a cross to pdp11-unknown-aout. >> >> Ok for master? (Note: I don't have commit rights.) > > Yes, thank you! > > Question for the experts: how is this handled? Do I need to apply this change to my workspace and commit it, with Mikael as the change author? That's what I usually do for someone without write access. commit it locally using the --author flag, then push. jeff
> On Jul 13, 2023, at 12:47 PM, Mikael Pettersson <mikpelinux@gmail.com> wrote: > > If the stack frame only contains an alloca area, then > pdp11_expand_epilogue fails to deallocate it, resulting > in callee-saved registers and the return address being > restored from the wrong stack slots. Fixed by adding > || cfun->calls_alloca to the condition for deallocating > the frame. > > Tested with a cross to pdp11-unknown-aout. > > Ok for master? (Note: I don't have commit rights.) > > gcc/ > > PR target/107841 > * config/pdp11/pdp11.c (pdp11_expand_epilogue): Also > deallocate alloca-only frame. > > gcc/testsuite/ > > PR target/107841 > * gcc.target/pdp11/pr107841.c: New test. > --- > gcc/config/pdp11/pdp11.cc | 2 +- > gcc/testsuite/gcc.target/pdp11/pr107841.c | 12 ++++++++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.target/pdp11/pr107841.c > > diff --git a/gcc/config/pdp11/pdp11.cc b/gcc/config/pdp11/pdp11.cc > index f6dd841f184..311a1d225e0 100644 > --- a/gcc/config/pdp11/pdp11.cc > +++ b/gcc/config/pdp11/pdp11.cc > @@ -393,7 +393,7 @@ pdp11_expand_epilogue (void) > rtx x, reg, via_ac = NULL; > > /* Deallocate the local variables. */ > - if (fsize) > + if (fsize || cfun->calls_alloca) > { > if (frame_pointer_needed) > { > diff --git a/gcc/testsuite/gcc.target/pdp11/pr107841.c b/gcc/testsuite/gcc.target/pdp11/pr107841.c > new file mode 100644 > index 00000000000..a363c468b0b > --- /dev/null > +++ b/gcc/testsuite/gcc.target/pdp11/pr107841.c > @@ -0,0 +1,12 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +/* Verify that the stack frame is deallocated using the frame pointer. */ > + > +void qq (int a) > +{ > + char *s = __builtin_alloca (128); > + __builtin_sprintf (s, "qq %d", 3); > +} > + > +/* { dg-final { scan-assembler "mov\tr5,sp" } } */ > -- > 2.41.0 > Thanks Mikael. I committed this fix. paul
On Thu, 13 Jul 2023, Jeff Law via Gcc-patches wrote: > > Question for the experts: how is this handled? Do I need to apply this > > change to my workspace and commit it, with Mikael as the change author? > That's what I usually do for someone without write access. commit it locally > using the --author flag, then push. FWIW `git am' taking the e-mail containing a change on stdin is I believe the most straightforward way, as long as the submission has been properly formatted. The command automatically sets the author correctly, picking the `From:' entry from the e-mail headers or the first line of the e-mail body as appropriate, sets the author date from the `Date:' header, picks up the contents of the `Subject:' header as the change heading and the body of the e-mail sans any `From:' entry as the change description, and strips any discussion between the non-commit delimiter (`--') and the patch itself. The command was designed for maintainers to import changes submitted by e-mail so it does what expected. Any issues with an imported change can then be addressed with `git commit --amend', etc. HTH, Maciej
diff --git a/gcc/config/pdp11/pdp11.cc b/gcc/config/pdp11/pdp11.cc index f6dd841f184..311a1d225e0 100644 --- a/gcc/config/pdp11/pdp11.cc +++ b/gcc/config/pdp11/pdp11.cc @@ -393,7 +393,7 @@ pdp11_expand_epilogue (void) rtx x, reg, via_ac = NULL; /* Deallocate the local variables. */ - if (fsize) + if (fsize || cfun->calls_alloca) { if (frame_pointer_needed) { diff --git a/gcc/testsuite/gcc.target/pdp11/pr107841.c b/gcc/testsuite/gcc.target/pdp11/pr107841.c new file mode 100644 index 00000000000..a363c468b0b --- /dev/null +++ b/gcc/testsuite/gcc.target/pdp11/pr107841.c @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +/* Verify that the stack frame is deallocated using the frame pointer. */ + +void qq (int a) +{ + char *s = __builtin_alloca (128); + __builtin_sprintf (s, "qq %d", 3); +} + +/* { dg-final { scan-assembler "mov\tr5,sp" } } */