| Message ID | b002448f-c389-3078-82b9-fc87545bf533@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | avoid using an incompletely populated struct (PR 100574) | expand |
On 5/13/21 3:55 AM, Martin Sebor via Gcc-patches wrote: > A logic bug in the handling of PHI arguments in compute_objsize > that are all null pointers lets an incompletely populated struct > be used in a way that triggers an assertion causing an ICE. > > The attached patch corrects that by having compute_objsize fail > when the struct isn't fully populated (when all os the PHI's > arguments are null). > > Martin Martin, I'm getting test failures with your patch here: Running target unix/-m32 FAIL: g++.dg/pr100574.C -std=gnu++14 (test for excess errors) FAIL: g++.dg/pr100574.C -std=gnu++17 (test for excess errors) FAIL: g++.dg/pr100574.C -std=gnu++2a (test for excess errors) /home/ed/gnu/gcc-trunk/gcc/testsuite/g++.dg/pr100574.C:6:7: error: 'operator new' takes type 'size_t' ('unsigned int') as first parameter [-fpermissive]^M compiler exited with status 1 Bernd.
On 5/13/21 11:20 AM, Bernd Edlinger wrote: > On 5/13/21 3:55 AM, Martin Sebor via Gcc-patches wrote: >> A logic bug in the handling of PHI arguments in compute_objsize >> that are all null pointers lets an incompletely populated struct >> be used in a way that triggers an assertion causing an ICE. >> >> The attached patch corrects that by having compute_objsize fail >> when the struct isn't fully populated (when all os the PHI's >> arguments are null). >> >> Martin > > Martin, > > I'm getting test failures with your patch here: > > Running target unix/-m32 > FAIL: g++.dg/pr100574.C -std=gnu++14 (test for excess errors) > FAIL: g++.dg/pr100574.C -std=gnu++17 (test for excess errors) > FAIL: g++.dg/pr100574.C -std=gnu++2a (test for excess errors) > > /home/ed/gnu/gcc-trunk/gcc/testsuite/g++.dg/pr100574.C:6:7: error: 'operator new' takes type 'size_t' ('unsigned int') as first parameter [-fpermissive]^M > compiler exited with status 1 Thanks, I've just fixed it. Martin > > > Bernd. >
On 5/12/2021 7:55 PM, Martin Sebor via Gcc-patches wrote: > A logic bug in the handling of PHI arguments in compute_objsize > that are all null pointers lets an incompletely populated struct > be used in a way that triggers an assertion causing an ICE. > > The attached patch corrects that by having compute_objsize fail > when the struct isn't fully populated (when all os the PHI's > arguments are null). > > Martin > > gcc-100574.diff > > PR middle-end/100574 - ICE: in size_remaining, at builtins.c > > gcc/ChangeLog: > > PR middle-end/100574 > * builtins.c (access_ref::get_ref): Improve detection of PHIs with > all null arguments. > > gcc/testsuite/ChangeLog: > > PR middle-end/100574 > * g++.dg/pr100574.C: New test. OK jeff
On 5/13/21 11:36 AM, Martin Sebor wrote: > On 5/13/21 11:20 AM, Bernd Edlinger wrote: >> On 5/13/21 3:55 AM, Martin Sebor via Gcc-patches wrote: >>> A logic bug in the handling of PHI arguments in compute_objsize >>> that are all null pointers lets an incompletely populated struct >>> be used in a way that triggers an assertion causing an ICE. >>> >>> The attached patch corrects that by having compute_objsize fail >>> when the struct isn't fully populated (when all os the PHI's >>> arguments are null). >>> >>> Martin >> >> Martin, >> >> I'm getting test failures with your patch here: >> >> Running target unix/-m32 >> FAIL: g++.dg/pr100574.C -std=gnu++14 (test for excess errors) >> FAIL: g++.dg/pr100574.C -std=gnu++17 (test for excess errors) >> FAIL: g++.dg/pr100574.C -std=gnu++2a (test for excess errors) >> >> /home/ed/gnu/gcc-trunk/gcc/testsuite/g++.dg/pr100574.C:6:7: error: >> 'operator new' takes type 'size_t' ('unsigned int') as first parameter >> [-fpermissive]^M >> compiler exited with status 1 > > Thanks, I've just fixed it. I hadn't checked in the patch yet. I'm only now about to do it and see I inadvertently committed the test in response to your email about the failures. I didn't realize you were testing the patch I had posted for review, before I committed it. Martin > > Martin > >> >> >> Bernd. >> >
On 5/14/21 12:35 AM, Martin Sebor wrote: > On 5/13/21 11:36 AM, Martin Sebor wrote: >> On 5/13/21 11:20 AM, Bernd Edlinger wrote: >>> On 5/13/21 3:55 AM, Martin Sebor via Gcc-patches wrote: >>>> A logic bug in the handling of PHI arguments in compute_objsize >>>> that are all null pointers lets an incompletely populated struct >>>> be used in a way that triggers an assertion causing an ICE. >>>> >>>> The attached patch corrects that by having compute_objsize fail >>>> when the struct isn't fully populated (when all os the PHI's >>>> arguments are null). >>>> >>>> Martin >>> >>> Martin, >>> >>> I'm getting test failures with your patch here: >>> >>> Running target unix/-m32 >>> FAIL: g++.dg/pr100574.C -std=gnu++14 (test for excess errors) >>> FAIL: g++.dg/pr100574.C -std=gnu++17 (test for excess errors) >>> FAIL: g++.dg/pr100574.C -std=gnu++2a (test for excess errors) >>> >>> /home/ed/gnu/gcc-trunk/gcc/testsuite/g++.dg/pr100574.C:6:7: error: 'operator new' takes type 'size_t' ('unsigned int') as first parameter [-fpermissive]^M >>> compiler exited with status 1 >> >> Thanks, I've just fixed it. > > I hadn't checked in the patch yet. I'm only now about to do it and > see I inadvertently committed the test in response to your email > about the failures. I didn't realize you were testing the patch > I had posted for review, before I committed it. > Oh, well. I just wanted to help, since you didn't tell how you tested the patch. Bernd. > Martin > >> >> Martin >> >>> >>> >>> Bernd. >>> >> >
On 5/13/21 10:53 PM, Bernd Edlinger wrote: > On 5/14/21 12:35 AM, Martin Sebor wrote: >> On 5/13/21 11:36 AM, Martin Sebor wrote: >>> On 5/13/21 11:20 AM, Bernd Edlinger wrote: >>>> On 5/13/21 3:55 AM, Martin Sebor via Gcc-patches wrote: >>>>> A logic bug in the handling of PHI arguments in compute_objsize >>>>> that are all null pointers lets an incompletely populated struct >>>>> be used in a way that triggers an assertion causing an ICE. >>>>> >>>>> The attached patch corrects that by having compute_objsize fail >>>>> when the struct isn't fully populated (when all os the PHI's >>>>> arguments are null). >>>>> >>>>> Martin >>>> >>>> Martin, >>>> >>>> I'm getting test failures with your patch here: >>>> >>>> Running target unix/-m32 >>>> FAIL: g++.dg/pr100574.C -std=gnu++14 (test for excess errors) >>>> FAIL: g++.dg/pr100574.C -std=gnu++17 (test for excess errors) >>>> FAIL: g++.dg/pr100574.C -std=gnu++2a (test for excess errors) >>>> >>>> /home/ed/gnu/gcc-trunk/gcc/testsuite/g++.dg/pr100574.C:6:7: error: 'operator new' takes type 'size_t' ('unsigned int') as first parameter [-fpermissive]^M >>>> compiler exited with status 1 >>> >>> Thanks, I've just fixed it. >> >> I hadn't checked in the patch yet. I'm only now about to do it and >> see I inadvertently committed the test in response to your email >> about the failures. I didn't realize you were testing the patch >> I had posted for review, before I committed it. >> > > Oh, well. > > I just wanted to help, since you didn't tell how you tested the patch. > No problem and thanks for your help. Martin > > Bernd. > >> Martin >> >>> >>> Martin >>> >>>> >>>> >>>> Bernd. >>>> >>> >>
PR middle-end/100574 - ICE: in size_remaining, at builtins.c gcc/ChangeLog: PR middle-end/100574 * builtins.c (access_ref::get_ref): Improve detection of PHIs with all null arguments. gcc/testsuite/ChangeLog: PR middle-end/100574 * g++.dg/pr100574.C: New test. diff --git a/gcc/builtins.c b/gcc/builtins.c index 2f0efae11e8..e1b284846b1 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -362,15 +362,6 @@ access_ref::get_ref (vec<access_ref> *all_refs, same_ref.offrng[1] = phi_arg_ref.offrng[1]; } - if (phi_ref.sizrng[0] < 0) - { - /* Fail if none of the PHI's arguments resulted in updating PHI_REF - (perhaps because they have all been already visited by prior - recursive calls). */ - psnlim->leave_phi (ref); - return NULL_TREE; - } - if (!same_ref.ref && same_ref.offrng[0] != 0) /* Clear BASE0 if not all the arguments refer to the same object and if not all their offsets are zero-based. This allows the final @@ -390,6 +381,15 @@ access_ref::get_ref (vec<access_ref> *all_refs, phi_ref.parmarray = parmarray; } + if (phi_ref.sizrng[0] < 0) + { + /* Fail if none of the PHI's arguments resulted in updating PHI_REF + (perhaps because they have all been already visited by prior + recursive calls). */ + psnlim->leave_phi (ref); + return NULL_TREE; + } + /* Avoid changing *THIS. */ if (pref && pref != this) *pref = phi_ref; diff --git a/gcc/testsuite/g++.dg/pr100574.C b/gcc/testsuite/g++.dg/pr100574.C new file mode 100644 index 00000000000..e80dd2a9191 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr100574.C @@ -0,0 +1,64 @@ +/* PR middle-end/100574 - ICE: in size_remaining, at builtins.c:413 with + -O3 -ftracer -fno-tree-dominator-opts -fno-tree-fre + { dg-do compile { target c++11 } } + { dg-options "-O3 -ftracer -fno-tree-dominator-opts -fno-tree-fre" } */ + +void *operator new (unsigned long, void *__p) { return __p; } + +template <typename> struct allocator_traits; +template <typename> class allocator {}; +template <typename _Tp> struct allocator_traits<allocator<_Tp> > +{ + using allocator_type = allocator<_Tp>; + using pointer = _Tp *; + using size_type = long; + template <typename _Up> using rebind_alloc = allocator<_Up>; + static pointer allocate(allocator_type, size_type); + template <typename _Up> static void construct(_Up __p) { new (__p) _Up(); } +}; + +struct __alloc_traits : allocator_traits<allocator<char>> { + struct rebind { + typedef rebind_alloc<char> other; + }; +}; + +struct _Vector_base { + struct : __alloc_traits::rebind::other { + } _M_impl; + long _M_allocate___n; +}; + +template <typename, typename = char> class vector : _Vector_base { + long max_size(); +public: + void push_back() { _M_realloc_insert(); } + template <typename...> void _M_realloc_insert(); +}; + +template <typename _Tp, typename _Alloc> +template <typename...> +void vector<_Tp, _Alloc>::_M_realloc_insert() { + __alloc_traits::pointer __trans_tmp_5; + long __len(__len || max_size()), __elems_before; + __trans_tmp_5 = _M_allocate___n + ? __alloc_traits::allocate(_M_impl, _M_allocate___n) + : __alloc_traits::pointer(); + __alloc_traits::construct(__trans_tmp_5 + __elems_before); +} + +enum { MIDIST_PITCHBEND }; +struct DataBlock { + vector<char> data; +}; + +char ReadTrackChunk_status; + +void ReadTrackChunk() +{ + DataBlock block; + while (!0) + switch (ReadTrackChunk_status) + case MIDIST_PITCHBEND: + block.data.push_back(); +}