[PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Jakub Jelinek]

Hi!

The following testcase fails -fcompare-debug on aarch64-linux.  The problem
is that for the n variable we create a varpool node, then remove it again
because the var isn't really used, but it keeps being referenced in debug
stmts/insns with -g.  Later during sched1 pass we ask whether the n var
can be modified through some store to an anchored variable and with -g
we create a new varpool node for it again just so that we can find its
ultimate alias target.  Even later on we create some cgraph node for the
loop parallelization, but as there has been an extra varpool node creation
in between, we get higher node->order with -g than without.

I think we just shouldn't create varpool nodes during RTL optimizations,
either the vars exist and we have varpool nodes for those, or they are some
late created variables which will have itself as ultimate alias target
(in debugging dumps I've done on powerpc64le-linux for this, 828576 out of
828580 cases where symtab_node::get (x_decl) returned NULL here it was
some DECL_IN_CONSTANT_POOL decl so certainly
symtab_node::get_create (x_decl)->ultimate_alias_target ()->decl == x_decl
), or as in this case it is a var referenced only in debug insns and we'd
better not to create varpool node for it, just:
gcc.dg/pr105415.c foo n
gcc.dg/pr105415.c foo n
gcc.dg/torture/pr41555.c main g_47
gcc.dg/torture/pr41555.c main g_47
).
So the following patch calls just get instead of get_create and assumes
that if we'd create a new varpool node, it would have itself as its
ultimate_alias_target that late and it would be a definition unless
DECL_EXTERNAL.

Bootstrapped/regtested on aarch64-linux and powerpc64le-linux, ok for trunk?

2022-04-29  Jakub Jelinek  <jakub@redhat.com>

	PR debug/105415
	* alias.cc (compare_base_symbol_refs): Avoid creating new symtab
	nodes, if it doesn't exist, punt if DECL_EXTERNAL, otherwise use
	x_decl instead of x_node->decl.

	* gcc.dg/pr105415.c: New test.

--- gcc/alias.cc.jj	2022-02-21 16:51:50.261232505 +0100
+++ gcc/alias.cc	2022-04-28 11:57:18.940425126 +0200
@@ -2219,12 +2219,19 @@ compare_base_symbol_refs (const_rtx x_ba
 	  || (!TREE_STATIC (x_decl) && !TREE_PUBLIC (x_decl)))
 	return 0;
 
-      symtab_node *x_node = symtab_node::get_create (x_decl)
-			    ->ultimate_alias_target ();
-      /* External variable cannot be in section anchor.  */
-      if (!x_node->definition)
+      symtab_node *x_node = symtab_node::get (x_decl);
+      tree x_decl2 = x_decl;
+      if (x_node != NULL)
+	{
+	  x_node = x_node->ultimate_alias_target ();
+	  /* External variable cannot be in section anchor.  */
+	  if (!x_node->definition)
+	    return 0;
+	  x_decl2 = x_node->decl;
+	}
+      else if (DECL_EXTERNAL (x_decl))
 	return 0;
-      x_base = XEXP (DECL_RTL (x_node->decl), 0);
+      x_base = XEXP (DECL_RTL (x_decl2), 0);
       /* If not in anchor, we can disambiguate.  */
       if (!SYMBOL_REF_HAS_BLOCK_INFO_P (x_base))
 	return 0;
--- gcc/testsuite/gcc.dg/pr105415.c.jj	2022-04-28 12:26:13.174302870 +0200
+++ gcc/testsuite/gcc.dg/pr105415.c	2022-04-28 12:25:36.770809149 +0200
@@ -0,0 +1,26 @@
+/* PR debug/105415 */
+/* { dg-do compile } */
+/* { dg-require-effective-target pthread } */
+/* { dg-options "-O2 -ftree-parallelize-loops=2 -fcompare-debug" } */
+
+int m;
+static int n;
+
+void
+foo (void)
+{
+  int s = 0;
+
+  while (m < 1)
+    {
+      s += n;
+      ++m;
+    }
+}
+
+void
+bar (int *arr, int i)
+{
+  while (i < 1)
+    arr[i++] = 1;
+}

	Jakub

Re: [PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Richard Biener]

On Fri, 29 Apr 2022, Jakub Jelinek wrote:

