aarch64: falkor-tag-collision-avoidance.c fix valid_src_p for use of uninitialized value

Message ID gkrpncawfni.fsf_-_@arm.com
State New
Headers show
Series
  • aarch64: falkor-tag-collision-avoidance.c fix valid_src_p for use of uninitialized value
Related show

Commit Message

Andrea Corallo April 14, 2020, 10 a.m.
Hi all,

Testing the backport of pr94530 fix (af19e4d0e23e) to GCC 9 I realized
this is not entirely correct.

aarch64_classify_address sets the base field of struct
aarch64_address_info for all aarch64_address_type with the exception
of ADDRESS_SYMBOLIC.  In this case we would access an uninitialized
value.  The attached patch fixes the issue.

Bootstraped on aarch64 and regressioned.  Okay for trunk?

I've also tested af19e4d0e23e + the current patch rebased on on top of
the gcc-9 branch.  Okay to backport?

Andrea

PS I'm wondering if would be good also to always set the addr field in
aarch64_classify_address for safeness.

gcc/ChangeLog

2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>

	* config/aarch64/falkor-tag-collision-avoidance.c
	(valid_src_p): Check for aarch64_address_info type before
	accessing base field.

Comments

Kyrylo Tkachov April 14, 2020, 4:55 p.m. | #1
Hi Andrea,

> -----Original Message-----

> From: Andrea Corallo <Andrea.Corallo@arm.com>

> Sent: 14 April 2020 11:01

> To: gcc-patches@gcc.gnu.org

> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>; nd <nd@arm.com>;

> Christophe Lyon <christophe.lyon@linaro.org>

> Subject: [PATCH]aarch64: falkor-tag-collision-avoidance.c fix valid_src_p for

> use of uninitialized value

> 

> Hi all,

> 

> Testing the backport of pr94530 fix (af19e4d0e23e) to GCC 9 I realized

> this is not entirely correct.

> 

> aarch64_classify_address sets the base field of struct

> aarch64_address_info for all aarch64_address_type with the exception

> of ADDRESS_SYMBOLIC.  In this case we would access an uninitialized

> value.  The attached patch fixes the issue.

> 

> Bootstraped on aarch64 and regressioned.  Okay for trunk?

> 

> I've also tested af19e4d0e23e + the current patch rebased on on top of

> the gcc-9 branch.  Okay to backport?

> 

> Andrea

> 

> PS I'm wondering if would be good also to always set the addr field in

> aarch64_classify_address for safeness.

> 

> gcc/ChangeLog

> 

> 2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>

> 

> 	* config/aarch64/falkor-tag-collision-avoidance.c

> 	(valid_src_p): Check for aarch64_address_info type before

> 	accessing base field.


From 4af5bb32cd88bb4e65591207839fb0e276b2eb23 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>

Date: Thu, 9 Apr 2020 15:34:50 +0100
Subject: [PATCH] pr94530 2

---
 gcc/config/aarch64/falkor-tag-collision-avoidance.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/aarch64/falkor-tag-collision-avoidance.c b/gcc/config/aarch64/falkor-tag-collision-avoidance.c
index f850153fae02..fb56ff66bfab 100644
--- a/gcc/config/aarch64/falkor-tag-collision-avoidance.c
+++ b/gcc/config/aarch64/falkor-tag-collision-avoidance.c
@@ -538,7 +538,7 @@ valid_src_p (rtx src, rtx_insn *insn, struct loop *loop, bool *pre_post,
   if (!aarch64_classify_address (&addr, XEXP (x, 0), mode, true))
     return false;
 
-  if (!REG_P (addr.base))
+  if (addr.type == ADDRESS_SYMBOLIC || !REG_P (addr.base))
     return false;
 
Hmm, given that the code below only handles the  ADDRESS_REG_* forms, how about we get defensive here and check that addr.type is not one of ADDRESS_REG_* instead?
That way, if aarch64_classify_address is extended in the future with new addr.type values that leave addr.base in the wrong forms, this code will not need to be updated.
Thanks,
Kyrill


   unsigned regno = REGNO (addr.base);
-- 
2.17.1
Andrea Corallo April 14, 2020, 5:02 p.m. | #2
Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> writes:

> -  if (!REG_P (addr.base))

> +  if (addr.type == ADDRESS_SYMBOLIC || !REG_P (addr.base))

>      return false;

>

> Hmm, given that the code below only handles the  ADDRESS_REG_* forms, how about we get defensive here and check that addr.type is not one of ADDRESS_REG_* instead?

> That way, if aarch64_classify_address is extended in the future with new addr.type values that leave addr.base in the wrong forms, this code will not need to be updated.


Yeah make sense.

Thanks

  Andrea
Andrea Corallo April 15, 2020, 8:46 a.m. | #3
Hi all,

Second version of this addressing comments.

Bootstraped on aarch64 and regressioned.  Okay for trunk?

Andrea

gcc/ChangeLog

2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>

	* config/aarch64/falkor-tag-collision-avoidance.c
	(valid_src_p): Check for aarch64_address_info type before
	accessing base field.
diff --git a/gcc/config/aarch64/falkor-tag-collision-avoidance.c b/gcc/config/aarch64/falkor-tag-collision-avoidance.c
index f850153fae02..a96a3320e8fc 100644
--- a/gcc/config/aarch64/falkor-tag-collision-avoidance.c
+++ b/gcc/config/aarch64/falkor-tag-collision-avoidance.c
@@ -538,7 +538,11 @@ valid_src_p (rtx src, rtx_insn *insn, struct loop *loop, bool *pre_post,
   if (!aarch64_classify_address (&addr, XEXP (x, 0), mode, true))
     return false;
 
-  if (!REG_P (addr.base))
+  if (addr.type != ADDRESS_REG_IMM
+      && addr.type != ADDRESS_REG_WB
+      && addr.type != ADDRESS_REG_REG
+      && addr.type != ADDRESS_REG_UXTW
+      && addr.type != ADDRESS_REG_SXTW)
     return false;
 
   unsigned regno = REGNO (addr.base);
Kyrylo Tkachov April 15, 2020, 11:23 a.m. | #4
Hi Andrea,

> -----Original Message-----

> From: Andrea Corallo <Andrea.Corallo@arm.com>

> Sent: 15 April 2020 09:47

> To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>

> Cc: nd <nd@arm.com>; gcc-patches@gcc.gnu.org

> Subject: Re: [PATCH V2]aarch64: falkor-tag-collision-avoidance.c fix

> valid_src_p for use of uninitialized value

> 

> Hi all,

> 

> Second version of this addressing comments.

> 

> Bootstraped on aarch64 and regressioned.  Okay for trunk?


Ok.
Thanks,
Kyrill

> 

> Andrea

> 

> gcc/ChangeLog

> 

> 2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>

> 

> 	* config/aarch64/falkor-tag-collision-avoidance.c

> 	(valid_src_p): Check for aarch64_address_info type before

> 	accessing base field.
Andrea Corallo April 15, 2020, 1:09 p.m. | #5
Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> writes:

> Hi Andrea,

>

>> -----Original Message-----

>> From: Andrea Corallo <Andrea.Corallo@arm.com>

>> Sent: 15 April 2020 09:47

>> To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>

>> Cc: nd <nd@arm.com>; gcc-patches@gcc.gnu.org

>> Subject: Re: [PATCH V2]aarch64: falkor-tag-collision-avoidance.c fix

>> valid_src_p for use of uninitialized value

>>

>> Hi all,

>>

>> Second version of this addressing comments.

>>

>> Bootstraped on aarch64 and regressioned.  Okay for trunk?

>

> Ok.

> Thanks,

> Kyrill


Thanks pushed as 8a4436d89bfa.

Preparing the backport for gcc-9.

  Andrea
Andrea Corallo April 16, 2020, 8:01 a.m. | #6
Hi all,

I'd like to back-port this to the gcc-9 branch.
This patch is directly based on:

https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543627.html
https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543901.html

Bootstrapped and reg tested.  Ok for release/gcc-9?

Bests
  Andrea

gcc/ChangeLog

2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>

	* config/aarch64/falkor-tag-collision-avoidance.c
	(valid_src_p): Check for aarch64_address_info type before
	accessing base field.

gcc/testsuite/ChangeLog

2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>

	* gcc.target/aarch64/pr94530.c: New test.
From 1e70131c2c099c1071baba3f40d610f41ff4e9ea Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>

Date: Tue, 14 Apr 2020 19:51:54 +0100
Subject: [PATCH] pr94530 gcc-9

---
 gcc/config/aarch64/falkor-tag-collision-avoidance.c | 7 +++++++
 gcc/testsuite/gcc.target/aarch64/pr94530.c          | 9 +++++++++
 2 files changed, 16 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr94530.c

diff --git a/gcc/config/aarch64/falkor-tag-collision-avoidance.c b/gcc/config/aarch64/falkor-tag-collision-avoidance.c
index 779dee81f7f4..698d1595d0a6 100644
--- a/gcc/config/aarch64/falkor-tag-collision-avoidance.c
+++ b/gcc/config/aarch64/falkor-tag-collision-avoidance.c
@@ -537,6 +537,13 @@ valid_src_p (rtx src, rtx_insn *insn, struct loop *loop, bool *pre_post,
   if (!aarch64_classify_address (&addr, XEXP (x, 0), mode, true))
     return false;
 
+  if (addr.type != ADDRESS_REG_IMM
+      && addr.type != ADDRESS_REG_WB
+      && addr.type != ADDRESS_REG_REG
+      && addr.type != ADDRESS_REG_UXTW
+      && addr.type != ADDRESS_REG_SXTW)
+    return false;
+
   unsigned regno = REGNO (addr.base);
   if (global_regs[regno] || fixed_regs[regno])
     return false;
diff --git a/gcc/testsuite/gcc.target/aarch64/pr94530.c b/gcc/testsuite/gcc.target/aarch64/pr94530.c
new file mode 100644
index 000000000000..1f98201c50a8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr94530.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -mcpu=falkor -mpc-relative-literal-loads -mcmodel=large" } */
+
+extern void bar(const char *);
+
+void foo(void) {
+  for (;;)
+    bar("");
+}
-- 
2.17.1
Kyrylo Tkachov April 20, 2020, 8:04 a.m. | #7
> -----Original Message-----

> From: Andrea Corallo <Andrea.Corallo@arm.com>

> Sent: 16 April 2020 09:01

> To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>

> Cc: nd <nd@arm.com>; gcc-patches@gcc.gnu.org

> Subject: Re: [PATCH V2]aarch64: falkor-tag-collision-avoidance.c fix

> valid_src_p for use of uninitialized value

> 

> Hi all,

> 

> I'd like to back-port this to the gcc-9 branch.

> This patch is directly based on:

> 

> https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543627.html

> https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543901.html

> 

> Bootstrapped and reg tested.  Ok for release/gcc-9?


Ok.
Thanks,
Kyrill

> 

> Bests

>   Andrea

> 

> gcc/ChangeLog

> 

> 2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>

> 

> 	* config/aarch64/falkor-tag-collision-avoidance.c

> 	(valid_src_p): Check for aarch64_address_info type before

> 	accessing base field.

> 

> gcc/testsuite/ChangeLog

> 

> 2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>

> 

> 	* gcc.target/aarch64/pr94530.c: New test.
Andrea Corallo April 20, 2020, 12:32 p.m. | #8
Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> writes:

>> Bootstrapped and reg tested.  Ok for release/gcc-9?

>

> Ok.

> Thanks,

> Kyrill


Pushed as 5e022e3b3f7b.

Thanks
  Andrea

Patch

From 4af5bb32cd88bb4e65591207839fb0e276b2eb23 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>
Date: Thu, 9 Apr 2020 15:34:50 +0100
Subject: [PATCH] pr94530 2

---
 gcc/config/aarch64/falkor-tag-collision-avoidance.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/aarch64/falkor-tag-collision-avoidance.c b/gcc/config/aarch64/falkor-tag-collision-avoidance.c
index f850153fae02..fb56ff66bfab 100644
--- a/gcc/config/aarch64/falkor-tag-collision-avoidance.c
+++ b/gcc/config/aarch64/falkor-tag-collision-avoidance.c
@@ -538,7 +538,7 @@  valid_src_p (rtx src, rtx_insn *insn, struct loop *loop, bool *pre_post,
   if (!aarch64_classify_address (&addr, XEXP (x, 0), mode, true))
     return false;
 
-  if (!REG_P (addr.base))
+  if (addr.type == ADDRESS_SYMBOLIC || !REG_P (addr.base))
     return false;
 
   unsigned regno = REGNO (addr.base);
-- 
2.17.1