[PATCH][ARM] New testcase to check parameter passing bug

[PATCH][ARM] New testcase to check parameter passing bug

[Honggyu Kim]

Hi,

I have wrote a testcase that reproduces argument overwriting bug during arm
code generation.

I wrote this testcase with the help of Mikael Pettersson.
If some format is not proper to run in gcc testsuite framework, please
correct me.

Please refer to the following bugzilla link for details:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65358

Honggyu
---
 gcc/testsuite/ChangeLog                |    5 +++++
 gcc/testsuite/gcc.target/arm/pr65358.c |   34 ++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/arm/pr65358.c

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 5302dbd..9acd12a 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2015-03-13  Honggyu Kim  <hong.gyu.kim@lge.com>
+
+	PR target/65235
+	* gcc.target/arm/pr65358.c: New test for sibcall argument passing bug.
+
 2015-03-12  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
 
 	PR rtl-optimization/65235
diff --git a/gcc/testsuite/gcc.target/arm/pr65358.c b/gcc/testsuite/gcc.target/arm/pr65358.c
new file mode 100644
index 0000000..d663dcf
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr65358.c
@@ -0,0 +1,34 @@
+/* PR target/65358 */
+/* { dg-do compile { target arm*-*-* } } */
+/* { dg-options "-O2" } */
+
+struct pack
+{
+  int fine;
+  int victim;
+  int killer;
+};
+
+int __attribute__ ((__noinline__, __noclone__))
+bar (int a, int b, struct pack p)
+{
+  if (a != 20 || b != 30)
+    __builtin_abort ();
+  if (p.fine != 40 || p.victim != 50 || p.killer != 60)
+    __builtin_abort ();
+  return 0;
+}
+
+int __attribute__ ((__noinline__, __noclone__))
+foo (int arg1, int arg2, int arg3, struct pack p)
+{
+  return bar (arg2, arg3, p);
+}
+
+int main (void)
+{
+  struct pack p = { 40, 50, 60 };
+
+  (void) foo (10, 20, 30, p);
+  return 0;
+}
-- 
1.7.9.5

RE: [PATCH][ARM] New testcase to check parameter passing bug

[Kyrill Tkachov]

> Hi,

Hi Honggyu,
Thanks for helping out. I've got a couple of pointers for the testcase
inline.

> 
> I have wrote a testcase that reproduces argument overwriting bug during
> arm code generation.
> 
> I wrote this testcase with the help of Mikael Pettersson.
> If some format is not proper to run in gcc testsuite framework, please
correct
> me.
> 
> Please refer to the following bugzilla link for details:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65358
> 
> Honggyu
> ---
>  gcc/testsuite/ChangeLog                |    5 +++++
>  gcc/testsuite/gcc.target/arm/pr65358.c |   34
> ++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/arm/pr65358.c
> 
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index
> 5302dbd..9acd12a 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2015-03-13  Honggyu Kim  <hong.gyu.kim@lge.com>
> +
> +	PR target/65235
> +	* gcc.target/arm/pr65358.c: New test for sibcall argument passing
> bug.

Just 'New test.' for the entry should be enough, but it's not a big deal.

> +
>  2015-03-12  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>  	PR rtl-optimization/65235
> diff --git a/gcc/testsuite/gcc.target/arm/pr65358.c
> b/gcc/testsuite/gcc.target/arm/pr65358.c
> new file mode 100644
> index 0000000..d663dcf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/pr65358.c
> @@ -0,0 +1,34 @@
> +/* PR target/65358 */
> +/* { dg-do compile { target arm*-*-* } } */

No need for the target selector. The fact that it's in gcc.target/arm
already
means it will only be run for the arm targets.
Also, the bug is exposed at runtime, so this should be a dg-do run test.

Cheers,
Kyrill

> +/* { dg-options "-O2" } */
> +
> +struct pack
> +{
> +  int fine;
> +  int victim;
> +  int killer;
> +};
> +
> +int __attribute__ ((__noinline__, __noclone__)) bar (int a, int b,
> +struct pack p) {
> +  if (a != 20 || b != 30)
> +    __builtin_abort ();
> +  if (p.fine != 40 || p.victim != 50 || p.killer != 60)
> +    __builtin_abort ();
> +  return 0;
> +}
> +
> +int __attribute__ ((__noinline__, __noclone__)) foo (int arg1, int
> +arg2, int arg3, struct pack p) {
> +  return bar (arg2, arg3, p);
> +}
> +
> +int main (void)
> +{
> +  struct pack p = { 40, 50, 60 };
> +
> +  (void) foo (10, 20, 30, p);
> +  return 0;
> +}
> --
> 1.7.9.5
> 

Re: [PATCH][ARM] New testcase to check parameter passing bug

[Christophe Lyon]