> Hi!
> 
> The following testcase fails -fcompare-debug on aarch64-linux.  The problem
> is that for the n variable we create a varpool node, then remove it again
> because the var isn't really used, but it keeps being referenced in debug
> stmts/insns with -g.  Later during sched1 pass we ask whether the n var
> can be modified through some store to an anchored variable and with -g
> we create a new varpool node for it again just so that we can find its
> ultimate alias target.  Even later on we create some cgraph node for the
> loop parallelization, but as there has been an extra varpool node creation
> in between, we get higher node->order with -g than without.
> 
> I think we just shouldn't create varpool nodes during RTL optimizations,
> either the vars exist and we have varpool nodes for those, or they are some
> late created variables which will have itself as ultimate alias target
> (in debugging dumps I've done on powerpc64le-linux for this, 828576 out of
> 828580 cases where symtab_node::get (x_decl) returned NULL here it was
> some DECL_IN_CONSTANT_POOL decl so certainly
> symtab_node::get_create (x_decl)->ultimate_alias_target ()->decl == x_decl
> ), or as in this case it is a var referenced only in debug insns and we'd
> better not to create varpool node for it, just:
> gcc.dg/pr105415.c foo n
> gcc.dg/pr105415.c foo n
> gcc.dg/torture/pr41555.c main g_47
> gcc.dg/torture/pr41555.c main g_47
> ).
> So the following patch calls just get instead of get_create and assumes
> that if we'd create a new varpool node, it would have itself as its
> ultimate_alias_target that late and it would be a definition unless
> DECL_EXTERNAL.
> 
> Bootstrapped/regtested on aarch64-linux and powerpc64le-linux, ok for trunk?

I think that's reasonable (we indeed shouldn't create a varpool node
here).  I do think that we eventually want to retain removed nodes
but mark them so.  In fact any debug references will be thrown away
because of this anyway.

So I wonder if we can instead simply do if (!x_node) return 0;?
The question is also why sched does any queries for debug-insns,
does it merely reset them based on the answer?  That said,
it would be nice to be able to assert that x_node is not NULL
and catch this in the callers somehow.

Richard.

> 2022-04-29  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/105415
> 	* alias.cc (compare_base_symbol_refs): Avoid creating new symtab
> 	nodes, if it doesn't exist, punt if DECL_EXTERNAL, otherwise use
> 	x_decl instead of x_node->decl.
> 
> 	* gcc.dg/pr105415.c: New test.
> 
> --- gcc/alias.cc.jj	2022-02-21 16:51:50.261232505 +0100
> +++ gcc/alias.cc	2022-04-28 11:57:18.940425126 +0200
> @@ -2219,12 +2219,19 @@ compare_base_symbol_refs (const_rtx x_ba
>  	  || (!TREE_STATIC (x_decl) && !TREE_PUBLIC (x_decl)))
>  	return 0;
>  
> -      symtab_node *x_node = symtab_node::get_create (x_decl)
> -			    ->ultimate_alias_target ();
> -      /* External variable cannot be in section anchor.  */
> -      if (!x_node->definition)
> +      symtab_node *x_node = symtab_node::get (x_decl);
> +      tree x_decl2 = x_decl;
> +      if (x_node != NULL)
> +	{
> +	  x_node = x_node->ultimate_alias_target ();
> +	  /* External variable cannot be in section anchor.  */
> +	  if (!x_node->definition)
> +	    return 0;
> +	  x_decl2 = x_node->decl;
> +	}
> +      else if (DECL_EXTERNAL (x_decl))
>  	return 0;
> -      x_base = XEXP (DECL_RTL (x_node->decl), 0);
> +      x_base = XEXP (DECL_RTL (x_decl2), 0);
>        /* If not in anchor, we can disambiguate.  */
>        if (!SYMBOL_REF_HAS_BLOCK_INFO_P (x_base))
>  	return 0;
> --- gcc/testsuite/gcc.dg/pr105415.c.jj	2022-04-28 12:26:13.174302870 +0200
> +++ gcc/testsuite/gcc.dg/pr105415.c	2022-04-28 12:25:36.770809149 +0200
> @@ -0,0 +1,26 @@
> +/* PR debug/105415 */
> +/* { dg-do compile } */
> +/* { dg-require-effective-target pthread } */
> +/* { dg-options "-O2 -ftree-parallelize-loops=2 -fcompare-debug" } */
> +
> +int m;
> +static int n;
> +
> +void
> +foo (void)
> +{
> +  int s = 0;
> +
> +  while (m < 1)
> +    {
> +      s += n;
> +      ++m;
> +    }
> +}
> +
> +void
> +bar (int *arr, int i)
> +{
> +  while (i < 1)
> +    arr[i++] = 1;
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)

