| Message ID | mpt4l3yuqp4.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Prevent -Og from deleting stores to write-only variables | expand |
On 7/7/19 3:41 AM, Richard Sandiford wrote: > This patch prevents -Og from deleting stores to write-only variables, > so that the values are still available when debugging. This seems > more convenient than forcing users to use __attribute__((used)) > (probably conditionally, if it's not something they want in release > builds). > > Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install? > > Richard > > > 2019-07-07 Richard Sandiford <richard.sandiford@arm.com> > > gcc/ > * tree-cfg.c (execute_fixup_cfg): Don't delete stores to write-only > variables for -Og. > > gcc/testsuite/ > * c-c++-common/guality/Og-static-wo-1.c: New test. > * g++.dg/guality/guality.exp: Separate the c-c++-common tests into > "Og" and "general" tests. Run the latter at -O0 and -Og only. > * gcc.dg/guality/guality.exp: Likewise. OK jeff
On Sun, Jul 07, 2019 at 10:41:43AM +0100, Richard Sandiford wrote: > gcc/testsuite/ > * c-c++-common/guality/Og-static-wo-1.c: New test. > * g++.dg/guality/guality.exp: Separate the c-c++-common tests into > "Og" and "general" tests. Run the latter at -O0 and -Og only. Do we really want further filename prefixes based tests? I find it extremely ugly in /vect/ and would appreciate not to add further ones. The tests can just use dg-skip-if, can't they? /* { dg-skip-if "" { *-*-* } { "*" } { "-O0" "-Og" } } */ would do it. > --- gcc/testsuite/g++.dg/guality/guality.exp 2019-07-01 10:15:31.000000000 +0100 > +++ gcc/testsuite/g++.dg/guality/guality.exp 2019-07-07 10:29:19.999365874 +0100 > @@ -65,8 +65,22 @@ if {[check_guality " > return 0; > } > "]} { > - gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.C]] "" "" > - gcc-dg-runtest [lsort [glob $srcdir/c-c++-common/guality/*.c]] "" "" > + set general [list] > + set Og [list] > + foreach file [lsort [glob $srcdir/c-c++-common/guality/*.c]] { > + switch -glob -- [file tail $file] { > + Og-* { lappend Og $file } > + * { lappend general $file } > + } > + } > + > + gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.C]] "" "" > + gcc-dg-runtest $general "" "" > + set-torture-options \ > + [list "-O0" "-Og"] \ > + [list {}] \ > + [list "-Og -flto"] > + gcc-dg-runtest $Og "" "" > } > > if [info exists guality_gdb_name] { > Index: gcc/testsuite/gcc.dg/guality/guality.exp > =================================================================== > --- gcc/testsuite/gcc.dg/guality/guality.exp 2019-07-01 10:15:31.000000000 +0100 > +++ gcc/testsuite/gcc.dg/guality/guality.exp 2019-07-07 10:29:19.999365874 +0100 > @@ -80,8 +80,22 @@ if {[check_guality " > return 0; > } > "]} { > - gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.c]] "" "" > - gcc-dg-runtest [lsort [glob $srcdir/c-c++-common/guality/*.c]] "" "-Wc++-compat" > + set general [list] > + set Og [list] > + foreach file [lsort [glob $srcdir/c-c++-common/guality/*.c]] { > + switch -glob -- [file tail $file] { > + Og-* { lappend Og $file } > + * { lappend general $file } > + } > + } > + > + gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.c]] "" "" > + gcc-dg-runtest $general "" "-Wc++-compat" > + set-torture-options \ > + [list "-O0" "-Og"] \ > + [list {}] \ > + [list "-Og -flto"] > + gcc-dg-runtest $Og "" "-Wc++-compat" > } > > if [info exists guality_gdb_name] { Jakub
Jakub Jelinek <jakub@redhat.com> writes: > On Sun, Jul 07, 2019 at 10:41:43AM +0100, Richard Sandiford wrote: >> gcc/testsuite/ >> * c-c++-common/guality/Og-static-wo-1.c: New test. >> * g++.dg/guality/guality.exp: Separate the c-c++-common tests into >> "Og" and "general" tests. Run the latter at -O0 and -Og only. > > Do we really want further filename prefixes based tests? > I find it extremely ugly in /vect/ and would appreciate not to add further > ones. But IMO the reason the /vect/ stuff is so ugly is that we use prefixes for things that ought to be in dg-additional-options instead (-fno-tree-dce, -fno-tree-sra, -fwrapv, -ftrapv, etc.). I think the core vect.exp split between bb-slp-* and loop vectorisation makes more sense though. We're testing two different modes of vectorisation, and at least in principle, it's better to differentiate between them in one place rather than in each individual test. The same idea applies here. guality.exp is testing how optimisation affects debug info quality, and we have two major modes: optimise normally, or optimise for the debug experience. Some things are fundamentally only going to work with the latter. > The tests can just use dg-skip-if, can't they? > /* { dg-skip-if "" { *-*-* } { "*" } { "-O0" "-Og" } } */ > would do it. Yeah, but I'd rather not maintain the list in each individual test. E.g. I'd still like to add -O1g/-O1+g etc. at some point. Would having a subdirectory for the new tests be better? Either with or without a separate .exp harness. Richard > >> --- gcc/testsuite/g++.dg/guality/guality.exp 2019-07-01 10:15:31.000000000 +0100 >> +++ gcc/testsuite/g++.dg/guality/guality.exp 2019-07-07 10:29:19.999365874 +0100 >> @@ -65,8 +65,22 @@ if {[check_guality " >> return 0; >> } >> "]} { >> - gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.C]] "" "" >> - gcc-dg-runtest [lsort [glob $srcdir/c-c++-common/guality/*.c]] "" "" >> + set general [list] >> + set Og [list] >> + foreach file [lsort [glob $srcdir/c-c++-common/guality/*.c]] { >> + switch -glob -- [file tail $file] { >> + Og-* { lappend Og $file } >> + * { lappend general $file } >> + } >> + } >> + >> + gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.C]] "" "" >> + gcc-dg-runtest $general "" "" >> + set-torture-options \ >> + [list "-O0" "-Og"] \ >> + [list {}] \ >> + [list "-Og -flto"] >> + gcc-dg-runtest $Og "" "" >> } >> >> if [info exists guality_gdb_name] { >> Index: gcc/testsuite/gcc.dg/guality/guality.exp >> =================================================================== >> --- gcc/testsuite/gcc.dg/guality/guality.exp 2019-07-01 10:15:31.000000000 +0100 >> +++ gcc/testsuite/gcc.dg/guality/guality.exp 2019-07-07 10:29:19.999365874 +0100 >> @@ -80,8 +80,22 @@ if {[check_guality " >> return 0; >> } >> "]} { >> - gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.c]] "" "" >> - gcc-dg-runtest [lsort [glob $srcdir/c-c++-common/guality/*.c]] "" "-Wc++-compat" >> + set general [list] >> + set Og [list] >> + foreach file [lsort [glob $srcdir/c-c++-common/guality/*.c]] { >> + switch -glob -- [file tail $file] { >> + Og-* { lappend Og $file } >> + * { lappend general $file } >> + } >> + } >> + >> + gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.c]] "" "" >> + gcc-dg-runtest $general "" "-Wc++-compat" >> + set-torture-options \ >> + [list "-O0" "-Og"] \ >> + [list {}] \ >> + [list "-Og -flto"] >> + gcc-dg-runtest $Og "" "-Wc++-compat" >> } >> >> if [info exists guality_gdb_name] { > > Jakub
Index: gcc/tree-cfg.c =================================================================== --- gcc/tree-cfg.c 2019-07-06 09:25:08.717486045 +0100 +++ gcc/tree-cfg.c 2019-07-07 10:29:19.999365874 +0100 @@ -9571,7 +9571,8 @@ execute_fixup_cfg (void) Keep access when store has side effect, i.e. in case when source is volatile. */ if (gimple_store_p (stmt) - && !gimple_has_side_effects (stmt)) + && !gimple_has_side_effects (stmt) + && !optimize_debug) { tree lhs = get_base_address (gimple_get_lhs (stmt)); Index: gcc/testsuite/c-c++-common/guality/Og-static-wo-1.c =================================================================== --- /dev/null 2019-06-14 15:59:19.298479944 +0100 +++ gcc/testsuite/c-c++-common/guality/Og-static-wo-1.c 2019-07-07 10:29:19.999365874 +0100 @@ -0,0 +1,15 @@ +/* { dg-do run } */ +/* { dg-options "-g" } */ + +#include "../../gcc.dg/nop.h" + +static int x = 0; + +int +main (void) +{ + asm volatile (NOP); /* { dg-final { gdb-test . "x" "0" } } */ + x = 1; + asm volatile (NOP); /* { dg-final { gdb-test . "x" "1" } } */ + return 0; +} Index: gcc/testsuite/g++.dg/guality/guality.exp =================================================================== --- gcc/testsuite/g++.dg/guality/guality.exp 2019-07-01 10:15:31.000000000 +0100 +++ gcc/testsuite/g++.dg/guality/guality.exp 2019-07-07 10:29:19.999365874 +0100 @@ -65,8 +65,22 @@ if {[check_guality " return 0; } "]} { - gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.C]] "" "" - gcc-dg-runtest [lsort [glob $srcdir/c-c++-common/guality/*.c]] "" "" + set general [list] + set Og [list] + foreach file [lsort [glob $srcdir/c-c++-common/guality/*.c]] { + switch -glob -- [file tail $file] { + Og-* { lappend Og $file } + * { lappend general $file } + } + } + + gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.C]] "" "" + gcc-dg-runtest $general "" "" + set-torture-options \ + [list "-O0" "-Og"] \ + [list {}] \ + [list "-Og -flto"] + gcc-dg-runtest $Og "" "" } if [info exists guality_gdb_name] { Index: gcc/testsuite/gcc.dg/guality/guality.exp =================================================================== --- gcc/testsuite/gcc.dg/guality/guality.exp 2019-07-01 10:15:31.000000000 +0100 +++ gcc/testsuite/gcc.dg/guality/guality.exp 2019-07-07 10:29:19.999365874 +0100 @@ -80,8 +80,22 @@ if {[check_guality " return 0; } "]} { - gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.c]] "" "" - gcc-dg-runtest [lsort [glob $srcdir/c-c++-common/guality/*.c]] "" "-Wc++-compat" + set general [list] + set Og [list] + foreach file [lsort [glob $srcdir/c-c++-common/guality/*.c]] { + switch -glob -- [file tail $file] { + Og-* { lappend Og $file } + * { lappend general $file } + } + } + + gcc-dg-runtest [lsort [glob $srcdir/$subdir/*.c]] "" "" + gcc-dg-runtest $general "" "-Wc++-compat" + set-torture-options \ + [list "-O0" "-Og"] \ + [list {}] \ + [list "-Og -flto"] + gcc-dg-runtest $Og "" "-Wc++-compat" } if [info exists guality_gdb_name] {