On 03/13/15 13:04, Kyrill Tkachov wrote:
>
>> Hi,
>
> Hi Honggyu,
> Thanks for helping out. I've got a couple of pointers for the testcase
> inline.
>
>>
>> I have wrote a testcase that reproduces argument overwriting bug during
>> arm code generation.
>>
>> I wrote this testcase with the help of Mikael Pettersson.
>> If some format is not proper to run in gcc testsuite framework, please
> correct
>> me.
>>
>> Please refer to the following bugzilla link for details:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65358
>>
>> Honggyu
>> ---
>>   gcc/testsuite/ChangeLog                |    5 +++++
>>   gcc/testsuite/gcc.target/arm/pr65358.c |   34
>> ++++++++++++++++++++++++++++++++
>>   2 files changed, 39 insertions(+)
>>   create mode 100644 gcc/testsuite/gcc.target/arm/pr65358.c
>>
>> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index
>> 5302dbd..9acd12a 100644
>> --- a/gcc/testsuite/ChangeLog
>> +++ b/gcc/testsuite/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2015-03-13  Honggyu Kim  <hong.gyu.kim@lge.com>
>> +
>> +	PR target/65235
Looks like the PR number is wrong.

>> +	* gcc.target/arm/pr65358.c: New test for sibcall argument passing
>> bug.
>
> Just 'New test.' for the entry should be enough, but it's not a big deal.
>
>> +
>>   2015-03-12  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>   	PR rtl-optimization/65235
>> diff --git a/gcc/testsuite/gcc.target/arm/pr65358.c
>> b/gcc/testsuite/gcc.target/arm/pr65358.c
>> new file mode 100644
>> index 0000000..d663dcf
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/pr65358.c
>> @@ -0,0 +1,34 @@
>> +/* PR target/65358 */
>> +/* { dg-do compile { target arm*-*-* } } */
>
> No need for the target selector. The fact that it's in gcc.target/arm
> already
> means it will only be run for the arm targets.
> Also, the bug is exposed at runtime, so this should be a dg-do run test.
>
> Cheers,
> Kyrill
>
>> +/* { dg-options "-O2" } */
>> +
>> +struct pack
>> +{
>> +  int fine;
>> +  int victim;
>> +  int killer;
>> +};
>> +
>> +int __attribute__ ((__noinline__, __noclone__)) bar (int a, int b,
>> +struct pack p) {
>> +  if (a != 20 || b != 30)
>> +    __builtin_abort ();
>> +  if (p.fine != 40 || p.victim != 50 || p.killer != 60)
>> +    __builtin_abort ();
>> +  return 0;
>> +}
>> +
>> +int __attribute__ ((__noinline__, __noclone__)) foo (int arg1, int
>> +arg2, int arg3, struct pack p) {
>> +  return bar (arg2, arg3, p);
>> +}
>> +
>> +int main (void)
>> +{
>> +  struct pack p = { 40, 50, 60 };
>> +
>> +  (void) foo (10, 20, 30, p);
>> +  return 0;
>> +}
>> --
>> 1.7.9.5
>>
>
>
>
>
> .
>

Re: [PATCH][ARM] New testcase to check parameter passing bug

[Segher Boessenkool]

On Fri, Mar 13, 2015 at 12:04:28PM -0000, Kyrill Tkachov wrote:
> > diff --git a/gcc/testsuite/gcc.target/arm/pr65358.c
> > b/gcc/testsuite/gcc.target/arm/pr65358.c
> > new file mode 100644
> > index 0000000..d663dcf
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/arm/pr65358.c
> > @@ -0,0 +1,34 @@
> > +/* PR target/65358 */
> > +/* { dg-do compile { target arm*-*-* } } */
> 
> No need for the target selector. The fact that it's in gcc.target/arm
> already
> means it will only be run for the arm targets.
> Also, the bug is exposed at runtime, so this should be a dg-do run test.

Why make this testcase arm-specific at all?  I don't see anything that
wouldn't work on other targets?


Segher

RE: [PATCH][ARM] New testcase to check parameter passing bug

[Honggyu Kim]

Hi,

I have modified and moved the testcase following your comments.
(from gcc.target/arm to gcc.dg)
Please let me know if there's still something to fix more.
I appreciate all your comments.

Honggyu
---
 gcc/testsuite/ChangeLog        |    4 ++++
 gcc/testsuite/gcc.dg/pr65358.c |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr65358.c

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 5302dbd..5dcf2cf 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2015-03-16  Honggyu Kim  <hong.gyu.kim@lge.com>
+
+	* gcc.dg/pr65358.c: New test.
+
 2015-03-15  John David Anglin  <danglin@gcc.gnu.org>
 
 	* gcc.dg/torture/pr65270-1.c: Add -fno-common to dg-options on