Re: [PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Jakub Jelinek]

On Fri, Apr 29, 2022 at 11:32:15AM +0200, Richard Biener wrote:
> I think that's reasonable (we indeed shouldn't create a varpool node
> here).  I do think that we eventually want to retain removed nodes
> but mark them so.  In fact any debug references will be thrown away
> because of this anyway.
> 
> So I wonder if we can instead simply do if (!x_node) return 0;?

I had that in my first version, but after finding out that it triggers
so often for the constant pool decls I thought better to just use
x_decl in that case instead of x_node->decl.
I must say I'm unsure if constant pool decls always stay out of section
anchors or if they can be put there too.

> The question is also why sched does any queries for debug-insns,
> does it merely reset them based on the answer?  That said,
> it would be nice to be able to assert that x_node is not NULL
> and catch this in the callers somehow.

Unfortunately, several layers of callers don't really know it is for debug
insn.  And the touched code is solely for the section anchors, so e.g. just
checking symtab_node::get (decl) on all mentioned decls when we perhaps can
see if it is debug insn or not would be quite costly and we wouldn't know
if the other reference is anchored.

	Jakub

Re: [PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Richard Biener]

On Fri, 29 Apr 2022, Jakub Jelinek wrote:

> On Fri, Apr 29, 2022 at 11:32:15AM +0200, Richard Biener wrote:
> > I think that's reasonable (we indeed shouldn't create a varpool node
> > here).  I do think that we eventually want to retain removed nodes
> > but mark them so.  In fact any debug references will be thrown away
> > because of this anyway.
> > 
> > So I wonder if we can instead simply do if (!x_node) return 0;?
> 
> I had that in my first version, but after finding out that it triggers
> so often for the constant pool decls I thought better to just use
> x_decl in that case instead of x_node->decl.
> I must say I'm unsure if constant pool decls always stay out of section
> anchors or if they can be put there too.
> 
> > The question is also why sched does any queries for debug-insns,
> > does it merely reset them based on the answer?  That said,
> > it would be nice to be able to assert that x_node is not NULL
> > and catch this in the callers somehow.
> 
> Unfortunately, several layers of callers don't really know it is for debug
> insn.  And the touched code is solely for the section anchors, so e.g. just
> checking symtab_node::get (decl) on all mentioned decls when we perhaps can
> see if it is debug insn or not would be quite costly and we wouldn't know
> if the other reference is anchored.

We might want to reset debug stmts at the time we RTL expand them
if referred symbols have no cgraph node?  As said, ->get () instead
of ->get_create () is obviously OK but the way we deal with the fallout
is a bit suspicious there IMHO.

Richard.

Re: [PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Jakub Jelinek]

[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]

On Fri, Apr 29, 2022 at 11:52:37AM +0200, Richard Biener wrote:
> On Fri, 29 Apr 2022, Jakub Jelinek wrote:
> 
> > On Fri, Apr 29, 2022 at 11:32:15AM +0200, Richard Biener wrote:
> > > I think that's reasonable (we indeed shouldn't create a varpool node
> > > here).  I do think that we eventually want to retain removed nodes
> > > but mark them so.  In fact any debug references will be thrown away
> > > because of this anyway.
> > > 
> > > So I wonder if we can instead simply do if (!x_node) return 0;?
> > 
> > I had that in my first version, but after finding out that it triggers
> > so often for the constant pool decls I thought better to just use
> > x_decl in that case instead of x_node->decl.
> > I must say I'm unsure if constant pool decls always stay out of section
> > anchors or if they can be put there too.
> > 
> > > The question is also why sched does any queries for debug-insns,
> > > does it merely reset them based on the answer?  That said,
> > > it would be nice to be able to assert that x_node is not NULL
> > > and catch this in the callers somehow.
> > 
> > Unfortunately, several layers of callers don't really know it is for debug
> > insn.  And the touched code is solely for the section anchors, so e.g. just
> > checking symtab_node::get (decl) on all mentioned decls when we perhaps can
> > see if it is debug insn or not would be quite costly and we wouldn't know
> > if the other reference is anchored.
> 
> We might want to reset debug stmts at the time we RTL expand them
> if referred symbols have no cgraph node?  As said, ->get () instead
> of ->get_create () is obviously OK but the way we deal with the fallout
> is a bit suspicious there IMHO.

