diff mbox

combine: Fix PR80233

Message ID e7b9da8d80a76e24b3a27144a8f32f7e4a725fee.1490811436.git.segher@kernel.crashing.org
State New
Headers show

Commit Message

Segher Boessenkool March 29, 2017, 6:23 p.m. UTC 
If combine has added an unconditional trap there will be a new basic
block as well.  It will then end up considering the NOTE_INSN_BASIC_BLOCK
as the last_combined_insn, but then it tries to take the DF_INSN_LUID
of that and that dereferences a NULL pointer (since such a note is not
an INSN_P).

This fixes it by not taking non-insns as last_combined_insn.

Bootstrapped and tested on powerpc64-linux {-m32,-m64}.


Segher


2017-03-29  Segher Boessenkool  <segher@kernel.crashing.org>

	PR rtl-optimization/80233
	* combine.c (combine_instructions): Only take NONDEBUG_INSN_P insns
	as last_combined_insn.  Do not test for BARRIER_P separately.

gcc/testsuite/
	PR rtl-optimization/80233
	* gcc.c-torture/compile/pr80233.c: New testcase.

---
 gcc/combine.c                                 |  4 ++--
 gcc/testsuite/gcc.c-torture/compile/pr80233.c | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr80233.c

Comments

Jeff Law March 29, 2017, 8:35 p.m. UTC | #1 
On 03/29/2017 12:23 PM, Segher Boessenkool wrote:
> If combine has added an unconditional trap there will be a new basic
> block as well.  It will then end up considering the NOTE_INSN_BASIC_BLOCK
> as the last_combined_insn, but then it tries to take the DF_INSN_LUID
> of that and that dereferences a NULL pointer (since such a note is not
> an INSN_P).
>
> This fixes it by not taking non-insns as last_combined_insn.
>
> Bootstrapped and tested on powerpc64-linux {-m32,-m64}.
>
>
> Segher
>
>
> 2017-03-29  Segher Boessenkool  <segher@kernel.crashing.org>
>
> 	PR rtl-optimization/80233
> 	* combine.c (combine_instructions): Only take NONDEBUG_INSN_P insns
> 	as last_combined_insn.  Do not test for BARRIER_P separately.
>
> gcc/testsuite/
> 	PR rtl-optimization/80233
> 	* gcc.c-torture/compile/pr80233.c: New testcase.
No strong opinions on this vs Jakub's patch.  I guess yours may walk 
more objects on the chain, but in doing so is more likely to find a 
useful LAST_COMBINED_INSN.  Jakub's stops earlier, but is less likely to 
have stopped on something useful.

Your call Segher.

jeff

ps.  Never in a million years would I have expected isolation of 
division by zero to have exposed as many latent issues as it has.  Sigh.
Jakub Jelinek March 29, 2017, 8:44 p.m. UTC | #2 
On Wed, Mar 29, 2017 at 02:35:32PM -0600, Jeff Law wrote:
> On 03/29/2017 12:23 PM, Segher Boessenkool wrote:
> > If combine has added an unconditional trap there will be a new basic
> > block as well.  It will then end up considering the NOTE_INSN_BASIC_BLOCK
> > as the last_combined_insn, but then it tries to take the DF_INSN_LUID
> > of that and that dereferences a NULL pointer (since such a note is not
> > an INSN_P).
> > 
> > This fixes it by not taking non-insns as last_combined_insn.
> > 
> > Bootstrapped and tested on powerpc64-linux {-m32,-m64}.
> > 
> > 
> > Segher
> > 
> > 
> > 2017-03-29  Segher Boessenkool  <segher@kernel.crashing.org>
> > 
> > 	PR rtl-optimization/80233
> > 	* combine.c (combine_instructions): Only take NONDEBUG_INSN_P insns
> > 	as last_combined_insn.  Do not test for BARRIER_P separately.
> > 
> > gcc/testsuite/
> > 	PR rtl-optimization/80233
> > 	* gcc.c-torture/compile/pr80233.c: New testcase.
> No strong opinions on this vs Jakub's patch.  I guess yours may walk more
> objects on the chain, but in doing so is more likely to find a useful
> LAST_COMBINED_INSN.  Jakub's stops earlier, but is less likely to have
> stopped on something useful.
> 
> Your call Segher.

