Re: fix math wrt volatile-bitfields vs C++ model
On Wed, Jun 11, 2014 at 11:35 PM, DJ Delorie wrote: > > If the combined bitfields are exactly the size of the mode, the logic > for detecting range overflow is flawed - it calculates an ending > "position" that's the position of the first bit in the next field. > > In the case of "short" for example, you get "16 > 15" without this > patch (comparing size to position), and "15 > 15" with (comparing > position to position). > > Ok to apply? Looks ok to me, but can you add a testcase please? Also check if 4.9 is affected. Thanks, Richard. > * expmed.c (strict_volatile_bitfield_p): Fix off-by-one error. > > Index: expmed.c > === > --- expmed.c(revision 211479) > +++ expmed.c(working copy) > @@ -472,13 +472,13 @@ strict_volatile_bitfield_p (rtx op0, uns > && bitnum % GET_MODE_ALIGNMENT (fieldmode) + bitsize > modesize)) > return false; > >/* Check for cases where the C++ memory model applies. */ >if (bitregion_end != 0 >&& (bitnum - bitnum % modesize < bitregion_start > - || bitnum - bitnum % modesize + modesize > bitregion_end)) > + || bitnum - bitnum % modesize + modesize - 1 > bitregion_end)) > return false; > >return true; > } > > /* Return true if OP is a memory and if a bitfield of size BITSIZE at
Re: fix math wrt volatile-bitfields vs C++ model
> Looks ok to me, but can you add a testcase please? I have a testcase, but if -flto the testcase doesn't include *any* definition of the test function, just all the LTO data. Is this normal? > Also check if 4.9 is affected. It is... same fix works, though.
Re: fix math wrt volatile-bitfields vs C++ model
On Tue, Jun 17, 2014 at 4:08 AM, DJ Delorie wrote: > >> Looks ok to me, but can you add a testcase please? > > I have a testcase, but if -flto the testcase doesn't include *any* > definition of the test function, just all the LTO data. Is this > normal? Without -ffat-lto-objects yes, this is normal. If you are trying to do a scan-assembler or so then this will be difficult with LTO. If LTO is not necessary to trigger the bug and you just want to use the torture I suggest to dg-skip-if -flto. >> Also check if 4.9 is affected. > > It is... same fix works, though. Thanks, Richard.
Re: fix math wrt volatile-bitfields vs C++ model
Hi, On Tue, 17 Jun 2014 10:08:33, Richard Biener wrote: > On Tue, Jun 17, 2014 at 4:08 AM, DJ Delorie wrote: >> >>> Looks ok to me, but can you add a testcase please? >> >> I have a testcase, but if -flto the testcase doesn't include *any* >> definition of the test function, just all the LTO data. Is this >> normal? > > Without -ffat-lto-objects yes, this is normal. If you are trying to > do a scan-assembler or so then this will be difficult with LTO. > If LTO is not necessary to trigger the bug and you just want to > use the torture I suggest to dg-skip-if -flto. > >>> Also check if 4.9 is affected. >> >> It is... same fix works, though. > > Thanks, > Richard. If you have a test case where the generated code is actually different with and without your patch, that would be interesting. Please see gcc.dg/pr23623.c and gcc.dg/pr56997-4.c for examples how to automatically scan the intermediate code which is generated by -fdump-rtl-final to check the expected access mode. That should work for all targets, even if they have different assembler syntax. Thanks Bernd.
Re: fix math wrt volatile-bitfields vs C++ model
> Looks ok to me, but can you add a testcase please? > > Also check if 4.9 is affected. Sorry for the delay, this finally made it back to the top of my to-do list. Testcase included which fails without and passes with this patch. 4.9 is affected and the same patch fixes it. Tested on rx-elf, x86 32/64, and arm32. 2014-10-29 DJ Delorie * expmed.c (strict_volatile_bitfield_p): Fix off-by-one error. 2014-10-29 DJ Delorie * gcc.dg/20141029-1.c: New. Index: expmed.c === --- expmed.c(revision 216811) +++ expmed.c(working copy) @@ -454,13 +454,13 @@ strict_volatile_bitfield_p (rtx op0, uns && bitnum % GET_MODE_ALIGNMENT (fieldmode) + bitsize > modesize)) return false; /* Check for cases where the C++ memory model applies. */ if (bitregion_end != 0 && (bitnum - bitnum % modesize < bitregion_start - || bitnum - bitnum % modesize + modesize > bitregion_end)) + || bitnum - bitnum % modesize + modesize - 1 > bitregion_end)) return false; return true; } /* Return true if OP is a memory and if a bitfield of size BITSIZE at Index: testsuite/gcc.dg/20141029-1.c === --- testsuite/gcc.dg/20141029-1.c (revision 0) +++ testsuite/gcc.dg/20141029-1.c (revision 0) @@ -0,0 +1,28 @@ +/* { dg-do compile } */ +/* { dg-options "-fstrict-volatile-bitfields -fdump-rtl-final" } */ + +#define PERIPH (*(volatile struct system_periph *)0x81234) + +struct system_periph { + union { +unsigned short WORD; +struct { + unsigned short a:1; + unsigned short b:1; + unsigned short :5; + unsigned short c:1; + unsigned short :8; +} BIT; + } ALL; +}; + +void +foo() +{ + while (1) +{ + PERIPH.ALL.BIT.a = 1; +} +} +/* { dg-final { scan-rtl-dump-times "mem/v(/.)*:HI" 4 "final" } } */ +/* { dg-final { cleanup-rtl-dump "final" } } */
Re: fix math wrt volatile-bitfields vs C++ model
On Wed, Oct 29, 2014 at 6:24 AM, DJ Delorie wrote: > >> Looks ok to me, but can you add a testcase please? >> >> Also check if 4.9 is affected. > > Sorry for the delay, this finally made it back to the top of my to-do > list. Testcase included which fails without and passes with this > patch. 4.9 is affected and the same patch fixes it. Tested on > rx-elf, x86 32/64, and arm32. Ok. For the branch please wait until after 4.9.2 is out. Thanks, Richard. > 2014-10-29 DJ Delorie > > * expmed.c (strict_volatile_bitfield_p): Fix off-by-one error. > > 2014-10-29 DJ Delorie > > * gcc.dg/20141029-1.c: New. > > > Index: expmed.c > === > --- expmed.c(revision 216811) > +++ expmed.c(working copy) > @@ -454,13 +454,13 @@ strict_volatile_bitfield_p (rtx op0, uns > && bitnum % GET_MODE_ALIGNMENT (fieldmode) + bitsize > modesize)) > return false; > >/* Check for cases where the C++ memory model applies. */ >if (bitregion_end != 0 >&& (bitnum - bitnum % modesize < bitregion_start > - || bitnum - bitnum % modesize + modesize > bitregion_end)) > + || bitnum - bitnum % modesize + modesize - 1 > bitregion_end)) > return false; > >return true; > } > > /* Return true if OP is a memory and if a bitfield of size BITSIZE at > > Index: testsuite/gcc.dg/20141029-1.c > === > --- testsuite/gcc.dg/20141029-1.c (revision 0) > +++ testsuite/gcc.dg/20141029-1.c (revision 0) > @@ -0,0 +1,28 @@ > +/* { dg-do compile } */ > +/* { dg-options "-fstrict-volatile-bitfields -fdump-rtl-final" } */ > + > +#define PERIPH (*(volatile struct system_periph *)0x81234) > + > +struct system_periph { > + union { > +unsigned short WORD; > +struct { > + unsigned short a:1; > + unsigned short b:1; > + unsigned short :5; > + unsigned short c:1; > + unsigned short :8; > +} BIT; > + } ALL; > +}; > + > +void > +foo() > +{ > + while (1) > +{ > + PERIPH.ALL.BIT.a = 1; > +} > +} > +/* { dg-final { scan-rtl-dump-times "mem/v(/.)*:HI" 4 "final" } } */ > +/* { dg-final { cleanup-rtl-dump "final" } } */
Re: fix math wrt volatile-bitfields vs C++ model
> Ok. For the branch please wait until after 4.9.2 is out. Thanks! Committed to trunk.
Re: fix math wrt volatile-bitfields vs C++ model
> Ok. For the branch please wait until after 4.9.2 is out. 4.9.2 being out, I applied this to the branch.