[pushed] Darwin : Fix out-of-bounds access to df_regs_ever_live.

Message ID 47CF2BB7-F330-44EB-8C76-83C3498D1E30@sandoe.co.uk
State New
Headers show
Series
  • [pushed] Darwin : Fix out-of-bounds access to df_regs_ever_live.
Related show

Commit Message

Iain Sandoe April 3, 2021, 8:39 p.m.
Hi

I’ve been chasing a bootstrap failure present when we use clang as
the bootstrap - and seen for GCC11 (where the requirement is C++11)
even though the same clang toolchain bootstraps GCC-10.

The C++11 aspect turned out to be a red-herring .. the problem was
latent already, and is a long-standing regression - most likely since the
changes for LRA.

The patch applied is a conservative fix for GCC-11 and backporting to
the open release branches.

During GCC-12, I plan to factor this code (since I now have a third arch
to deal with) and continue machopic tidy up.  I have a strong suspicion
that this code is actually dead with LRA (but was not removed at the time
the change was made).  However, taking it out is too risky at this stage.

tested across the Darwin range,
pushed to master, so far.

———

This is needed on the open branches, and given that we are re-spining
10.3 - I’d like to push it there rather than tell my downstream to apply it
after the release.


OK for 10.3?

-- 
2.24.1

Comments

Richard Biener April 4, 2021, 8:20 a.m. | #1
On April 3, 2021 10:39:20 PM GMT+02:00, Iain Sandoe <iain@sandoe.co.uk> wrote:
>Hi

>

>I’ve been chasing a bootstrap failure present when we use clang as

>the bootstrap - and seen for GCC11 (where the requirement is C++11)

>even though the same clang toolchain bootstraps GCC-10.

>

>The C++11 aspect turned out to be a red-herring .. the problem was

>latent already, and is a long-standing regression - most likely since

>the

>changes for LRA.

>

>The patch applied is a conservative fix for GCC-11 and backporting to

>the open release branches.

>

>During GCC-12, I plan to factor this code (since I now have a third

>arch

>to deal with) and continue machopic tidy up.  I have a strong suspicion

>that this code is actually dead with LRA (but was not removed at the

>time

>the change was made).  However, taking it out is too risky at this

>stage.

>

>tested across the Darwin range,

>pushed to master, so far.

>

>———

>

>This is needed on the open branches, and given that we are re-spining

>10.3 - I’d like to push it there rather than tell my downstream to

>apply it

>after the release.

>

>

>OK for 10.3?


Ok. 

Richard. 

>=====

>

>During changes made for LRA (or, perhaps, even before) we omitted

>a check that the current register we are working on is a hard reg

>before we tried to note its liveness.

>

>A stage 1 built with fsanitize=address catches this, as does any

>attempt to build master with clang and -std=c++11.

>

>gcc/ChangeLog:

>

>	* config/darwin.c (machopic_legitimize_pic_address): Check

>	that the current pic register is one of the hard reg set

>	before setting liveness.

>---

> gcc/config/darwin.c | 5 +++--

> 1 file changed, 3 insertions(+), 2 deletions(-)

>

>diff --git a/gcc/config/darwin.c b/gcc/config/darwin.c

>index c1086a04700..5d173919ee0 100644

>--- a/gcc/config/darwin.c

>+++ b/gcc/config/darwin.c

>@@ -907,7 +907,7 @@ machopic_legitimize_pic_address (rtx orig,

>machine_mode mode, rtx reg)

> 		  pic = reg;

> 		}

> 

>-	      if (lra_in_progress)

>+	      if (lra_in_progress && HARD_REGISTER_P (pic))

> 		df_set_regs_ever_live (REGNO (pic), true);

> 	      pic_ref = gen_rtx_PLUS (Pmode, pic,

> 				      machopic_gen_offset (XEXP (orig, 0)));

>@@ -974,7 +974,8 @@ machopic_legitimize_pic_address (rtx orig,

>machine_mode mode, rtx reg)

> 		      emit_move_insn (reg, pic);

> 		      pic = reg;

> 		    }

>-		  if (lra_in_progress)

>+

>+		  if (lra_in_progress && HARD_REGISTER_P (pic))

> 		    df_set_regs_ever_live (REGNO (pic), true);

> 		  pic_ref = gen_rtx_PLUS (Pmode,

> 					  pic,

Patch

=====

During changes made for LRA (or, perhaps, even before) we omitted
a check that the current register we are working on is a hard reg
before we tried to note its liveness.

A stage 1 built with fsanitize=address catches this, as does any
attempt to build master with clang and -std=c++11.

gcc/ChangeLog:

	* config/darwin.c (machopic_legitimize_pic_address): Check
	that the current pic register is one of the hard reg set
	before setting liveness.
---
 gcc/config/darwin.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gcc/config/darwin.c b/gcc/config/darwin.c
index c1086a04700..5d173919ee0 100644
--- a/gcc/config/darwin.c
+++ b/gcc/config/darwin.c
@@ -907,7 +907,7 @@  machopic_legitimize_pic_address (rtx orig, machine_mode mode, rtx reg)
 		  pic = reg;
 		}
 
-	      if (lra_in_progress)
+	      if (lra_in_progress && HARD_REGISTER_P (pic))
 		df_set_regs_ever_live (REGNO (pic), true);
 	      pic_ref = gen_rtx_PLUS (Pmode, pic,
 				      machopic_gen_offset (XEXP (orig, 0)));
@@ -974,7 +974,8 @@  machopic_legitimize_pic_address (rtx orig, machine_mode mode, rtx reg)
 		      emit_move_insn (reg, pic);
 		      pic = reg;
 		    }
-		  if (lra_in_progress)
+
+		  if (lra_in_progress && HARD_REGISTER_P (pic))
 		    df_set_regs_ever_live (REGNO (pic), true);
 		  pic_ref = gen_rtx_PLUS (Pmode,
 					  pic,