So, what about doing that if (!x_node) return 0; in alias.c with the
exception of DECL_IN_CONSTANT_POOL, plus in cfgexpand.cc throw away
VAR_DECLs without symtab node?

I'll need to do some extra checking on whether we don't really lose any
useful debug info with the second patch.

	Jakub

[-- Attachment #2: R468 --]
[-- Type: text/plain, Size: 1803 bytes --]

2022-04-29  Jakub Jelinek  <jakub@redhat.com>

	PR debug/105415
	* alias.cc (compare_base_symbol_refs): Avoid creating new symtab
	nodes, if it doesn't exist, punt.  For DECL_IN_CONSTANT_POOL decls
	use x_decl instead of x_node->decl.

	* gcc.dg/pr105415.c: New test.

--- gcc/alias.cc.jj	2022-02-21 16:51:50.261232505 +0100
+++ gcc/alias.cc	2022-04-29 12:25:02.022392689 +0200
@@ -2219,12 +2219,19 @@ compare_base_symbol_refs (const_rtx x_ba
 	  || (!TREE_STATIC (x_decl) && !TREE_PUBLIC (x_decl)))
 	return 0;
 
-      symtab_node *x_node = symtab_node::get_create (x_decl)
-			    ->ultimate_alias_target ();
-      /* External variable cannot be in section anchor.  */
-      if (!x_node->definition)
-	return 0;
-      x_base = XEXP (DECL_RTL (x_node->decl), 0);
+      tree x_decl2 = x_decl;
+      if (!DECL_IN_CONSTANT_POOL (x_decl))
+	{
+	  symtab_node *x_node = symtab_node::get (x_decl);
+	  if (!x_node)
+	    return 0;
+	  x_node = x_node->ultimate_alias_target ();
+	  /* External variable cannot be in section anchor.  */
+	  if (!x_node->definition)
+	    return 0;
+	  x_decl2 = x_node->decl;
+	}
+      x_base = XEXP (DECL_RTL (x_decl2), 0);
       /* If not in anchor, we can disambiguate.  */
       if (!SYMBOL_REF_HAS_BLOCK_INFO_P (x_base))
 	return 0;
--- gcc/testsuite/gcc.dg/pr105415.c.jj	2022-04-28 12:26:13.174302870 +0200
+++ gcc/testsuite/gcc.dg/pr105415.c	2022-04-28 12:25:36.770809149 +0200
@@ -0,0 +1,26 @@
+/* PR debug/105415 */
+/* { dg-do compile } */
+/* { dg-require-effective-target pthread } */
+/* { dg-options "-O2 -ftree-parallelize-loops=2 -fcompare-debug" } */
+
+int m;
+static int n;
+
+void
+foo (void)
+{
+  int s = 0;
+
+  while (m < 1)
+    {
+      s += n;
+      ++m;
+    }
+}
+
+void
+bar (int *arr, int i)
+{
+  while (i < 1)
+    arr[i++] = 1;
+}

[-- Attachment #3: R469 --]
[-- Type: text/plain, Size: 594 bytes --]

2022-04-29  Jakub Jelinek  <jakub@redhat.com>

	* cfgexpand.cc (expand_debug_expr): Don't make_decl_rtl_for_debug
	if there is no symtab node for the VAR_DECL.

--- gcc/cfgexpand.cc.jj	2022-03-09 19:54:17.112284770 +0100
+++ gcc/cfgexpand.cc	2022-04-29 12:33:32.585363999 +0200
@@ -4565,7 +4565,8 @@ expand_debug_expr (tree exp)
 	      || !DECL_NAME (exp)
 	      || DECL_HARD_REGISTER (exp)
 	      || DECL_IN_CONSTANT_POOL (exp)
-	      || mode == VOIDmode)
+	      || mode == VOIDmode
+	      || symtab_node::get (exp) == NULL)
 	    return NULL;
 
 	  op0 = make_decl_rtl_for_debug (exp);

Re: [PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Richard Biener]

On Fri, 29 Apr 2022, Jakub Jelinek wrote:

