Re: [PATCH] Fix -fcompare-debug due to DEBUG_BEGIN_STMTs (PR debug/83419)

[Alexandre Oliva <aoliva at redhat dot com>]

On Dec 15, 2017, Richard Biener <rguenther@suse.de> wrote:

> On Thu, 14 Dec 2017, Jakub Jelinek wrote:
>> Hi!
>> 
>> The following testcase FAILs -fcompare-debug, because one COND_EXPR
>> branch from the FE during gimplifications is just <nop_expr <integer_cst>>
>> which doesn't have TREE_SIDE_EFFECTS, but for -gstatement-frontiers it
>> is a STATEMENT_LIST which contains # DEBUG BEGIN_STMT and that <nop_expr
>> <integer_cst>>.

> Ugh...  the issue is that this difference might have many other
> -fcompare-debug issues, like when folding things?

I fixed a number of -fcompare-debug issues related with
TREE_SIDE_EFFECTS in STATEMENT_LISTs during the development of SFN.
It's not too hard, really: most surviving statement lists end up with
TREE_SIDE_EFFECTS set.

This case, that I had not caught, was one in which the non-debug case
actually discards the STATEMENT_LIST (that had TREE_SIDE_EFFECTS set),
and just uses a naked expr (that does NOT have TREE_SIDE_EFFECTS).  It
wasn't too hard to detect and fix this case, but of course there might
be others still lurking: that's why we have -fcompare-debug, and every
now and then some new case pops up.  Some are new regressions, but
others were just latent issues that hadn't been noticed.

> Why is it a STATEMENT_LIST rather than a COMPOUND_EXPR?

Since we introduce the begin stmt marker in the existing statement list
long before we even start parsing the corresponding statement, using a
stand-alone statement made sense to me.  I did not even consider using
COMPOUND_EXPRs.

I suspect, however, that this could cause more complications, for it
would hide any specific forms that might be searched for within the
COMPOUND_EXPRs.  Do you not think so?  I guess it wouldn't be too hard
to adjust the same spot I'm touching in this patch so as to turn begin
stmt markers + stmt into COMPOUND_EXPRs.

> like adding a TREE_SIDE_EFFECTS_VALID flag on STATEMENT_LIST,
> keeping TREE_SIDE_EFFECTS up-to-date when easily possible and when doing
> the expensive thing cache the result.

I experimented a bit with that yesterday.  Surprisingly, just deferring
the computation of TREE_SIDE_EFFECTS for STATEMENT_LISTs caused trouble:
pop_stmt_list expects STATEMENT_LISTs to have TREE_SIDE_EFFECTs set
unless they're empty lists.  Suspecting there might be other such issues
lurking, I decided to go back to the strategy I used for earlier
occurrences of such bugs, namely to get the behavior to match as closely
as possible what we'd get if debug stmts weren't there.  It didn't take
me long to get a fix this way.

> Is it really necessary to introduce this IL difference so early and in
> such an intrusive way?

The most reasonable way to introduce markers at statement boundaries is
to have the parser do so.  I don't see why this should be such a big
deal, though; every time I introduce such IL debug-only changes, it took
me significant effort to approach the state in which nearly all of the
code behaves in the same way regardless of the debug-only stuff.  That
things work reasonably flawlessly now is not suggestive that it was
easy, or not too intrusive, but rather that the work to make it seamless
despite the intrusiveness was done, at first, and then over time.
That's the reason for -fcompare-debug and the various bootstrap options
that stress it further.

> Can't we avoid adding # DEBUG BEGIN_STMT when there's not already
> a STATEMENT_LIST around for example?

We could, but that would drop most of the begin_stmt markers, or none.
A STATEMENT_LIST is always there, ready to get stmts, and after parsing
a statement we often extract it from the statement list containing it,
and throw the list away.

IMHO the way these markers are being introduced is just fine, it will
just take us (me?) a bit more work to shake out the few remaining bugs,
just like it did when VTA was introduced.  A lot of work was done before
the patch was submitted to this end, but a number of problems only
showed up later, on other platforms or under different combinations of
optimization flags.  There's no reason to expect it to be different this
time.



The patch below fixes the PR83419 bug.  I've bootstrapped it on x86_64-,
i686-, ppc64-, ppc64le-, and aarch64-linux-gnu, with -fcompare-debug in
all of stage3.  sparc64-linux-gnu ran into -fcompare-debug failures
early in stage3, the same I ran into before, and that I'll investigate
next.

Ok to install?


[SFN] propagate single-nondebug-stmt's side effects to enclosing list

Statements without side effects, preceded by debug begin stmt markers,
would become a statement list with side effects, although the stmt on
its own would be extracted from the list and remain not having side
effects.  This causes debug info and possibly codegen differences.
This patch fixes it, identifying the situation in which the stmt would
have been extracted from the stmt list, and propagating the side
effects flag from the stmt to the list.

for  gcc/ChangeLog

	PR debug/83419
	* c-family/c-semantics.c (pop_stmt_list): Propagate side
	effects from single nondebug stmt to container list.

for  gcc/testsuite/ChangeLog

	PR debug/83419
	* gcc.dg/pr83419.c: New.
---
 gcc/c-family/c-semantics.c     |    9 +++++++++
 gcc/testsuite/gcc.dg/pr83419.c |   16 ++++++++++++++++
 2 files changed, 25 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr83419.c

diff --git a/gcc/c-family/c-semantics.c b/gcc/c-family/c-semantics.c
index cd872d8ac740..3a972c32c8ea 100644
--- a/gcc/c-family/c-semantics.c
+++ b/gcc/c-family/c-semantics.c
@@ -96,6 +96,15 @@ pop_stmt_list (tree t)
 	      t = l;
 	      tsi_link_before (&i, u, TSI_SAME_STMT);
 	    }
+	  while (!tsi_end_p (i)
+		 && TREE_CODE (tsi_stmt (i)) == DEBUG_BEGIN_STMT)
+	    tsi_next (&i);
+	  /* If there's only one nondebug stmt in the list, we'd have
+	     extracted the stmt and dropped the list, and we'd take
+	     TREE_SIDE_EFFECTS from that statement, so keep the list's
+	     TREE_SIDE_EFFECTS in sync.  */
+	  if (tsi_one_before_end_p (i))
+	    TREE_SIDE_EFFECTS (t) = TREE_SIDE_EFFECTS (tsi_stmt (i));
 	}
     }
 
diff --git a/gcc/testsuite/gcc.dg/pr83419.c b/gcc/testsuite/gcc.dg/pr83419.c
new file mode 100644
index 000000000000..3d4f1d277011
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr83419.c
@@ -0,0 +1,16 @@
+/* PR debug/83419 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fcompare-debug" } */
+
+int a, b;
+void foo (int, ...);
+
+void
+bar (void)
+{
+  if (a || 1 == b)
+    foo (1);
+  else
+    0;
+  foo (1, 0);
+}


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer