On 12/20/2017 05:02 PM, Alexandre Oliva wrote:
> On Dec 15, 2017, Richard Biener <rguent...@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.
OK.
jeff