diff --git a/gcc/testsuite/gcc.dg/pr65358.c b/gcc/testsuite/gcc.dg/pr65358.c
new file mode 100644
index 0000000..3790764
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr65358.c
@@ -0,0 +1,33 @@
+/* { dg-do run */
+/* { dg-options "-O2" } */
+
+struct pack
+{
+  int fine;
+  int victim;
+  int killer;
+};
+
+int __attribute__ ((__noinline__, __noclone__))
+bar (int a, int b, struct pack p)
+{
+  if (a != 20 || b != 30)
+    __builtin_abort ();
+  if (p.fine != 40 || p.victim != 50 || p.killer != 60)
+    __builtin_abort ();
+  return 0;
+}
+
+int __attribute__ ((__noinline__, __noclone__))
+foo (int arg1, int arg2, int arg3, struct pack p)
+{
+  return bar (arg2, arg3, p);
+}
+
+int main (void)
+{
+  struct pack p = { 40, 50, 60 };
+
+  (void) foo (10, 20, 30, p);
+  return 0;
+}
-- 
1.7.9.5

Re: [PATCH][ARM] New testcase to check parameter passing bug

[Kyrill Tkachov]

Hi Honggyu,

On 16/03/15 00:53, Honggyu Kim wrote:
> new file mode 100644
> index 0000000..3790764
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr65358.c
> @@ -0,0 +1,33 @@
> +/* { dg-do run */

Forgot to close the brace here after 'run'
Dejagnu happily ignores that and transforms this into a compile-only 
testcase
without that.

Otherwise looks good to me.

Kyrill

> +/* { dg-options "-O2" } */
> +
> +struct pack
> +{
> +  int fine;
> +  int victim;
> +  int killer;
> +};
> +

[PATCH v2] New testcase to check parameter passing bug

[Honggyu Kim]

Hi,

I have modified the test-case to check parameter passing bug based on the
comments from Kyrill Tkachov, Christophe Lyon, and Segher Boessenkool
as follows:
 1. move from "gcc.target/arm" to "gcc.dg"
 2. change "dg-do compile" to "dg-do run"

Please let me know if there's still something to fix more.
Thanks for your comment.

Honggyu
---
 gcc/testsuite/ChangeLog        |    4 ++++
 gcc/testsuite/gcc.dg/pr65358.c |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr65358.c

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 77d24a1..218f908 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2015-03-19  Honggyu Kim  <hong.gyu.kim@lge.com>
+
+	* gcc.dg/pr65358.c: New test.
+
 2015-03-18  Paolo Carlini  <paolo.carlini@oracle.com>
 
 	PR c++/59816
diff --git a/gcc/testsuite/gcc.dg/pr65358.c b/gcc/testsuite/gcc.dg/pr65358.c
new file mode 100644
index 0000000..ba89fd4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr65358.c
@@ -0,0 +1,33 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+struct pack
+{
+  int fine;
+  int victim;
+  int killer;
+};
+
+int __attribute__ ((__noinline__, __noclone__))
+bar (int a, int b, struct pack p)
+{
+  if (a != 20 || b != 30)
+    __builtin_abort ();
+  if (p.fine != 40 || p.victim != 50 || p.killer != 60)
+    __builtin_abort ();
+  return 0;
+}
+
+int __attribute__ ((__noinline__, __noclone__))
+foo (int arg1, int arg2, int arg3, struct pack p)
+{
+  return bar (arg2, arg3, p);
+}
+
+int main (void)
+{
+  struct pack p = { 40, 50, 60 };
+
+  (void) foo (10, 20, 30, p);
+  return 0;
+}
-- 
1.7.9.5

Re: [PATCH v2] New testcase to check parameter passing bug

[Honggyu Kim]

On Thu, Mar 19, 2015 at 10:40:44AM +0900, Honggyu Kim wrote:
> ---
>  gcc/testsuite/ChangeLog        |    4 ++++
>  gcc/testsuite/gcc.dg/pr65358.c |   33 +++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.dg/pr65358.c
> 
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index 77d24a1..218f908 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> 
I'm kind of new to gcc community and I'm used to send a patch throught
git send-email. If you have other preferable way, please let me know.
I will try to follow the format and style more like other gcc patches.
Thanks you all.

Honggyu

Re: [PATCH v2] New testcase to check parameter passing bug

[Jeff Law]

On 03/18/15 19:40, Honggyu Kim wrote:
> Hi,
>
> I have modified the test-case to check parameter passing bug based on the
> comments from Kyrill Tkachov, Christophe Lyon, and Segher Boessenkool
> as follows:
>   1. move from "gcc.target/arm" to "gcc.dg"
>   2. change "dg-do compile" to "dg-do run"
>
> Please let me know if there's still something to fix more.
> Thanks for your comment.
>
> Honggyu
> ---
>   gcc/testsuite/ChangeLog        |    4 ++++
>   gcc/testsuite/gcc.dg/pr65358.c |   33 +++++++++++++++++++++++++++++++++
>   2 files changed, 37 insertions(+)
>   create mode 100644 gcc/testsuite/gcc.dg/pr65358.c
>
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index 77d24a1..218f908 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,7 @@
> +2015-03-19  Honggyu Kim  <hong.gyu.kim@lge.com>
> +
> +	* gcc.dg/pr65358.c: New test.
This should be included as part of Kyrill's patch.  If the test goes in 
without Kryill's fix, then it'll just create testsuite noise.

Jeff

RE: [PATCH v2] New testcase to check parameter passing bug

[Kyrill Tkachov]

> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: 25 March 2015 12:27
> To: Honggyu Kim; gcc-patches@gcc.gnu.org
> Cc: Kyrylo Tkachov; segher@kernel.crashing.org; christophe.lyon@st.com
> Subject: Re: [PATCH v2] New testcase to check parameter passing bug
> 
> On 03/18/15 19:40, Honggyu Kim wrote:
> > Hi,
> >
> > I have modified the test-case to check parameter passing bug based on
> > the comments from Kyrill Tkachov, Christophe Lyon, and Segher
> > Boessenkool as follows:
> >   1. move from "gcc.target/arm" to "gcc.dg"
> >   2. change "dg-do compile" to "dg-do run"
> >
> > Please let me know if there's still something to fix more.
> > Thanks for your comment.
> >
> > Honggyu
> > ---
> >   gcc/testsuite/ChangeLog        |    4 ++++
> >   gcc/testsuite/gcc.dg/pr65358.c |   33
> +++++++++++++++++++++++++++++++++
> >   2 files changed, 37 insertions(+)
> >   create mode 100644 gcc/testsuite/gcc.dg/pr65358.c
> >
> > diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index
> > 77d24a1..218f908 100644
> > --- a/gcc/testsuite/ChangeLog
> > +++ b/gcc/testsuite/ChangeLog
> > @@ -1,3 +1,7 @@
> > +2015-03-19  Honggyu Kim  <hong.gyu.kim@lge.com>
> > +
> > +	* gcc.dg/pr65358.c: New test.
> This should be included as part of Kyrill's patch.  If the test goes in
without
> Kryill's fix, then it'll just create testsuite noise.

I'll make sure to commit this together with my fix (at
https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01014.html)
if it gets approved. I agree that there's no point taking the test in by
itself .

Thanks,
Kyrill

> 
> Jeff
> 

Re: [PATCH v2] New testcase to check parameter passing bug

[Honggyu Kim]

On Wed, Mar 25, 2015 at 02:05:37PM -0000, Kyrill Tkachov wrote:
> 
> 
> > -----Original Message-----
> > From: Jeff Law [mailto:law@redhat.com]
> > Sent: 25 March 2015 12:27
> > To: Honggyu Kim; gcc-patches@gcc.gnu.org
> > Cc: Kyrylo Tkachov; segher@kernel.crashing.org; christophe.lyon@st.com
> > Subject: Re: [PATCH v2] New testcase to check parameter passing bug
> > 
> > On 03/18/15 19:40, Honggyu Kim wrote:
> > > Hi,
> > >
> > > I have modified the test-case to check parameter passing bug based on
> > > the comments from Kyrill Tkachov, Christophe Lyon, and Segher
> > > Boessenkool as follows:
> > >   1. move from "gcc.target/arm" to "gcc.dg"
> > >   2. change "dg-do compile" to "dg-do run"
> > >
> > > Please let me know if there's still something to fix more.
> > > Thanks for your comment.
> > >
> > > Honggyu
> > > ---
> > >   gcc/testsuite/ChangeLog        |    4 ++++
> > >   gcc/testsuite/gcc.dg/pr65358.c |   33
> > +++++++++++++++++++++++++++++++++
> > >   2 files changed, 37 insertions(+)
> > >   create mode 100644 gcc/testsuite/gcc.dg/pr65358.c
> > >
> > > diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index
> > > 77d24a1..218f908 100644
> > > --- a/gcc/testsuite/ChangeLog
> > > +++ b/gcc/testsuite/ChangeLog
> > > @@ -1,3 +1,7 @@
> > > +2015-03-19  Honggyu Kim  <hong.gyu.kim@lge.com>
> > > +
> > > +	* gcc.dg/pr65358.c: New test.
> > This should be included as part of Kyrill's patch.  If the test goes in
> without
> > Kryill's fix, then it'll just create testsuite noise.
> 
> I'll make sure to commit this together with my fix (at
> https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01014.html)
> if it gets approved. I agree that there's no point taking the test in by
> itself .
> 
> Thanks,
> Kyrill
> 
> > 
> > Jeff
> > 

I have tested Kyrill's patch and it works fine.
Thanks for all your comments.
Especially, Mikael Pettersson's help for this testcase.

Honggyu