I like Segher's latest patch.  But it is his call anyway ;)

	Jakub
Jeff Law March 29, 2017, 8:49 p.m. UTC | #3 
On 03/29/2017 02:44 PM, Jakub Jelinek wrote:
> On Wed, Mar 29, 2017 at 02:35:32PM -0600, Jeff Law wrote:
>> On 03/29/2017 12:23 PM, Segher Boessenkool wrote:
>>> If combine has added an unconditional trap there will be a new basic
>>> block as well.  It will then end up considering the NOTE_INSN_BASIC_BLOCK
>>> as the last_combined_insn, but then it tries to take the DF_INSN_LUID
>>> of that and that dereferences a NULL pointer (since such a note is not
>>> an INSN_P).
>>>
>>> This fixes it by not taking non-insns as last_combined_insn.
>>>
>>> Bootstrapped and tested on powerpc64-linux {-m32,-m64}.
>>>
>>>
>>> Segher
>>>
>>>
>>> 2017-03-29  Segher Boessenkool  <segher@kernel.crashing.org>
>>>
>>> 	PR rtl-optimization/80233
>>> 	* combine.c (combine_instructions): Only take NONDEBUG_INSN_P insns
>>> 	as last_combined_insn.  Do not test for BARRIER_P separately.
>>>
>>> gcc/testsuite/
>>> 	PR rtl-optimization/80233
>>> 	* gcc.c-torture/compile/pr80233.c: New testcase.
>> No strong opinions on this vs Jakub's patch.  I guess yours may walk more
>> objects on the chain, but in doing so is more likely to find a useful
>> LAST_COMBINED_INSN.  Jakub's stops earlier, but is less likely to have
>> stopped on something useful.
>>
>> Your call Segher.
>
> I like Segher's latest patch.  But it is his call anyway ;)
In that case let's just go with Segher's patch :-)

jeff
Segher Boessenkool March 29, 2017, 8:49 p.m. UTC | #4 
On Wed, Mar 29, 2017 at 02:35:32PM -0600, Jeff Law wrote:
> ps.  Never in a million years would I have expected isolation of 
> division by zero to have exposed as many latent issues as it has.  Sigh.

The trap insns weren't handled very well before, but we didn't generate
many either.  They still aren't handled very well, but hopefully we don't
ICE so much on them anymore ;-)


Segher
diff mbox

Patch

diff --git a/gcc/combine.c b/gcc/combine.c
index 87e9670..88ac475 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -1250,10 +1250,10 @@  combine_instructions (rtx_insn *f, unsigned int nregs)
 	    continue;
 
 	  while (last_combined_insn
-		 && last_combined_insn->deleted ())
+		 && (!NONDEBUG_INSN_P (last_combined_insn)
+		     || last_combined_insn->deleted ()))
 	    last_combined_insn = PREV_INSN (last_combined_insn);
 	  if (last_combined_insn == NULL_RTX
-	      || BARRIER_P (last_combined_insn)
 	      || BLOCK_FOR_INSN (last_combined_insn) != this_basic_block
 	      || DF_INSN_LUID (last_combined_insn) <= DF_INSN_LUID (insn))
 	    last_combined_insn = insn;
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr80233.c b/gcc/testsuite/gcc.c-torture/compile/pr80233.c
new file mode 100644
index 0000000..eb9b4a4
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr80233.c
@@ -0,0 +1,22 @@ 
+/* PR rtl-optimization/80233 */
+
+int xg;
+
+void
+t4 (int o9)
+{
+  int it;
+
+  if (o9 == 0)
+    {
+      int fx;
+
+      xg *= it;
+      if (xg == 0)
+        it /= 0;
+
+      fx = (it != 0) ? (xg < 0) : (xg / o9);
+      if (fx != 0)
+        xg = 0;
+    }
+}