| Message ID | CAFULd4bfL3s697D5RScwtgNrSoRyMSTc7pFmRx0kazAHEde9Kg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 20, 2014 at 08:34:46PM +0100, Uros Bizjak wrote: > 2014-02-20 Uros Bizjak <ubizjak@gmail.com> > > * emit-rtl.c (gen_reg_rtx): Assert that > crtl->emit.regno_pointer_align_length is non-zero. > > The patch was bootstrapped and regression tested on > x86_64-pc-linux-gnu {,-m32} on 4.8 and 4.9 branch. > > OK for mainline and release branches? Ok for trunk for the time being. While it has been tested on x86_64/i686, we have tons of other targets, we need some time to find out if this doesn't break those other targets, perhaps they have similar bug like i?86 had. > --- emit-rtl.c (revision 207965) > +++ emit-rtl.c (working copy) > @@ -896,6 +896,9 @@ gen_reg_rtx (enum machine_mode mode) > return gen_rtx_CONCAT (mode, realpart, imagpart); > } > > + /* Do not call gen_reg_rtx with uninitialized crtl. */ > + gcc_assert (crtl->emit.regno_pointer_align_length); > + > /* Make sure regno_pointer_align, and regno_reg_rtx are large > enough to have an element for this pseudo reg number. */ Jakub
On Thu, Feb 20, 2014 at 9:05 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Feb 20, 2014 at 08:34:46PM +0100, Uros Bizjak wrote: >> 2014-02-20 Uros Bizjak <ubizjak@gmail.com> >> >> * emit-rtl.c (gen_reg_rtx): Assert that >> crtl->emit.regno_pointer_align_length is non-zero. >> >> The patch was bootstrapped and regression tested on >> x86_64-pc-linux-gnu {,-m32} on 4.8 and 4.9 branch. >> >> OK for mainline and release branches? > > Ok for trunk for the time being. While it has been tested > on x86_64/i686, we have tons of other targets, we need some time > to find out if this doesn't break those other targets, > perhaps they have similar bug like i?86 had. OK. I will ping for backport after 4.9 is released. Thanks, Uros.
Index: emit-rtl.c =================================================================== --- emit-rtl.c (revision 207965) +++ emit-rtl.c (working copy) @@ -896,6 +896,9 @@ gen_reg_rtx (enum machine_mode mode) return gen_rtx_CONCAT (mode, realpart, imagpart); } + /* Do not call gen_reg_rtx with uninitialized crtl. */ + gcc_assert (crtl->emit.regno_pointer_align_length); + /* Make sure regno_pointer_align, and regno_reg_rtx are large enough to have an element for this pseudo reg number. */
Hello! As discussed in PR 57896, calling gen_reg_rtx without populated crtl was the cause of severe internal data corruption. The code following /* Make sure regno_pointer_align, and regno_reg_rtx are large enough to have an element for this pseudo reg number. */ comment is simply not prepared to handle case where crtl->emit.regno_pointer_align_length (and reg_rtx_no) is zero. Even when resizing the array, the allocated array size stays zero (new size is calculated as crtl->emit.regno_pointer_align_length * 2), and consequently val = gen_raw_REG (mode, reg_rtx_no); regno_reg_rtx[reg_rtx_no++] = val; return val; at the end of the function writes well outside the array. IMO, the consequences of this data corruption warrant assert in gen_reg_rtx that crtl->emit.regno_pointer_align_length is non-zero. In case of invalid use, the compiler would ICE in a predictable way, with a clear message where and what went wrong, instead of ICEing in random place due to various unpredictable effects of nearby data corruption (please see debugging troubles in the PR 57896). I would like to propose the patch for all release branches. The benefit of gcc_assert instead of gcc_checking_assert is in consistent ICE (and consequently consistent bugreports) instead of a data corruption even for the non-checking compiler. I am aware that this assert *could* trigger some ICEs on other targets, but these will be triggered _instead_ of internal data corruption. 2014-02-20 Uros Bizjak <ubizjak@gmail.com> * emit-rtl.c (gen_reg_rtx): Assert that crtl->emit.regno_pointer_align_length is non-zero. The patch was bootstrapped and regression tested on x86_64-pc-linux-gnu {,-m32} on 4.8 and 4.9 branch. OK for mainline and release branches? Uros.