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.
