[PATCH] fix pdp11_expand_epilogue (PR target/107841)

[PATCH] fix pdp11_expand_epilogue (PR target/107841)

[Mikael Pettersson]

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

Re: [PATCH] fix pdp11_expand_epilogue (PR target/107841)

[Paul Koning]

> 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
> 

Re: [PATCH] fix pdp11_expand_epilogue (PR target/107841)

[Jeff Law]

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

Re: [PATCH] fix pdp11_expand_epilogue (PR target/107841)

[Paul Koning]

> 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

Re: [PATCH] fix pdp11_expand_epilogue (PR target/107841)

[Maciej W. Rozycki]

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