> On Fri, Apr 29, 2022 at 11:52:37AM +0200, Richard Biener wrote:
> > On Fri, 29 Apr 2022, Jakub Jelinek wrote:
> > 
> > > On Fri, Apr 29, 2022 at 11:32:15AM +0200, Richard Biener wrote:
> > > > I think that's reasonable (we indeed shouldn't create a varpool node
> > > > here).  I do think that we eventually want to retain removed nodes
> > > > but mark them so.  In fact any debug references will be thrown away
> > > > because of this anyway.
> > > > 
> > > > So I wonder if we can instead simply do if (!x_node) return 0;?
> > > 
> > > I had that in my first version, but after finding out that it triggers
> > > so often for the constant pool decls I thought better to just use
> > > x_decl in that case instead of x_node->decl.
> > > I must say I'm unsure if constant pool decls always stay out of section
> > > anchors or if they can be put there too.
> > > 
> > > > The question is also why sched does any queries for debug-insns,
> > > > does it merely reset them based on the answer?  That said,
> > > > it would be nice to be able to assert that x_node is not NULL
> > > > and catch this in the callers somehow.
> > > 
> > > Unfortunately, several layers of callers don't really know it is for debug
> > > insn.  And the touched code is solely for the section anchors, so e.g. just
> > > checking symtab_node::get (decl) on all mentioned decls when we perhaps can
> > > see if it is debug insn or not would be quite costly and we wouldn't know
> > > if the other reference is anchored.
> > 
> > We might want to reset debug stmts at the time we RTL expand them
> > if referred symbols have no cgraph node?  As said, ->get () instead
> > of ->get_create () is obviously OK but the way we deal with the fallout
> > is a bit suspicious there IMHO.
> 
> So, what about doing that if (!x_node) return 0; in alias.c with the
> exception of DECL_IN_CONSTANT_POOL, plus in cfgexpand.cc throw away
> VAR_DECLs without symtab node?

So we don't have varpool nodes for the constant pool decls?  Are they
CONST_DECLs at least?  I think that's reasonable though ideally we'd
be able to assert that there is a symtab node for the decls ...

> I'll need to do some extra checking on whether we don't really lose any
> useful debug info with the second patch.

At least it was surprisingly simple ;)

Richard.

Re: [PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Jakub Jelinek]

On Fri, Apr 29, 2022 at 12:58:12PM +0200, Richard Biener wrote:
> So we don't have varpool nodes for the constant pool decls?  Are they

Depends.  DECL_IN_CONSTANT_POOL decls can appear 2 ways, through
tree_output_constant_def which does create a varpool node for them
and is generally invoked during GIMPLE passes or so, and using
output_constant_def, which is called during expansion or later and doesn't
have varpool nodes created unless say alias.cc creates those for them.

> CONST_DECLs at least?  I think that's reasonable though ideally we'd

No, they are VAR_DECLs, mostly created as something to refer to in
SYMBOL_REFs created for the constant pool constants.
varasm.cc (build_constant_desc) creates them.
Changing those from VAR_DECLs to CONST_DECLs could be a lot of work.

Given that tree_output_constant_def created DECL_IN_CONSTANT_POOL nodes
do have varpool nodes, it might be better to do get first, if it returns
NULL return 0; for !DECL_IN_CONSTANT_POOL otherwise use x_decl2 = x_decl.

> be able to assert that there is a symtab node for the decls ...
> 
> > I'll need to do some extra checking on whether we don't really lose any
> > useful debug info with the second patch.
> 
> At least it was surprisingly simple ;)

	Jakub

Re: [PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Jakub Jelinek]

On Fri, Apr 29, 2022 at 01:11:31PM +0200, Jakub Jelinek via Gcc-patches wrote:
> Depends.  DECL_IN_CONSTANT_POOL decls can appear 2 ways, through
> tree_output_constant_def which does create a varpool node for them
> and is generally invoked during GIMPLE passes or so, and using
> output_constant_def, which is called during expansion or later and doesn't
> have varpool nodes created unless say alias.cc creates those for them.

Oh, and one thing I forgot.  The constant pool decls can be put into section
anchors, so it is essential that we handle DECL_IN_CONSTANT_POOL decls
there and don't just punt on those.

	Jakub

Re: [PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Richard Biener]

On Fri, 29 Apr 2022, Jakub Jelinek wrote:

