| Message ID | 20180701161920.lzm5ikaiyr3pspxn@delia |
|---|---|
| State | New |
| Headers | show |
| Series | [testsuite/guality,committed] Prevent optimization of local in vla-1.c | expand |
On Sun, Jul 01, 2018 at 06:19:20PM +0200, Tom de Vries wrote: > So, the local vla a is optimized away. > > This patch adds VOLATILE to 'a', which prevents it from being optimized away, > and fixes the non-lto failures. > > Committed as obvious. That isn't obvious, it is just wrong. The intent of the testcase is to test how debugging of optimized code with VLAs works. With your change we don't test that anymore. Please revert. > [testsuite/guality] Prevent optimization of local in vla-1.c > > 2018-07-01 Tom de Vries <tdevries@suse.de> > > * gcc.dg/guality/prevent-optimization.h (VOLATILE): Define. > * gcc.dg/guality/vla-1.c (f1): Mark local vla a as VOLATILE. Jakub
On 07/01/2018 09:11 PM, Jakub Jelinek wrote: > On Sun, Jul 01, 2018 at 06:19:20PM +0200, Tom de Vries wrote: >> So, the local vla a is optimized away. >> >> This patch adds VOLATILE to 'a', which prevents it from being optimized away, >> and fixes the non-lto failures. >> >> Committed as obvious. > > That isn't obvious, it is just wrong. > The intent of the testcase is to test how debugging of optimized code with > VLAs works. With your change we don't test that anymore. Please revert. > Hi, Sure, but ... if this is wrong, then for my understanding can you explain to me how the fail should be addressed? [ FWIW, I considered this obvious, given the ok for "[testsuite] Fix guality/pr45882.c for flto" ( https://gcc.gnu.org/ml/gcc-patches/2018-06/msg01304.html ) which seemed similar to me. ] Thanks, - Tom >> [testsuite/guality] Prevent optimization of local in vla-1.c >> >> 2018-07-01 Tom de Vries <tdevries@suse.de> >> >> * gcc.dg/guality/prevent-optimization.h (VOLATILE): Define. >> * gcc.dg/guality/vla-1.c (f1): Mark local vla a as VOLATILE. > > Jakub >
On Sun, Jul 1, 2018 at 9:25 PM Tom de Vries <tdevries@suse.de> wrote: > > On 07/01/2018 09:11 PM, Jakub Jelinek wrote: > > On Sun, Jul 01, 2018 at 06:19:20PM +0200, Tom de Vries wrote: > >> So, the local vla a is optimized away. > >> > >> This patch adds VOLATILE to 'a', which prevents it from being optimized away, > >> and fixes the non-lto failures. > >> > >> Committed as obvious. > > > > That isn't obvious, it is just wrong. > > The intent of the testcase is to test how debugging of optimized code with > > VLAs works. With your change we don't test that anymore. Please revert. > > > > Hi, > > Sure, but ... if this is wrong, then for my understanding can you > explain to me how the fail should be addressed? Given the array has size i + 1 it's upper bound should be 'i' and 'i' should be available via DW_OP_[GNU_]entry_value. I see it is <175> DW_AT_upper_bound : 10 byte block: 75 1 8 20 24 8 20 26 31 1c (DW_OP_breg5 (rdi): 1; DW_OP_const1u: 32; DW_OP_shl; DW_OP_const1u: 32; DW_OP_shra; DW_OP_lit1; DW_OP_minus) and %rdi is 1. Not sure why gdb fails to print it's length. Yes, the storage itself doesn't have a location but the type specifies the size. (gdb) ptype a type = char [variable length] (gdb) p sizeof(a) $3 = 0 this looks like a gdb bug to me? Btw, the location expression looks odd, if I deciper it correctly we end up with ((%rdi << 32) >> 32) - 1 which computes to 4 but the upper bound should be 5. The GIMPLE debug stmts compute the upper bound as (sizetype)((long)(i_1(D) + 1) - 1) vartrack correctly prints (debug_insn 7 26 8 2 (var_location:SI D#3 (plus:SI (entry_value:SI (reg:SI 5 di [ i ])) (const_int 1 [0x1]))) "/space/rguenther/src/gcc-slpcost/gcc/testsuite/gcc.dg/guality/vla-1.c":15 -1 (nil)) (debug_insn 8 7 9 2 (var_location:DI D#2 (sign_extend:DI (debug_expr:SI D#3))) "/space/rguenther/src/gcc-slpcost/gcc/testsuite/gcc.dg/guality/vla-1.c":15 -1 (nil)) (debug_insn 9 8 10 2 (var_location:DI D#1 (plus:DI (debug_expr:DI D#2) (const_int -1 [0xffffffffffffffff]))) "/space/rguenther/src/gcc-slpcost/gcc/testsuite/gcc.dg/guality/vla-1.c":15 -1 (nil)) (debug_insn 10 9 11 2 (var_location:DI D.1914 (debug_expr:DI D#1)) "/space/rguenther/src/gcc-slpcost/gcc/testsuite/gcc.dg/guality/vla-1.c":15 -1 (nil)) Richard. > [ FWIW, I considered this obvious, given the ok for "[testsuite] Fix > guality/pr45882.c for flto" ( > https://gcc.gnu.org/ml/gcc-patches/2018-06/msg01304.html ) which seemed > similar to me. ] > > Thanks, > - Tom > > >> [testsuite/guality] Prevent optimization of local in vla-1.c > >> > >> 2018-07-01 Tom de Vries <tdevries@suse.de> > >> > >> * gcc.dg/guality/prevent-optimization.h (VOLATILE): Define. > >> * gcc.dg/guality/vla-1.c (f1): Mark local vla a as VOLATILE. > > > > Jakub > >
On Mon, Jul 02, 2018 at 09:44:04AM +0200, Richard Biener wrote: > Given the array has size i + 1 it's upper bound should be 'i' and 'i' > should be available via DW_OP_[GNU_]entry_value. > > I see it is > > <175> DW_AT_upper_bound : 10 byte block: 75 1 8 20 24 8 20 26 31 > 1c (DW_OP_breg5 (rdi): 1; DW_OP_const1u: 32; DW_OP_shl; > DW_OP_const1u: 32; DW_OP_shra; DW_OP_lit1; DW_OP_minus) > > and %rdi is 1. Not sure why gdb fails to print it's length. Yes, the > storage itself doesn't have a location but the > type specifies the size. > > (gdb) ptype a > type = char [variable length] > (gdb) p sizeof(a) > $3 = 0 > > this looks like a gdb bug to me? > > Btw, the location expression looks odd, if I deciper it correctly > we end up with ((%rdi << 32) >> 32) - 1 which computes to 4 > but the upper bound should be 5. The GIMPLE debug stmts compute > the upper bound as (sizetype)((long)(i_1(D) + 1) - 1) The << 32 >> 32 is sign extension. And yes, for f1 I don't see why DW_OP_GNU_entry_value shouldn't work, i in main is needed for the call to f2, so needs to live in some register or memory in that function until the second call. For f2 i is needed after the bar call for the a[i + 4] read, worst case in form of precomputed i + 4, but that is reversible op. Jakub
On 07/02/2018 10:16 AM, Jakub Jelinek wrote: > On Mon, Jul 02, 2018 at 09:44:04AM +0200, Richard Biener wrote: >> Given the array has size i + 1 it's upper bound should be 'i' and 'i' >> should be available via DW_OP_[GNU_]entry_value. >> >> I see it is >> >> <175> DW_AT_upper_bound : 10 byte block: 75 1 8 20 24 8 20 26 31 >> 1c (DW_OP_breg5 (rdi): 1; DW_OP_const1u: 32; DW_OP_shl; >> DW_OP_const1u: 32; DW_OP_shra; DW_OP_lit1; DW_OP_minus) >> >> and %rdi is 1. Not sure why gdb fails to print it's length. Yes, the >> storage itself doesn't have a location but the >> type specifies the size. >> >> (gdb) ptype a >> type = char [variable length] >> (gdb) p sizeof(a) >> $3 = 0 >> >> this looks like a gdb bug to me? >> With gdb patch: ... diff --git a/gdb/findvar.c b/gdb/findvar.c index 8ad5e25cb2..ebaff923a1 100644 --- a/gdb/findvar.c +++ b/gdb/findvar.c @@ -789,6 +789,8 @@ default_read_var_value break; case LOC_OPTIMIZED_OUT: + if (is_dynamic_type (type)) + type = resolve_dynamic_type (type, NULL, + /* Unused address. */ 0); return allocate_optimized_out_value (type); default: ... I get: ... $ ./gdb -batch -ex "b f1" -ex "r" -ex "p sizeof (a)" vla-1.exe Breakpoint 1 at 0x4004a8: file vla-1.c, line 17. Breakpoint 1, f1 (i=i@entry=5) at vla-1.c:17 17 return a[0]; $1 = 6 ... So I'd say it's a gdb bug indeed. Thanks, - Tom >> Btw, the location expression looks odd, if I deciper it correctly >> we end up with ((%rdi << 32) >> 32) - 1 which computes to 4 >> but the upper bound should be 5. The GIMPLE debug stmts compute >> the upper bound as (sizetype)((long)(i_1(D) + 1) - 1) > > The << 32 >> 32 is sign extension. And yes, for f1 I don't see why > DW_OP_GNU_entry_value shouldn't work, i in main is needed for the call to > f2, so needs to live in some register or memory in that function until the > second call. For f2 i is needed after the bar call for the a[i + 4] read, > worst case in form of precomputed i + 4, but that is reversible op. > > Jakub >
On 07/03/2018 11:05 AM, Tom de Vries wrote: > On 07/02/2018 10:16 AM, Jakub Jelinek wrote: >> On Mon, Jul 02, 2018 at 09:44:04AM +0200, Richard Biener wrote: >>> Given the array has size i + 1 it's upper bound should be 'i' and 'i' >>> should be available via DW_OP_[GNU_]entry_value. >>> >>> I see it is >>> >>> <175> DW_AT_upper_bound : 10 byte block: 75 1 8 20 24 8 20 26 31 >>> 1c (DW_OP_breg5 (rdi): 1; DW_OP_const1u: 32; DW_OP_shl; >>> DW_OP_const1u: 32; DW_OP_shra; DW_OP_lit1; DW_OP_minus) >>> >>> and %rdi is 1. Not sure why gdb fails to print it's length. Yes, the >>> storage itself doesn't have a location but the >>> type specifies the size. >>> >>> (gdb) ptype a >>> type = char [variable length] >>> (gdb) p sizeof(a) >>> $3 = 0 >>> >>> this looks like a gdb bug to me? >>> > > With gdb patch: > ... > diff --git a/gdb/findvar.c b/gdb/findvar.c > index 8ad5e25cb2..ebaff923a1 100644 > --- a/gdb/findvar.c > +++ b/gdb/findvar.c > @@ -789,6 +789,8 @@ default_read_var_value > break; > > case LOC_OPTIMIZED_OUT: > + if (is_dynamic_type (type)) > + type = resolve_dynamic_type (type, NULL, > + /* Unused address. */ 0); > return allocate_optimized_out_value (type); > > default: > ... > > I get: > ... > $ ./gdb -batch -ex "b f1" -ex "r" -ex "p sizeof (a)" vla-1.exe > Breakpoint 1 at 0x4004a8: file vla-1.c, line 17. > > Breakpoint 1, f1 (i=i@entry=5) at vla-1.c:17 > 17 return a[0]; > $1 = 6 > ... > Well, for -O1 and -O2. For O3, I get instead: ... $ ./gdb vla-1.exe -q -batch -ex "b f1" -ex "run" -ex "p sizeof (a)" Breakpoint 1 at 0x4004b0: f1. (2 locations) Breakpoint 1, f1 (i=5) at vla-1.c:17 17 return a[0]; $1 = 0 ... At O3, f1 is cloned to a version f1.constprop with no parameters, eliminating parameter i, but i is still accessible via DW_OP_GNU_parameter_ref: ... $ ./gdb vla-1.exe -q -batch -ex "b f1" -ex "run" -ex "p i" -ex "info addr i" Breakpoint 1 at 0x4004b0: f1. (2 locations) Breakpoint 1, f1 (i=5) at vla-1.c:17 17 return a[0]; $1 = 5 Symbol "i" is a complex DWARF expression: 0: DW_OP_GNU_parameter_ref offset 125 5: DW_OP_stack_value . ... Variable a in f1 has full info available: ... <2><1f6>: Abbrev Number: 20 (DW_TAG_variable) <1f7> DW_AT_name : a <1f9> DW_AT_decl_file : 1 <1fa> DW_AT_decl_line : 15 <1fb> DW_AT_decl_column : 8 <1fc> DW_AT_type : <0x201> <2><200>: Abbrev Number: 0 <1><201>: Abbrev Number: 15 (DW_TAG_array_type) <202> DW_AT_type : <0x21b> <206> DW_AT_sibling : <0x21b> <2><20a>: Abbrev Number: 21 (DW_TAG_subrange_type) <20b> DW_AT_type : <0x1ce> <20f> DW_AT_upper_bound : 10 byte block: 75 1 8 20 24 8 20 26 31 1c (DW_OP_breg5 (rdi): 1; DW_OP_const1u: 32; DW_OP_shl; DW_OP_const1u: 32; DW_OP_shra; DW_OP_lit1; DW_OP_minus) <2><21a>: Abbrev Number: 0 ... but a in f1.constprop has no DW_AT_upper_bound: ... <2><26e>: Abbrev Number: 26 (DW_TAG_variable) <26f> DW_AT_abstract_origin: <0x1f6> <273> DW_AT_type : <0x284> <2><283>: Abbrev Number: 0 <1><284>: Abbrev Number: 15 (DW_TAG_array_type) <285> DW_AT_type : <0x21b> <289> DW_AT_sibling : <0x293> <2><28d>: Abbrev Number: 28 (DW_TAG_subrange_type) <28e> DW_AT_type : <0x1ce> <2><292>: Abbrev Number: 0 ... AFAIU, we could emit a DW_AT_upper_bound here as well, using DW_OP_GNU_parameter_ref as we do for i. Thanks, - Tom
diff --git a/gcc/testsuite/gcc.dg/guality/prevent-optimization.h b/gcc/testsuite/gcc.dg/guality/prevent-optimization.h index 0ef84a3d9c8..57e945cafb4 100644 --- a/gcc/testsuite/gcc.dg/guality/prevent-optimization.h +++ b/gcc/testsuite/gcc.dg/guality/prevent-optimization.h @@ -21,8 +21,10 @@ along with GCC; see the file COPYING3. If not see #ifdef PREVENT_OPTIMIZATION #define ATTRIBUTE_USED __attribute__((used)) +#define VOLATILE volatile #else #define ATTRIBUTE_USED +#define VOLATILE #endif #endif diff --git a/gcc/testsuite/gcc.dg/guality/vla-1.c b/gcc/testsuite/gcc.dg/guality/vla-1.c index 264b9f3f92b..d281185c18c 100644 --- a/gcc/testsuite/gcc.dg/guality/vla-1.c +++ b/gcc/testsuite/gcc.dg/guality/vla-1.c @@ -2,6 +2,8 @@ /* { dg-do run } */ /* { dg-options "-g" } */ +#include "prevent-optimization.h" + void __attribute__((noinline)) bar (short *p) { @@ -12,7 +14,7 @@ bar (short *p) int __attribute__((noinline)) f1 (int i) { - char a[i + 1]; + VOLATILE char a[i + 1]; a[0] = 5; /* { dg-final { gdb-test .+1 "i" "5" } } */ return a[0]; /* { dg-final { gdb-test . "sizeof (a)" "6" } } */ }
Hi, Atm vla-1.c has the following failures: ... FAIL: gcc.dg/guality/vla-1.c -O1 -DPREVENT_OPTIMIZATION line 17 sizeof (a) == 6 FAIL: gcc.dg/guality/vla-1.c -O2 -DPREVENT_OPTIMIZATION line 17 sizeof (a) == 6 FAIL: gcc.dg/guality/vla-1.c -O3 -g -DPREVENT_OPTIMIZATION line 17 sizeof (a) == 6 FAIL: gcc.dg/guality/vla-1.c -Os -DPREVENT_OPTIMIZATION line 17 sizeof (a) == 6 FAIL: gcc.dg/guality/vla-1.c -O2 -flto -fno-use-linker-plugin -flto-partition=none -DPREVENT_OPTIMIZATION line 17 sizeof (a) == 6 FAIL: gcc.dg/guality/vla-1.c -O2 -flto -fno-use-linker-plugin -flto-partition=none -DPREVENT_OPTIMIZATION line 24 sizeof (a) == 17 * sizeof (short) FAIL: gcc.dg/guality/vla-1.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects -DPREVENT_OPTIMIZATION line 17 sizeof (a) == 6 FAIL: gcc.dg/guality/vla-1.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects -DPREVENT_OPTIMIZATION line 24 sizeof (a) == 17 * sizeof (short) ... For the non-lto failures, the relevant bit is: ... int __attribute__((noinline)) f1 (int i) { char a[i + 1]; a[0] = 5; /* { dg-final { gdb-test .+1 "i" "5" } } */ return a[0]; /* { dg-final { gdb-test . "sizeof (a)" "6" } } */ } ... which at -O1 already ends up as: ... # vla-1.c:14:1 .loc 1 14 1 is_stmt 1 .cfi_startproc # DEBUG i => di # vla-1.c:15:3 .loc 1 15 3 # DEBUG => sxn(di+0x1)-0x1 # vla-1.c:16:3 .loc 1 16 3 # vla-1.c:17:3 .loc 1 17 3 # vla-1.c:18:1 .loc 1 18 1 is_stmt 0 movl $5, %eax ret ... So, the local vla a is optimized away. This patch adds VOLATILE to 'a', which prevents it from being optimized away, and fixes the non-lto failures. Committed as obvious. Thanks, - Tom [testsuite/guality] Prevent optimization of local in vla-1.c 2018-07-01 Tom de Vries <tdevries@suse.de> * gcc.dg/guality/prevent-optimization.h (VOLATILE): Define. * gcc.dg/guality/vla-1.c (f1): Mark local vla a as VOLATILE. --- gcc/testsuite/gcc.dg/guality/prevent-optimization.h | 2 ++ gcc/testsuite/gcc.dg/guality/vla-1.c | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-)