| Message ID | 49192c12-c10b-49d0-9eed-23cc81691fa2@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | avoid assuming every type has a size (PR 89662) | expand |
On Mon, Mar 11, 2019 at 9:16 PM Martin Sebor <msebor@gmail.com> wrote: > > A -Warray-bounds enhancement committed last year into GCC 9 > introduced an assumption that the MEM_REF type argument has > a size. The test case submitted in PR89662 does pointer > addition on void*, in which the MEM_REF type is void*, which > breaks the assumption. > > The attached change removes this assumption and considers such > types to have the size of 1. (The result is used to scale > the offset in diagnostics after it has been determined to be > out of bounds.) Why's this not catched here: if (POINTER_TYPE_P (reftype) || !COMPLETE_TYPE_P (reftype) ^^^ || TREE_CODE (TYPE_SIZE_UNIT (reftype)) != INTEGER_CST || RECORD_OR_UNION_TYPE_P (reftype)) return; and what avoids the bad situation for char (*a)[n]; sink (a - 1); ? That is, the code assumes TYPE_SIZE_UNIT is an INTEGER_CST but the above should get you a non-constant type? It's probably easier to generate a gimple testcase with this. Richard. > Martin
On 3/12/19 2:20 AM, Richard Biener wrote: > On Mon, Mar 11, 2019 at 9:16 PM Martin Sebor <msebor@gmail.com> wrote: >> >> A -Warray-bounds enhancement committed last year into GCC 9 >> introduced an assumption that the MEM_REF type argument has >> a size. The test case submitted in PR89662 does pointer >> addition on void*, in which the MEM_REF type is void*, which >> breaks the assumption. >> >> The attached change removes this assumption and considers such >> types to have the size of 1. (The result is used to scale >> the offset in diagnostics after it has been determined to be >> out of bounds.) > > Why's this not catched here: > > if (POINTER_TYPE_P (reftype) > || !COMPLETE_TYPE_P (reftype) > ^^^ > > || TREE_CODE (TYPE_SIZE_UNIT (reftype)) != INTEGER_CST > || RECORD_OR_UNION_TYPE_P (reftype)) > return; Reftype is the type of the argument referenced by the MEM_REF, not the type of the MEM_REF itself. The type examined in the patch is the latter. In the test case in PR 89662, reftype is unsigned char but the MEM_REF type is void*. void *f (void *c) { return c; } void g () { // unsigned char D.1930[1]; int n = 1; char c[n]; h (f(c) - 1); // h (&MEM[(void *)&D.1930 + -1B]); } > > and what avoids the bad situation for > > char (*a)[n]; > sink (a - 1); > > ? That is, the code assumes TYPE_SIZE_UNIT is an INTEGER_CST > but the above should get you a non-constant type? It's probably > easier to generate a gimple testcase with this. I think it will depend on what a points to. The only kind of VLA the code works with is one whose size is known (for others, like in some range, I haven't seen a MEM_REF) . In the following for instance we get: void f (void) { int n = 4; char a[n]; // unsigned char D.1935[4]; int (*p)[n] = &a; sink (p - 1); // &MEM[(void *)&D.1935 - 16B] // warning: array subscript -4 is outside array bounds of ‘unsigned char[4]’ sink (*p - 1); // &MEM[(void *)&D.1935 - 4B] // warning: array subscript -1 is outside array bounds of ‘unsigned char[4]’ } I'm not sure what a GIMPLE test case might look like that would make the code misbehave. I tried a few variations but most of them ICE somewhere else. Martin
On Tue, Mar 12, 2019 at 5:36 PM Martin Sebor <msebor@gmail.com> wrote: > > On 3/12/19 2:20 AM, Richard Biener wrote: > > On Mon, Mar 11, 2019 at 9:16 PM Martin Sebor <msebor@gmail.com> wrote: > >> > >> A -Warray-bounds enhancement committed last year into GCC 9 > >> introduced an assumption that the MEM_REF type argument has > >> a size. The test case submitted in PR89662 does pointer > >> addition on void*, in which the MEM_REF type is void*, which > >> breaks the assumption. > >> > >> The attached change removes this assumption and considers such > >> types to have the size of 1. (The result is used to scale > >> the offset in diagnostics after it has been determined to be > >> out of bounds.) > > > > Why's this not catched here: > > > > if (POINTER_TYPE_P (reftype) > > || !COMPLETE_TYPE_P (reftype) > > ^^^ > > > > || TREE_CODE (TYPE_SIZE_UNIT (reftype)) != INTEGER_CST > > || RECORD_OR_UNION_TYPE_P (reftype)) > > return; > > Reftype is the type of the argument referenced by the MEM_REF, > not the type of the MEM_REF itself. Ugh how misleading... > The type examined in > the patch is the latter. In the test case in PR 89662, reftype > is unsigned char but the MEM_REF type is void*. > > void *f (void *c) { return c; } > void g () { > // unsigned char D.1930[1]; > int n = 1; > char c[n]; > h (f(c) - 1); // h (&MEM[(void *)&D.1930 + -1B]); > } > > > > > and what avoids the bad situation for > > > > char (*a)[n]; > > sink (a - 1); > > > > ? That is, the code assumes TYPE_SIZE_UNIT is an INTEGER_CST > > but the above should get you a non-constant type? It's probably > > easier to generate a gimple testcase with this. > > I think it will depend on what a points to. The only kind of VLA > the code works with is one whose size is known (for others, like > in some range, I haven't seen a MEM_REF) . In the following for > instance we get: > > void f (void) > { > int n = 4; > char a[n]; // unsigned char D.1935[4]; > int (*p)[n] = &a; > sink (p - 1); // &MEM[(void *)&D.1935 - 16B] > // warning: array subscript -4 is outside array bounds of ‘unsigned > char[4]’ > sink (*p - 1); // &MEM[(void *)&D.1935 - 4B] > // warning: array subscript -1 is outside array bounds of ‘unsigned > char[4]’ > } > > I'm not sure what a GIMPLE test case might look like that would > make the code misbehave. I tried a few variations but most of > them ICE somewhere else. Thanks for trying. The patch is OK if you adjust it to verify TYPE_SIZE_UNIT is an INTEGER_CST before throwing it at wi::to_offset. Richard. > Martin
PR tree-optimization/89662 - -Warray-bounds ICE on void* arithmetic gcc/ChangeLog: PR tree-optimization/89662 * tree-vrp.c (vrp_prop::check_mem_ref): Avoid assuming every type has a size. gcc/testsuite/ChangeLog: PR tree-optimization/89662 * gcc.dg/Warray-bounds-41.c: New test. Index: gcc/tree-vrp.c =================================================================== --- gcc/tree-vrp.c (revision 269445) +++ gcc/tree-vrp.c (working copy) @@ -4718,13 +4718,16 @@ vrp_prop::check_mem_ref (location_t location, tree { /* Extract the element type out of MEM_REF and use its size to compute the index to print in the diagnostic; arrays - in MEM_REF don't mean anything. */ + in MEM_REF don't mean anything. A type with no size like + void is as good as having a size of 1. */ tree type = TREE_TYPE (ref); while (TREE_CODE (type) == ARRAY_TYPE) type = TREE_TYPE (type); - tree size = TYPE_SIZE_UNIT (type); - offrange[0] = offrange[0] / wi::to_offset (size); - offrange[1] = offrange[1] / wi::to_offset (size); + if (tree size = TYPE_SIZE_UNIT (type)) + { + offrange[0] = offrange[0] / wi::to_offset (size); + offrange[1] = offrange[1] / wi::to_offset (size); + } } else { Index: gcc/testsuite/gcc.dg/Warray-bounds-41.c =================================================================== --- gcc/testsuite/gcc.dg/Warray-bounds-41.c (nonexistent) +++ gcc/testsuite/gcc.dg/Warray-bounds-41.c (working copy) @@ -0,0 +1,33 @@ +/* PR tree-optimization/89662- -Warray-bounds ICE on void* arithmetic + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +void* vptr (void *c) +{ + return c; +} + +void sink (void*); + +void test_vptr_arith_vla_cst (void) +{ + int n = 1; + char c[n]; + sink (vptr (c) - 1); /* { dg-warning "\\\[-Warray-bounds" } */ +} + +void test_vptr_arith_vla_range (int n) +{ + if (n < 1 || 4 < n) + return; + + char c[n]; + sink (vptr (c) - 1); /* { dg-warning "\\\[-Warray-bounds" "pr82608" { xfail *-*-* } } */ +} + +void test_vptr_arith_vla_var (int n) +{ + char c[n]; + sink (vptr (c) - 1); /* { dg-warning "\\\[-Warray-bounds" "pr82608" { xfail *-*-* } } */ +} +