> On Fri, Apr 29, 2022 at 12:58:12PM +0200, Richard Biener wrote:
> > So we don't have varpool nodes for the constant pool decls?  Are they
> 
> Depends.  DECL_IN_CONSTANT_POOL decls can appear 2 ways, through
> tree_output_constant_def which does create a varpool node for them
> and is generally invoked during GIMPLE passes or so, and using
> output_constant_def, which is called during expansion or later and doesn't
> have varpool nodes created unless say alias.cc creates those for them.
> 
> > CONST_DECLs at least?  I think that's reasonable though ideally we'd
> 
> No, they are VAR_DECLs, mostly created as something to refer to in
> SYMBOL_REFs created for the constant pool constants.
> varasm.cc (build_constant_desc) creates them.
> Changing those from VAR_DECLs to CONST_DECLs could be a lot of work.
> 
> Given that tree_output_constant_def created DECL_IN_CONSTANT_POOL nodes
> do have varpool nodes, it might be better to do get first, if it returns
> NULL return 0; for !DECL_IN_CONSTANT_POOL otherwise use x_decl2 = x_decl.

Yeah, maybe.  That said, the cfgexpand.cc part looks fine to me and
probably should fix the compare-debug issue in the PR on its own?

Richard.

> > be able to assert that there is a symtab node for the decls ...
> > 
> > > I'll need to do some extra checking on whether we don't really lose any
> > > useful debug info with the second patch.
> > 
> > At least it was surprisingly simple ;)
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)

Re: [PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Richard Biener]

On Fri, 29 Apr 2022, Jakub Jelinek wrote:

> On Fri, Apr 29, 2022 at 01:11:31PM +0200, Jakub Jelinek via Gcc-patches wrote:
> > Depends.  DECL_IN_CONSTANT_POOL decls can appear 2 ways, through
> > tree_output_constant_def which does create a varpool node for them
> > and is generally invoked during GIMPLE passes or so, and using
> > output_constant_def, which is called during expansion or later and doesn't
> > have varpool nodes created unless say alias.cc creates those for them.
> 
> Oh, and one thing I forgot.  The constant pool decls can be put into section
> anchors, so it is essential that we handle DECL_IN_CONSTANT_POOL decls
> there and don't just punt on those.

Ah, OK - that makes sense (maybe we should create varpool nodes at the
point we associate them with anchors, or alternatively use the varpool
node of the anchor?).

Richard.

Re: [PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Jakub Jelinek]

On Fri, Apr 29, 2022 at 01:23:46PM +0200, Richard Biener wrote:
> On Fri, 29 Apr 2022, Jakub Jelinek wrote:
> 
> > On Fri, Apr 29, 2022 at 01:11:31PM +0200, Jakub Jelinek via Gcc-patches wrote:
> > > Depends.  DECL_IN_CONSTANT_POOL decls can appear 2 ways, through
> > > tree_output_constant_def which does create a varpool node for them
> > > and is generally invoked during GIMPLE passes or so, and using
> > > output_constant_def, which is called during expansion or later and doesn't
> > > have varpool nodes created unless say alias.cc creates those for them.
> > 
> > Oh, and one thing I forgot.  The constant pool decls can be put into section
> > anchors, so it is essential that we handle DECL_IN_CONSTANT_POOL decls
> > there and don't just punt on those.
> 
> Ah, OK - that makes sense (maybe we should create varpool nodes at the
> point we associate them with anchors, or alternatively use the varpool
> node of the anchor?).

I've bootstrapped/regtested the following on x86_64-linux and i686-linux,
then bootstrapped with the patch reverted, reapplied the patch and did make
cc1plus in stage3.  The debug section sizes are identical, .debug_info and
.debug_loc is identical too, so I think we don't lose any debug info through
it.

Ok for trunk?

2022-05-02  Jakub Jelinek  <jakub@redhat.com>

	PR debug/105415
	* cfgexpand.cc (expand_debug_expr): Don't make_decl_rtl_for_debug
	if there is no symtab node for the VAR_DECL.

	* gcc.dg/pr105415.c: New test.

--- gcc/cfgexpand.cc.jj	2022-03-09 19:54:17.112284770 +0100
+++ gcc/cfgexpand.cc	2022-04-29 12:33:32.585363999 +0200
@@ -4565,7 +4565,8 @@ expand_debug_expr (tree exp)
 	      || !DECL_NAME (exp)
 	      || DECL_HARD_REGISTER (exp)
 	      || DECL_IN_CONSTANT_POOL (exp)
-	      || mode == VOIDmode)
+	      || mode == VOIDmode
+	      || symtab_node::get (exp) == NULL)
 	    return NULL;
 
 	  op0 = make_decl_rtl_for_debug (exp);
