Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Mon, Mar 30, 2020 at 01:09:41PM +0200, Richard Biener wrote: > Btw, does the above fallout already happen if you add -g? Because > all the affected stmt-lists should end up with some DEBUG_BEGIN_STMTs > in them and thus preserved? Such STATEMENT_LISTs were marked !TREE_SIDE_EFFECTS, which means e.g. save_expr doesn't save those etc. Jakub
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Mon, 30 Mar 2020, Jakub Jelinek wrote:
> On Mon, Mar 30, 2020 at 12:44:40PM +0200, Richard Biener wrote:
> > >type = TREE_TYPE (stmt_expr);
> > > + /* For statement-expressions, force the STATEMENT_LIST
> > > + to be preserved with side-effects, even if it contains
> > > + just one statement or no statements. See PR93786. */
> > > + TREE_SIDE_EFFECTS (stmt_expr) = 0;
> > >result = pop_stmt_list (stmt_expr);
> > > + gcc_assert (result == stmt_expr);
> > > + TREE_SIDE_EFFECTS (result) = 1;
> >
> > The original code had !TREE_SIDE_EFFECT stmt lists (empty ones?) returned
> > directly which you above change to have side-effects - are you sure
> > the fallout is not due to that? Did you look into any of the fallout?
>
> Some of the fallout has been from missed optimizations e.g. in initializers,
> where the TREE_SIDE_EFFECTs (sometimes for empty statement exprs like
> ({ ; ; ; }), at other times for one with a single real statement in there
> which didn't have side-effects) is with the patch now set when it wasn't
> before, but there were ICEs too.
I see. I was thinking of
if (TREE_SIDE_EFFECTS (stmt_expr))
{
TREE_SIDE_EFFECTS (stmt_expr) = 0;
result = pop_stmt_list (stmt_expr);
gcc_assert (result == stmt_expr);
TREE_SIDE_EFFECTS (result) = 1;
}
else
result = pop_stmt_list (stmt_expr);
which should make ({ ; ; ; }) work again, but obviously not the
single-stmt case.
> I guess another option is what Alex wrote, just throw the DEBUG_BEGIN_STMTs
> away if there would be just those or those plus a single other statement,
> perhaps limited to statement expressions (i.e. do it in the above spot).
Sure, that's another option - but of course only short-term since we
then end up with "bad" debug info.
Btw, does the above fallout already happen if you add -g? Because
all the affected stmt-lists should end up with some DEBUG_BEGIN_STMTs
in them and thus preserved?
Richard.
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Mon, Mar 30, 2020 at 12:44:40PM +0200, Richard Biener wrote:
> >type = TREE_TYPE (stmt_expr);
> > + /* For statement-expressions, force the STATEMENT_LIST
> > + to be preserved with side-effects, even if it contains
> > + just one statement or no statements. See PR93786. */
> > + TREE_SIDE_EFFECTS (stmt_expr) = 0;
> >result = pop_stmt_list (stmt_expr);
> > + gcc_assert (result == stmt_expr);
> > + TREE_SIDE_EFFECTS (result) = 1;
>
> The original code had !TREE_SIDE_EFFECT stmt lists (empty ones?) returned
> directly which you above change to have side-effects - are you sure
> the fallout is not due to that? Did you look into any of the fallout?
Some of the fallout has been from missed optimizations e.g. in initializers,
where the TREE_SIDE_EFFECTs (sometimes for empty statement exprs like
({ ; ; ; }), at other times for one with a single real statement in there
which didn't have side-effects) is with the patch now set when it wasn't
before, but there were ICEs too.
I guess another option is what Alex wrote, just throw the DEBUG_BEGIN_STMTs
away if there would be just those or those plus a single other statement,
perhaps limited to statement expressions (i.e. do it in the above spot).
Jakub
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Mon, 30 Mar 2020, Jakub Jelinek wrote: > On Fri, Mar 27, 2020 at 09:09:42PM +0100, Richard Biener wrote: > > Sounds worth a try. > > Unfortunately that FAILed miserably: > +FAIL: g++.dg/cpp0x/constexpr-object1.C -std=c++11 (test for excess errors) > +FAIL: g++.dg/cpp0x/constexpr-object1.C -std=c++14 (test for excess errors) > +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++11 scan-assembler rodata > +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++14 scan-assembler rodata > +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++17 scan-assembler rodata > +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++2a scan-assembler rodata > +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++11 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++14 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++17 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++2a scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++11 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++14 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++17 scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++2a scan-assembler-not > static_initialization > +FAIL: g++.dg/cpp0x/pr78765.C -std=c++11 (test for excess errors) > +FAIL: g++.dg/cpp0x/pr78765.C -std=c++14 (test for excess errors) > +FAIL: g++.dg/cpp0x/pr78765.C -std=c++17 (test for excess errors) > +FAIL: g++.dg/cpp0x/pr78765.C -std=c++2a (test for excess errors) > +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++14 scan-assembler-not > static_init > +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++17 scan-assembler-not > static_init > +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++2a scan-assembler-not > static_init > +FAIL: g++.dg/cpp1z/decomp3.C (test for excess errors) > +FAIL: g++.dg/cpp1z/elide2.C -std=c++11 execution test > +FAIL: g++.dg/cpp1z/elide2.C -std=c++14 execution test > +FAIL: g++.dg/cpp1z/elide2.C -std=c++17 execution test > +FAIL: g++.dg/cpp1z/elide2.C -std=c++2a execution test > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++11 (test for errors, line 6) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++11 (test for excess errors) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++14 (test for errors, line 6) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++14 (test for excess errors) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++17 (test for errors, line 6) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++17 (test for excess errors) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++2a (test for errors, line 6) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++2a (test for excess errors) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++98 (test for errors, line 6) > +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++98 (test for excess errors) > +FAIL: g++.dg/other/error19.C -std=c++11 (test for excess errors) > +FAIL: g++.dg/other/error19.C -std=c++14 (test for excess errors) > +FAIL: g++.dg/other/error19.C -std=c++17 (test for excess errors) > +FAIL: g++.dg/other/error19.C -std=c++2a (test for excess errors) > +FAIL: g++.dg/other/error19.C -std=c++98 (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++11 (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++11 17 (test for errors, line 6) > +FAIL: g++.dg/parse/error26.C -std=gnu++14 (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++14 17 (test for errors, line 6) > +FAIL: g++.dg/parse/error26.C -std=gnu++17 (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++17 17 (test for errors, line 6) > +FAIL: g++.dg/parse/error26.C -std=gnu++2a (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++2a 17 (test for errors, line 6) > +FAIL: g++.dg/parse/error26.C -std=gnu++98 (test for excess errors) > +FAIL: g++.dg/parse/error26.C -std=gnu++98 17 (test for errors, line 6) > +FAIL: g++.dg/tree-ssa/pr69336.C (test for excess errors) > +UNRESOLVED: g++.dg/tree-ssa/pr69336.C scan-tree-dump-not optimized "cmap" > +FAIL: g++.dg/torture/pr45709-2.C -O1 (internal compiler error) > +FAIL: g++.dg/torture/pr45709-2.C -O1 (test for excess errors) > +FAIL: g++.dg/torture/pr45709-2.C -O2 (internal compiler error) > +FAIL: g++.dg/torture/pr45709-2.C -O2 (test for excess errors) > +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto (internal compiler error) > +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto (test for excess errors) > +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto -flto-partition=none (internal > compiler error) > +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto -flto-partition=none (test for > excess errors) > +FAIL: g++.dg/torture/pr45709-2.C -O3 -g (internal compiler error) > +FAIL: g++.dg/torture/pr45709-2.C -O3 -g (test for excess errors) > +FAIL:
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Fri, Mar 27, 2020 at 09:09:42PM +0100, Richard Biener wrote: > Sounds worth a try. Unfortunately that FAILed miserably: +FAIL: g++.dg/cpp0x/constexpr-object1.C -std=c++11 (test for excess errors) +FAIL: g++.dg/cpp0x/constexpr-object1.C -std=c++14 (test for excess errors) +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++11 scan-assembler rodata +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++14 scan-assembler rodata +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++17 scan-assembler rodata +FAIL: g++.dg/cpp0x/constexpr-rom.C -std=c++2a scan-assembler rodata +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++11 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++14 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++17 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static.C -std=gnu++2a scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++11 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++14 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++17 scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/constexpr-static3.C -std=gnu++2a scan-assembler-not static_initialization +FAIL: g++.dg/cpp0x/pr78765.C -std=c++11 (test for excess errors) +FAIL: g++.dg/cpp0x/pr78765.C -std=c++14 (test for excess errors) +FAIL: g++.dg/cpp0x/pr78765.C -std=c++17 (test for excess errors) +FAIL: g++.dg/cpp0x/pr78765.C -std=c++2a (test for excess errors) +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++14 scan-assembler-not static_init +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++17 scan-assembler-not static_init +FAIL: g++.dg/cpp1y/constexpr-empty3.C -std=c++2a scan-assembler-not static_init +FAIL: g++.dg/cpp1z/decomp3.C (test for excess errors) +FAIL: g++.dg/cpp1z/elide2.C -std=c++11 execution test +FAIL: g++.dg/cpp1z/elide2.C -std=c++14 execution test +FAIL: g++.dg/cpp1z/elide2.C -std=c++17 execution test +FAIL: g++.dg/cpp1z/elide2.C -std=c++2a execution test +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++11 (test for errors, line 6) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++11 (test for excess errors) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++14 (test for errors, line 6) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++14 (test for excess errors) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++17 (test for errors, line 6) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++17 (test for excess errors) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++2a (test for errors, line 6) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++2a (test for excess errors) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++98 (test for errors, line 6) +FAIL: g++.dg/ext/stmtexpr15.C -std=gnu++98 (test for excess errors) +FAIL: g++.dg/other/error19.C -std=c++11 (test for excess errors) +FAIL: g++.dg/other/error19.C -std=c++14 (test for excess errors) +FAIL: g++.dg/other/error19.C -std=c++17 (test for excess errors) +FAIL: g++.dg/other/error19.C -std=c++2a (test for excess errors) +FAIL: g++.dg/other/error19.C -std=c++98 (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++11 (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++11 17 (test for errors, line 6) +FAIL: g++.dg/parse/error26.C -std=gnu++14 (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++14 17 (test for errors, line 6) +FAIL: g++.dg/parse/error26.C -std=gnu++17 (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++17 17 (test for errors, line 6) +FAIL: g++.dg/parse/error26.C -std=gnu++2a (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++2a 17 (test for errors, line 6) +FAIL: g++.dg/parse/error26.C -std=gnu++98 (test for excess errors) +FAIL: g++.dg/parse/error26.C -std=gnu++98 17 (test for errors, line 6) +FAIL: g++.dg/tree-ssa/pr69336.C (test for excess errors) +UNRESOLVED: g++.dg/tree-ssa/pr69336.C scan-tree-dump-not optimized "cmap" +FAIL: g++.dg/torture/pr45709-2.C -O1 (internal compiler error) +FAIL: g++.dg/torture/pr45709-2.C -O1 (test for excess errors) +FAIL: g++.dg/torture/pr45709-2.C -O2 (internal compiler error) +FAIL: g++.dg/torture/pr45709-2.C -O2 (test for excess errors) +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto (internal compiler error) +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto (test for excess errors) +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto -flto-partition=none (internal compiler error) +FAIL: g++.dg/torture/pr45709-2.C -O2 -flto -flto-partition=none (test for excess errors) +FAIL: g++.dg/torture/pr45709-2.C -O3 -g (internal compiler error) +FAIL: g++.dg/torture/pr45709-2.C -O3 -g (test for excess errors) +FAIL: g++.dg/torture/pr45709.C -O1 (internal compiler error) +FAIL: g++.dg/torture/pr45709.C -O1 (test for excess errors) +FAIL: g++.dg/torture/pr45709.C -O2 (internal compiler error) +FAIL: g++.dg/torture/pr45709.C -O2
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On March 27, 2020 4:32:58 PM GMT+01:00, Jakub Jelinek wrote: >On Fri, Mar 27, 2020 at 10:57:05AM +0100, Richard Biener wrote: >> > That would just mean it ICEs at -g0 too, this PR isn't a >-fcompare-debug >> > issue but a bug where we try to gimplify the same STATEMENT_LIST >multiple >> > times and second time ICE on it as it has been voidified and >emptied. >> >> But only because we made STATEMENT_LISTs not having side-effects >which >> they all did before all the changes. We could revert that... > >So perhaps we could try to do that only for the C/C++ statement >expressions >which seems to be the most common reason of all these -fcompare-debug >-gstatement-frontiers related PRs, while it wouldn't consume too much >memory >in the common case? >I.e. when parsing a statement expression in the C/C++ FEs, ensure the >result >is wrapped in a STATEMENT_LIST with TREE_SIDE_EFFECTS set, no matter if >it >contains just a single stmt or more inside of it? Sounds worth a try. Richard. > Jakub
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Fri, Mar 27, 2020 at 10:57:05AM +0100, Richard Biener wrote: > > That would just mean it ICEs at -g0 too, this PR isn't a -fcompare-debug > > issue but a bug where we try to gimplify the same STATEMENT_LIST multiple > > times and second time ICE on it as it has been voidified and emptied. > > But only because we made STATEMENT_LISTs not having side-effects which > they all did before all the changes. We could revert that... So perhaps we could try to do that only for the C/C++ statement expressions which seems to be the most common reason of all these -fcompare-debug -gstatement-frontiers related PRs, while it wouldn't consume too much memory in the common case? I.e. when parsing a statement expression in the C/C++ FEs, ensure the result is wrapped in a STATEMENT_LIST with TREE_SIDE_EFFECTS set, no matter if it contains just a single stmt or more inside of it? Jakub
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Fri, 27 Mar 2020, Jakub Jelinek wrote: > On Fri, Mar 27, 2020 at 09:32:40AM +0100, Richard Biener wrote: > > On Fri, 27 Mar 2020, Jakub Jelinek wrote: > > > > > On Thu, Mar 26, 2020 at 08:41:41PM -0300, Alexandre Oliva wrote: > > > > On Mar 26, 2020, Jakub Jelinek wrote: > > > > > > > > > Or disable -gstatement-frontiers by default and declare it > > > > > -fcompare-debug > > > > > incompatible. > > > > > > > > I don't get what makes debug stmts introduced by -gstatement-frontiers > > > > special in this regard. I recall working a lot on making statement list > > > > management compatible with debug stmts for -fcompare-debug purposes back > > > > when introducing them, but not breaking new ground when extending them > > > > with markers rather than binds. > > > > > > The hardest issue I gave up completely on after trying 3 different > > > approaches is PR93786, I just don't know what to do in that case. > > > Gimplification of a STATEMENT_LIST is destructive, especially the > > > voidify_wrapper_expr part of it, and if the STATEMENT_LIST is used > > > multiple > > > times, that is a severe problem. > > > > The question for this case is wheter we can arrange for the STATEMENT_LIST > > to be present even for -g0 and "solve" the issue that way? > > That would just mean it ICEs at -g0 too, this PR isn't a -fcompare-debug > issue but a bug where we try to gimplify the same STATEMENT_LIST multiple > times and second time ICE on it as it has been voidified and emptied. But only because we made STATEMENT_LISTs not having side-effects which they all did before all the changes. We could revert that... Richard.
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Fri, Mar 27, 2020 at 09:32:40AM +0100, Richard Biener wrote: > On Fri, 27 Mar 2020, Jakub Jelinek wrote: > > > On Thu, Mar 26, 2020 at 08:41:41PM -0300, Alexandre Oliva wrote: > > > On Mar 26, 2020, Jakub Jelinek wrote: > > > > > > > Or disable -gstatement-frontiers by default and declare it > > > > -fcompare-debug > > > > incompatible. > > > > > > I don't get what makes debug stmts introduced by -gstatement-frontiers > > > special in this regard. I recall working a lot on making statement list > > > management compatible with debug stmts for -fcompare-debug purposes back > > > when introducing them, but not breaking new ground when extending them > > > with markers rather than binds. > > > > The hardest issue I gave up completely on after trying 3 different > > approaches is PR93786, I just don't know what to do in that case. > > Gimplification of a STATEMENT_LIST is destructive, especially the > > voidify_wrapper_expr part of it, and if the STATEMENT_LIST is used multiple > > times, that is a severe problem. > > The question for this case is wheter we can arrange for the STATEMENT_LIST > to be present even for -g0 and "solve" the issue that way? That would just mean it ICEs at -g0 too, this PR isn't a -fcompare-debug issue but a bug where we try to gimplify the same STATEMENT_LIST multiple times and second time ICE on it as it has been voidified and emptied. Jakub
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Fri, 27 Mar 2020, Jakub Jelinek wrote: > On Thu, Mar 26, 2020 at 08:41:41PM -0300, Alexandre Oliva wrote: > > On Mar 26, 2020, Jakub Jelinek wrote: > > > > > Or disable -gstatement-frontiers by default and declare it -fcompare-debug > > > incompatible. > > > > I don't get what makes debug stmts introduced by -gstatement-frontiers > > special in this regard. I recall working a lot on making statement list > > management compatible with debug stmts for -fcompare-debug purposes back > > when introducing them, but not breaking new ground when extending them > > with markers rather than binds. > > The hardest issue I gave up completely on after trying 3 different > approaches is PR93786, I just don't know what to do in that case. > Gimplification of a STATEMENT_LIST is destructive, especially the > voidify_wrapper_expr part of it, and if the STATEMENT_LIST is used multiple > times, that is a severe problem. The question for this case is wheter we can arrange for the STATEMENT_LIST to be present even for -g0 and "solve" the issue that way? > The PR94323 and PR94272 changes worked, but they are quite unclean and in > bugzilla there is at least one another -fcompare-debug issue that goes away > with -gno-statement-frontiers (PR94340). > > Jakub > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Mar 26, 2020, Jakub Jelinek wrote:
> The hardest issue I gave up completely on after trying 3 different
> approaches is PR93786, I just don't know what to do in that case.
struct S {virtual void v();};
void f(S * s) {
({ s; })->v();
}
Thanks, I can now see how markers create situations that were not
possible with binds.
The statement 's;' in the expression statement doesn't have any side
effects, but it will have a marker associated with it.
Without side effects, a statement wouldn't give rise to binds, so this
situation wouldn't arise.
Such a simple, side-effect-free statement wouldn't even have location
data associated with it, I suppose. We might have a wrapper to annotate
the expression with the location in which it is used, and we might
augment such a wrapper with pre- and post- debug stmts, but odds are
we'd eventually end up without means to carry the info any further.
This suggests two potential ways to address this problem:
- when gimplifying a STATEMENT_LIST without side effects, if it contains
a single nondebug statement, gimplify only that one stmt, discarding
any of the debug stmts that we can't hold on to.
- wherever we might have a sequence of statements, create a statement
list instead of working very hard in a premature (?) optimization that
makes it harder (impossible?) to retain debug info in cases we'd like
to. This would grow memory use a little during parsing, but since we
gimplify functions early on and that would address most of the
differences, and optimizers would take care of any unnecessary
SAVE_EXPRs, it might not matter much, and avoiding the current runtime
overheads in skipping debug stmts early on might more than make up for
that.
--
Alexandre Oliva, freedom fighterhe/himhttps://FSFLA.org/blogs/lxo/
Free Software Evangelist Stallman was right, but he's left :(
GNU Toolchain Engineer Live long and free, and prosper ethically
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Thu, Mar 26, 2020 at 08:41:41PM -0300, Alexandre Oliva wrote: > On Mar 26, 2020, Jakub Jelinek wrote: > > > Or disable -gstatement-frontiers by default and declare it -fcompare-debug > > incompatible. > > I don't get what makes debug stmts introduced by -gstatement-frontiers > special in this regard. I recall working a lot on making statement list > management compatible with debug stmts for -fcompare-debug purposes back > when introducing them, but not breaking new ground when extending them > with markers rather than binds. The hardest issue I gave up completely on after trying 3 different approaches is PR93786, I just don't know what to do in that case. Gimplification of a STATEMENT_LIST is destructive, especially the voidify_wrapper_expr part of it, and if the STATEMENT_LIST is used multiple times, that is a severe problem. The PR94323 and PR94272 changes worked, but they are quite unclean and in bugzilla there is at least one another -fcompare-debug issue that goes away with -gno-statement-frontiers (PR94340). Jakub
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Mar 26, 2020, Jakub Jelinek wrote: > Or disable -gstatement-frontiers by default and declare it -fcompare-debug > incompatible. I don't get what makes debug stmts introduced by -gstatement-frontiers special in this regard. I recall working a lot on making statement list management compatible with debug stmts for -fcompare-debug purposes back when introducing them, but not breaking new ground when extending them with markers rather than binds. > Or perhaps just a flag on the STATEMENT_LIST that would make it clear the > STATEMENT_LIST wouldn't be there without -g. Or different tree code like > STATEMENT_LIST, except that it would be only this kind of container. IIRC part of the problem in ensuring isomorphism for -fcompare-debug is that sometimes we end up with redundant STATEMENT_LISTs in the non-debug case, and then we can't tell whether a STATEMENT_LIST is to be significant or not in the debug case. A flag for STATEMENT_LIST, or maybe an alternate node type that amounted to an easier to recognize and manage transparent container for at most one nondebug stmt, and as many debug stmts as needed before and after it, might help avoid unintended differences. I'm not sure whether the separate node with direct access to the contained nondebug node would be advantageous. Maybe just recognizing this kind of node and turning it into a regular STATEMENT_LIST wherever we'd have created one would suffice. IIRC there are various operations of inserting stmts that search for nondebug stmts, and the direct link might help with them, if the flag alone wouldn't. -- Alexandre Oliva, freedom fighterhe/himhttps://FSFLA.org/blogs/lxo/ Free Software Evangelist Stallman was right, but he's left :( GNU Toolchain Engineer Live long and free, and prosper ethically
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Thu, 26 Mar 2020, Jakub Jelinek wrote: > Hi! > > On Thu, Mar 26, 2020 at 10:24:29AM +0100, Richard Biener wrote: > > On Thu, 26 Mar 2020, Jakub Jelinek wrote: > > > > > On Thu, Mar 26, 2020 at 10:14:35AM +0100, Richard Biener wrote: > > > > Ick. I fear we really need a better way of dealing with this > > > > STATEMENT_LIST appearance with only -g ... > > > > > > Yeah, it is very ugly and in some PRs I'm out of ideas what to do. > > > > I think a more sustainable "fix" would be to simply not emit > > the DEBUG_BEGIN_STMT when it would be the only reason to > > emit a STATEMENT_LIST ... > > > > Or always emit a STATEMENT_LIST node whenever there could be one > > (hopefully not every stmt could be a stmt list and hopefully > > we don't treat single-stmt STATEMENT_LIST specially). > > > > That is, the fix should be to avoid this difference in the first > > place, not try to deal with the difference later. > > Or perhaps just a flag on the STATEMENT_LIST that would make it clear the > STATEMENT_LIST wouldn't be there without -g. Or different tree code like > STATEMENT_LIST, except that it would be only this kind of container. But then you still need to deal with those so the fixups would look similar, it just shortens the checks a bit. > Or disable -gstatement-frontiers by default and declare it -fcompare-debug > incompatible. Do any debug info consumers consume this? I'm not aware of any. Richard. > In any case, I'd really like Alex' feedback on this. > > Jakub > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
Hi! On Thu, Mar 26, 2020 at 10:24:29AM +0100, Richard Biener wrote: > On Thu, 26 Mar 2020, Jakub Jelinek wrote: > > > On Thu, Mar 26, 2020 at 10:14:35AM +0100, Richard Biener wrote: > > > Ick. I fear we really need a better way of dealing with this > > > STATEMENT_LIST appearance with only -g ... > > > > Yeah, it is very ugly and in some PRs I'm out of ideas what to do. > > I think a more sustainable "fix" would be to simply not emit > the DEBUG_BEGIN_STMT when it would be the only reason to > emit a STATEMENT_LIST ... > > Or always emit a STATEMENT_LIST node whenever there could be one > (hopefully not every stmt could be a stmt list and hopefully > we don't treat single-stmt STATEMENT_LIST specially). > > That is, the fix should be to avoid this difference in the first > place, not try to deal with the difference later. Or perhaps just a flag on the STATEMENT_LIST that would make it clear the STATEMENT_LIST wouldn't be there without -g. Or different tree code like STATEMENT_LIST, except that it would be only this kind of container. Or disable -gstatement-frontiers by default and declare it -fcompare-debug incompatible. Do any debug info consumers consume this? In any case, I'd really like Alex' feedback on this. Jakub
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Thu, 26 Mar 2020, Jakub Jelinek wrote: > On Thu, Mar 26, 2020 at 10:14:35AM +0100, Richard Biener wrote: > > Ick. I fear we really need a better way of dealing with this > > STATEMENT_LIST appearance with only -g ... > > Yeah, it is very ugly and in some PRs I'm out of ideas what to do. I think a more sustainable "fix" would be to simply not emit the DEBUG_BEGIN_STMT when it would be the only reason to emit a STATEMENT_LIST ... Or always emit a STATEMENT_LIST node whenever there could be one (hopefully not every stmt could be a stmt list and hopefully we don't treat single-stmt STATEMENT_LIST specially). That is, the fix should be to avoid this difference in the first place, not try to deal with the difference later. Richard.
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Thu, Mar 26, 2020 at 10:14:35AM +0100, Richard Biener wrote: > Ick. I fear we really need a better way of dealing with this > STATEMENT_LIST appearance with only -g ... Yeah, it is very ugly and in some PRs I'm out of ideas what to do. Jakub
Re: [PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
On Thu, 26 Mar 2020, Jakub Jelinek wrote:
> Hi!
>
> The following testcase FAILs since recently when the C++ FE started calling
> protected_set_expr_location more often.
> With -g, it is called on a STATEMENT_LIST that contains a DEBUG_BEGIN_STMT
> and CLEANUP_POINT_EXPR, and as STATEMENT_LISTs have !CAN_HAVE_LOCATION_P,
> nothing is set. Without -g, it is called instead on the CLEANUP_POINT_EXPR
> directly and changes its location.
>
> The following patch recurses on the single non-DEBUG_BEGIN_STMT statement
> of a STATEMENT_LIST if any to make the two behave the same.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ick. I fear we really need a better way of dealing with this
STATEMENT_LIST appearance with only -g ...
But OK for now.
Richard.
> 2020-03-26 Jakub Jelinek
>
> PR debug/94323
> * tree.c (protected_set_expr_location): Recurse on STATEMENT_LIST
> that contains exactly one non-DEBUG_BEGIN_STMT statement.
>
> * g++.dg/debug/pr94323.C: New test.
>
> --- gcc/tree.c.jj 2020-03-23 19:46:45.552448327 +0100
> +++ gcc/tree.c2020-03-25 17:22:12.438904778 +0100
> @@ -5146,6 +5146,33 @@ protected_set_expr_location (tree t, loc
> {
>if (CAN_HAVE_LOCATION_P (t))
> SET_EXPR_LOCATION (t, loc);
> + else if (t && TREE_CODE (t) == STATEMENT_LIST)
> +{
> + /* With -gstatement-frontiers we could have a STATEMENT_LIST with
> + DEBUG_BEGIN_STMT(s) and only a single other stmt, which with
> + -g wouldn't be present and we'd have that single other stmt
> + directly instead. */
> + struct tree_statement_list_node *n = STATEMENT_LIST_HEAD (t);
> + if (!n)
> + return;
> + while (TREE_CODE (n->stmt) == DEBUG_BEGIN_STMT)
> + {
> + n = n->next;
> + if (!n)
> + return;
> + }
> + tree t2 = n->stmt;
> + do
> + {
> + n = n->next;
> + if (!n)
> + {
> + protected_set_expr_location (t2, loc);
> + return;
> + }
> + }
> + while (TREE_CODE (n->stmt) == DEBUG_BEGIN_STMT);
> +}
> }
>
> /* Data used when collecting DECLs and TYPEs for language data removal. */
> --- gcc/testsuite/g++.dg/debug/pr94323.C.jj 2020-03-25 17:17:49.857819078
> +0100
> +++ gcc/testsuite/g++.dg/debug/pr94323.C 2020-03-25 17:17:17.533300951
> +0100
> @@ -0,0 +1,13 @@
> +// PR debug/94323
> +// { dg-do compile }
> +// { dg-options "-O2 -fcompare-debug" }
> +
> +volatile int a;
> +
> +void
> +foo ()
> +{
> + ({
> + a;
> + });
> +}
>
> Jakub
>
>
--
Richard Biener
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
[PATCH] tree: Fix -fcompare-debug issues due to protected_set_expr_location [PR94323]
Hi!
The following testcase FAILs since recently when the C++ FE started calling
protected_set_expr_location more often.
With -g, it is called on a STATEMENT_LIST that contains a DEBUG_BEGIN_STMT
and CLEANUP_POINT_EXPR, and as STATEMENT_LISTs have !CAN_HAVE_LOCATION_P,
nothing is set. Without -g, it is called instead on the CLEANUP_POINT_EXPR
directly and changes its location.
The following patch recurses on the single non-DEBUG_BEGIN_STMT statement
of a STATEMENT_LIST if any to make the two behave the same.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2020-03-26 Jakub Jelinek
PR debug/94323
* tree.c (protected_set_expr_location): Recurse on STATEMENT_LIST
that contains exactly one non-DEBUG_BEGIN_STMT statement.
* g++.dg/debug/pr94323.C: New test.
--- gcc/tree.c.jj 2020-03-23 19:46:45.552448327 +0100
+++ gcc/tree.c 2020-03-25 17:22:12.438904778 +0100
@@ -5146,6 +5146,33 @@ protected_set_expr_location (tree t, loc
{
if (CAN_HAVE_LOCATION_P (t))
SET_EXPR_LOCATION (t, loc);
+ else if (t && TREE_CODE (t) == STATEMENT_LIST)
+{
+ /* With -gstatement-frontiers we could have a STATEMENT_LIST with
+DEBUG_BEGIN_STMT(s) and only a single other stmt, which with
+-g wouldn't be present and we'd have that single other stmt
+directly instead. */
+ struct tree_statement_list_node *n = STATEMENT_LIST_HEAD (t);
+ if (!n)
+ return;
+ while (TREE_CODE (n->stmt) == DEBUG_BEGIN_STMT)
+ {
+ n = n->next;
+ if (!n)
+ return;
+ }
+ tree t2 = n->stmt;
+ do
+ {
+ n = n->next;
+ if (!n)
+ {
+ protected_set_expr_location (t2, loc);
+ return;
+ }
+ }
+ while (TREE_CODE (n->stmt) == DEBUG_BEGIN_STMT);
+}
}
/* Data used when collecting DECLs and TYPEs for language data removal. */
--- gcc/testsuite/g++.dg/debug/pr94323.C.jj 2020-03-25 17:17:49.857819078
+0100
+++ gcc/testsuite/g++.dg/debug/pr94323.C2020-03-25 17:17:17.533300951
+0100
@@ -0,0 +1,13 @@
+// PR debug/94323
+// { dg-do compile }
+// { dg-options "-O2 -fcompare-debug" }
+
+volatile int a;
+
+void
+foo ()
+{
+ ({
+ a;
+ });
+}
Jakub
