Patchwork [PR,debug/53682] avoid crash in cselib promote_debug_loc

login
register
mail settings
Submitter Alexandre Oliva
Date June 20, 2012, 3:39 a.m.
Message ID <orfw9q1wjy.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/165871/
State New
Headers show

Comments

Alexandre Oliva - June 20, 2012, 3:39 a.m.
When promote_debug_loc was first introduced, it would never be called
with a NULL loc list.  However, because of the strategy of temporarily
resetting loc lists before recursion introduced a few months ago in
alias.c, the earlier assumption no longer holds.

This patch adusts promote_debug_loc to deal with this case.

Ok to install?
Jakub Jelinek - June 20, 2012, 6:19 a.m.
On Wed, Jun 20, 2012 at 12:39:29AM -0300, Alexandre Oliva wrote:
> When promote_debug_loc was first introduced, it would never be called
> with a NULL loc list.  However, because of the strategy of temporarily
> resetting loc lists before recursion introduced a few months ago in
> alias.c, the earlier assumption no longer holds.
> 
> This patch adusts promote_debug_loc to deal with this case.

The thing I'm worried about is what will happen with -g0 in that case.
If the loc list is temporarily reset, it will be restored again,
won't that mean that for -g0 we'll then have a loc that is in the
corresponding -g compilation referenced by a DEBUG_INSNs only (and thus
non-promoted)?

> for  gcc/ChangeLog
> from  Alexandre Oliva  <aoliva@redhat.com>
> 
> 	PR debug/53682
> 	* cselib.c (promote_debug_loc): Don't crash on NULL argument.
> 
> Index: gcc/cselib.c
> ===================================================================
> --- gcc/cselib.c.orig	2012-06-17 22:52:27.740087279 -0300
> +++ gcc/cselib.c	2012-06-18 08:55:32.948832112 -0300
> @@ -322,7 +322,7 @@ new_elt_loc_list (cselib_val *val, rtx l
>  static inline void
>  promote_debug_loc (struct elt_loc_list *l)
>  {
> -  if (l->setting_insn && DEBUG_INSN_P (l->setting_insn)
> +  if (l && l->setting_insn && DEBUG_INSN_P (l->setting_insn)
>        && (!cselib_current_insn || !DEBUG_INSN_P (cselib_current_insn)))
>      {
>        n_debug_values--;

	Jakub
Alexandre Oliva - June 20, 2012, 11:43 p.m.
On Jun 20, 2012, Jakub Jelinek <jakub@redhat.com> wrote:

> On Wed, Jun 20, 2012 at 12:39:29AM -0300, Alexandre Oliva wrote:
>> When promote_debug_loc was first introduced, it would never be called
>> with a NULL loc list.  However, because of the strategy of temporarily
>> resetting loc lists before recursion introduced a few months ago in
>> alias.c, the earlier assumption no longer holds.

>> This patch adusts promote_debug_loc to deal with this case.

> The thing I'm worried about is what will happen with -g0 in that case.
> If the loc list is temporarily reset, it will be restored again,
> won't that mean that for -g0 we'll then have a loc that is in the
> corresponding -g compilation referenced by a DEBUG_INSNs only (and thus
> non-promoted)?

I don't see how.  If we get to a NULL loc list, it means it's not the
first time we visit that node (it was visited upstack), so if it needed
promotion, it would have already been promoted then.
Jakub Jelinek - June 21, 2012, 6:04 a.m.
On Wed, Jun 20, 2012 at 08:43:53PM -0300, Alexandre Oliva wrote:
> On Jun 20, 2012, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> > On Wed, Jun 20, 2012 at 12:39:29AM -0300, Alexandre Oliva wrote:
> >> When promote_debug_loc was first introduced, it would never be called
> >> with a NULL loc list.  However, because of the strategy of temporarily
> >> resetting loc lists before recursion introduced a few months ago in
> >> alias.c, the earlier assumption no longer holds.
> 
> >> This patch adusts promote_debug_loc to deal with this case.
> 
> > The thing I'm worried about is what will happen with -g0 in that case.
> > If the loc list is temporarily reset, it will be restored again,
> > won't that mean that for -g0 we'll then have a loc that is in the
> > corresponding -g compilation referenced by a DEBUG_INSNs only (and thus
> > non-promoted)?
> 
> I don't see how.  If we get to a NULL loc list, it means it's not the
> first time we visit that node (it was visited upstack), so if it needed
> promotion, it would have already been promoted then.

The patch is ok then.

	Jakub
Alexandre Oliva - June 25, 2012, 10:32 a.m.
On Jun 20, 2012, Alexandre Oliva <aoliva@redhat.com> wrote:

> When promote_debug_loc was first introduced, it would never be called
> with a NULL loc list.  However, because of the strategy of temporarily
> resetting loc lists before recursion introduced a few months ago in
> alias.c, the earlier assumption no longer holds.

> This patch adusts promote_debug_loc to deal with this case.

> Ok to install?

Unless there are objections in the next 24 hours, I'm going to install
this in the 4.7 branch too.  Regstrapped on x86_64-linux-gnu and
i686-linux-gnu.

> for  gcc/ChangeLog
> from  Alexandre Oliva  <aoliva@redhat.com>

> 	PR debug/53682
> 	* cselib.c (promote_debug_loc): Don't crash on NULL argument.
Jakub Jelinek - June 25, 2012, 10:40 a.m.
On Mon, Jun 25, 2012 at 07:32:04AM -0300, Alexandre Oliva wrote:
> Unless there are objections in the next 24 hours, I'm going to install
> this in the 4.7 branch too.  Regstrapped on x86_64-linux-gnu and
> i686-linux-gnu.

Just install it right away.  Thanks.

> > for  gcc/ChangeLog
> > from  Alexandre Oliva  <aoliva@redhat.com>
> 
> > 	PR debug/53682
> > 	* cselib.c (promote_debug_loc): Don't crash on NULL argument.

	Jakub

Patch

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/53682
	* cselib.c (promote_debug_loc): Don't crash on NULL argument.

Index: gcc/cselib.c
===================================================================
--- gcc/cselib.c.orig	2012-06-17 22:52:27.740087279 -0300
+++ gcc/cselib.c	2012-06-18 08:55:32.948832112 -0300
@@ -322,7 +322,7 @@  new_elt_loc_list (cselib_val *val, rtx l
 static inline void
 promote_debug_loc (struct elt_loc_list *l)
 {
-  if (l->setting_insn && DEBUG_INSN_P (l->setting_insn)
+  if (l && l->setting_insn && DEBUG_INSN_P (l->setting_insn)
       && (!cselib_current_insn || !DEBUG_INSN_P (cselib_current_insn)))
     {
       n_debug_values--;