--- gcc/testsuite/gcc.dg/pr105415.c.jj	2022-04-28 12:26:13.174302870 +0200
+++ gcc/testsuite/gcc.dg/pr105415.c	2022-04-28 12:25:36.770809149 +0200
@@ -0,0 +1,26 @@
+/* PR debug/105415 */
+/* { dg-do compile } */
+/* { dg-require-effective-target pthread } */
+/* { dg-options "-O2 -ftree-parallelize-loops=2 -fcompare-debug" } */
+
+int m;
+static int n;
+
+void
+foo (void)
+{
+  int s = 0;
+
+  while (m < 1)
+    {
+      s += n;
+      ++m;
+    }
+}
+
+void
+bar (int *arr, int i)
+{
+  while (i < 1)
+    arr[i++] = 1;
+}


	Jakub

Re: [PATCH] alias: Fix -fcompare-debug issues caused by compare_base_symbol_refs [PR105415]

[Richard Biener]

On Mon, 2 May 2022, Jakub Jelinek wrote:

> On Fri, Apr 29, 2022 at 01:23:46PM +0200, Richard Biener wrote:
> > On Fri, 29 Apr 2022, Jakub Jelinek wrote:
> > 
> > > On Fri, Apr 29, 2022 at 01:11:31PM +0200, Jakub Jelinek via Gcc-patches wrote:
> > > > Depends.  DECL_IN_CONSTANT_POOL decls can appear 2 ways, through
> > > > tree_output_constant_def which does create a varpool node for them
> > > > and is generally invoked during GIMPLE passes or so, and using
> > > > output_constant_def, which is called during expansion or later and doesn't
> > > > have varpool nodes created unless say alias.cc creates those for them.
> > > 
> > > Oh, and one thing I forgot.  The constant pool decls can be put into section
> > > anchors, so it is essential that we handle DECL_IN_CONSTANT_POOL decls
> > > there and don't just punt on those.
> > 
> > Ah, OK - that makes sense (maybe we should create varpool nodes at the
> > point we associate them with anchors, or alternatively use the varpool
> > node of the anchor?).
> 
> I've bootstrapped/regtested the following on x86_64-linux and i686-linux,
> then bootstrapped with the patch reverted, reapplied the patch and did make
> cc1plus in stage3.  The debug section sizes are identical, .debug_info and
> .debug_loc is identical too, so I think we don't lose any debug info through
> it.
> 
> Ok for trunk?

OK.

Richard.

> 2022-05-02  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/105415
> 	* cfgexpand.cc (expand_debug_expr): Don't make_decl_rtl_for_debug
> 	if there is no symtab node for the VAR_DECL.
> 
> 	* gcc.dg/pr105415.c: New test.
> 
> --- gcc/cfgexpand.cc.jj	2022-03-09 19:54:17.112284770 +0100
> +++ gcc/cfgexpand.cc	2022-04-29 12:33:32.585363999 +0200
> @@ -4565,7 +4565,8 @@ expand_debug_expr (tree exp)
>  	      || !DECL_NAME (exp)
>  	      || DECL_HARD_REGISTER (exp)
>  	      || DECL_IN_CONSTANT_POOL (exp)
> -	      || mode == VOIDmode)
> +	      || mode == VOIDmode
> +	      || symtab_node::get (exp) == NULL)
>  	    return NULL;
>  
>  	  op0 = make_decl_rtl_for_debug (exp);
> --- gcc/testsuite/gcc.dg/pr105415.c.jj	2022-04-28 12:26:13.174302870 +0200
> +++ gcc/testsuite/gcc.dg/pr105415.c	2022-04-28 12:25:36.770809149 +0200
> @@ -0,0 +1,26 @@
> +/* PR debug/105415 */
> +/* { dg-do compile } */
> +/* { dg-require-effective-target pthread } */
> +/* { dg-options "-O2 -ftree-parallelize-loops=2 -fcompare-debug" } */
> +
> +int m;
> +static int n;
> +
> +void
> +foo (void)
> +{
> +  int s = 0;
> +
> +  while (m < 1)
> +    {
> +      s += n;
> +      ++m;
> +    }
> +}
> +
> +void
> +bar (int *arr, int i)
> +{
> +  while (i < 1)
> +    arr[i++] = 1;
> +}
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)