Re: Small problem in cse

Re: Small problem in cse

[Christian Iseli]

>   In message < 199710172043.WAA24596@Rivendell.MiddleEarth.net >you write:
>   > I've received a segfault from cc1 due to a NULL dereference in cse.c.
>   > The  following trivial patch cures the problem.
> Yes, but _why_ is classp NULL?

classp is not NULL, but classp->first_same_value is NULL, and later dereferenced.
I don't *know* why that field is NULL, I just assumed that no same value had
been seen yet...

					Christian

Re: Small problem in cse

[Jeffrey A Law]

  In message < 199710200842.KAA16565@lslsun17.epfl.ch >you write:
  > >   In message < 199710172043.WAA24596@Rivendell.MiddleEarth.net >you write:
  > >   > I've received a segfault from cc1 due to a NULL dereference in cse.c.
  > >   > The  following trivial patch cures the problem.
  > > Yes, but _why_ is classp NULL?
  > 
  > classp is not NULL, but classp->first_same_value is NULL, and later
  > dereferenced.  I don't *know* why that field is NULL, I just assumed
  > that no same value had been seen yet...
Sorry, I meant to say why is classp->first_same_value NULL?

From my review of the code I don't see that classp->first_same_value should
ever be NULL -- thus I suspect something has gone wrong elsewhere that
needs to be investigated.

But I could be wrong, since you've got a target & testcase which triggers
this problem you'll need to do some of the analysis.



Thanks,
jeff

Re: Small problem in cse

[Christian Iseli]

law@hurl.cygnus.com said:
> Sorry, I meant to say why is classp->first_same_value NULL?

> From my review of the code I don't see that classp->first_same_value 
> should ever be NULL -- thus I suspect something has gone wrong 
> elsewhere that needs to be investigated.

> But I could be wrong, since you've got a target & testcase which 
> triggers this problem you'll need to do some of the analysis. 

Ok, I'll do my best and keep you posted...

					Christian

Re: multiple_of_p change

[Christian Iseli]

>> Sorry, I meant to say why is classp->first_same_value NULL?

>> From my review of the code I don't see that classp->first_same_value
>> should ever be NULL -- thus I suspect something has gone wrong
>> elsewhere that needs to be investigated.

>> But I could be wrong, since you've got a target & testcase which
>> triggers this problem you'll need to do some of the analysis.

>Ok, I'll do my best and keep you posted...

Well, it seems you were right about the fact that first_same_value should never
be null... but here is what I managed to observe so far...

cse_insn is called with the following insn
(insn 4708 4707 4709 (set (subreg:SF (reg/v:SI 47) 0)
        (const_double:SF (const_int 0) 0 1076953088)) 4 {movsf} (nil)
    (expr_list:REG_EQUAL (minus:SF (const_double:SF (const_int 0) 0 1077018624)
            (const_double:SF (cc0) 0 1072693248))
        (nil)))
This is for an 8-bit target, where the source is attempting to do a 
pre-decrement of a long double
number.  The target defines float as TQF (24 bits), double and long double are 
both SF (32 bits).

During the processing, the subreg case around line 7390 of cse.c is entered.
a classp is found, and its exp field is (subreg:SI (reg/v:SF 35) 0).  At the 
next loop iteration,
new_src is set from gen_lowpart_if_possible to the same expression (subreg:SI 
(reg/v:SF 35) 0).
The *bad* thing is that HASH produces a different value than it did when 
classp->exp was hashed.
So, when insert_regs is called, the element pointed to by classp is deleted 
from the hash table
and thus classp->first_same_value becomes 0...

But now I'm stuck (and tired... ;-).  What would be the right thing(tm) to do 
now?

					Christian

Re: multiple_of_p change

[Jeffrey A Law]

  In message <199710242313.BAA08816@Rivendell.MiddleEarth.net>you write:
  > During the processing, the subreg case around line 7390 of cse.c is entered.
  > a classp is found, and its exp field is (subreg:SI (reg/v:SF 35) 0).  At the 
  > next loop iteration, new_src is set from gen_lowpart_if_possible to the same
  > expression (subreg:SI (reg/v:SF 35) 0).
  > The *bad* thing is that HASH produces a different value than it did when 
  > classp->exp was hashed.
OK.  This is where I'd suggest you concentrate your effort -- figure
out how/why the same RTL hashes differently from the two different
call sites.

jeff

Re: Small problem in cse

[Christian Iseli]

> Sorry, I meant to say why is classp->first_same_value NULL?
> 
> From my review of the code I don't see that classp->first_same_value should
> ever be NULL -- thus I suspect something has gone wrong elsewhere that
> needs to be investigated.
> 
> But I could be wrong, since you've got a target & testcase which triggers
> this problem you'll need to do some of the analysis.

Sorry for the long delay...

Turns out you were right, the problem was elsewhere.

Basically, the code around line 74xx in cse.c tries to find some
equivalence in the table and then sets classp to be the
first_same_value of the equivalence chain.  However, no check was
done to ensure that this first_same_value was valid.  So when
remove_invalid_refs was called later, the element pointed to by
classp was deleted, leading to trouble...

The following patch cures the problem (I hope :-)

Cheers,
					Christian

Tue Dec  2 16:07:45 1997  Christian Iseli  <Christian.Iseli@lslsun.epfl.ch>

	* cse.c (cse_insn): Check for invalid entries when taking references.


*** cse.c~	Mon Nov 17 07:50:55 1997
--- cse.c	Tue Dec  2 15:49:50 1997
*************** cse_insn (insn, in_libcall_block)
*** 7428,7433 ****
--- 7428,7438 ----
  		  merge_equiv_classes (src_elt, classp);
  
  		classp = src_elt->first_same_value;
+ 		/* Ignore invalid entries.  */
+ 		while (classp
+ 		       && GET_CODE (classp->exp) != REG
+ 		       && ! exp_equiv_p (classp->exp, classp->exp, 1, 0))
+ 		  classp = classp->next_same_value;
  	      }
  	  }
        }

Re: Small problem in cse

[Jeffrey A Law]

  In message < 199712021510.QAA08509@lslsun17.epfl.ch >you write:
  > Sorry for the long delay...
No problem.  It's worth trying to fix this correctly.

  > Turns out you were right, the problem was elsewhere.
  > 
  > Basically, the code around line 74xx in cse.c tries to find some
  > equivalence in the table and then sets classp to be the
  > first_same_value of the equivalence chain.  However, no check was
  > done to ensure that this first_same_value was valid.  So when
  > remove_invalid_refs was called later, the element pointed to by
  > classp was deleted, leading to trouble...
Sounds good to me.  I first thought that your change might be too
late to catch the problems, but it turns out that merge_equiv_classes
will to the correct thing for invalid entries.  So i think your patch
is correct and I've installed it.


  > Tue Dec  2 16:07:45 1997  Christian Iseli  <Christian.Iseli@lslsun.epfl.ch>
  > 
  > 	* cse.c (cse_insn): Check for invalid entries when taking references.
jeff

Re: Small problem in cse

[Christian Iseli]

> Fix HASH.

Right :-) ...  However, I'm not convinced yet that HASH is the real problem.

Is there any reason that reg_qty of a given reg might change from one call
of HASH to the next?

					Christian

Re: Small problem in cse

[Joern Rennecke]

> > Fix HASH.
> 
> Right :-) ...  However, I'm not convinced yet that HASH is the real problem.
> 
> Is there any reason that reg_qty of a given reg might change from one call
> of HASH to the next?

If the register is assigned something new, or a label is passed, that is
a valid reason.  Otherwise it's probably a bug.

Re: Small problem in cse

[Christian Iseli]

Sorry folks, I sent this with the wrong subject...  I'll just resend it here
so that threading and procmail freaks don't get confused...

>> Sorry, I meant to say why is classp->first_same_value NULL?

>> From my review of the code I don't see that classp->first_same_value
>> should ever be NULL -- thus I suspect something has gone wrong
>> elsewhere that needs to be investigated.

>> But I could be wrong, since you've got a target & testcase which
>> triggers this problem you'll need to do some of the analysis.

>Ok, I'll do my best and keep you posted...

Well, it seems you were right about the fact that first_same_value should never
be null... but here is what I managed to observe so far...

cse_insn is called with the following insn
(insn 4708 4707 4709 (set (subreg:SF (reg/v:SI 47) 0)
        (const_double:SF (const_int 0) 0 1076953088)) 4 {movsf} (nil)
    (expr_list:REG_EQUAL (minus:SF (const_double:SF (const_int 0) 0 1077018624)
            (const_double:SF (cc0) 0 1072693248))
        (nil)))
This is for an 8-bit target, where the source is attempting to do a 
pre-decrement of a long double
number.  The target defines float as TQF (24 bits), double and long double are 
both SF (32 bits).

During the processing, the subreg case around line 7390 of cse.c is entered.
a classp is found, and its exp field is (subreg:SI (reg/v:SF 35) 0).  At the 
next loop iteration,
new_src is set from gen_lowpart_if_possible to the same expression (subreg:SI 
(reg/v:SF 35) 0).
The *bad* thing is that HASH produces a different value than it did when 
classp->exp was hashed.
So, when insert_regs is called, the element pointed to by classp is deleted 
from the hash table
and thus classp->first_same_value becomes 0...

But now I'm stuck (and tired... ;-).  What would be the right thing(tm) to do 
now?

					Christian

Re: Small problem in cse

[Joern Rennecke]

> The *bad* thing is that HASH produces a different value than it did when 
> classp->exp was hashed.
> So, when insert_regs is called, the element pointed to by classp is deleted 
> from the hash table
> and thus classp->first_same_value becomes 0...
> 
> But now I'm stuck (and tired... ;-).  What would be the right thing(tm) to do 
> now?

Fix HASH.

Small problem in cse

[Christian Iseli]

Hi,

I've received a segfault from cc1 due to a NULL dereference in cse.c.  The 
following
trivial patch cures the problem.

					Christian

Fri Oct 17 12:29:48 1997  Christian Iseli  <Christian.Iseli@lslsun.epfl.ch>

	* cse.c (insert): check that classp->first_same_value is not NULL
 	before dereferencing.


*** cse.c.orig	Wed Oct  1 07:46:27 1997
--- cse.c	Fri Oct 17 22:30:11 1997
*************** insert (x, classp, hash, mode)
*** 1323,1329 ****
    table[hash] = elt;
  
    /* Put it into the proper value-class.  */
!   if (classp)
      {
        classp = classp->first_same_value;
        if (CHEAPER (elt, classp))
--- 1323,1329 ----
    table[hash] = elt;
  
    /* Put it into the proper value-class.  */
!   if (classp && classp->first_same_value)
      {
        classp = classp->first_same_value;
        if (CHEAPER (elt, classp))

Re: Small problem in cse

[Jeffrey A Law]

  In message < 199710172043.WAA24596@Rivendell.MiddleEarth.net >you write:
  > I've received a segfault from cc1 due to a NULL dereference in cse.c.
  > The  following trivial patch cures the problem.
Yes, but _why_ is classp NULL?

If classp isn't supposed to ever be NULL at this point in the code,
then your patch will just fix the symptom instead of the real
problem.

I'm not rejecting the patch, but I'd like some more information
before installing it.

jeff