diff mbox

fix math wrt volatile-bitfields vs C++ model

Message ID 201406112135.s5BLZYFj030281@greed.delorie.com
State New
Headers show

Commit Message

DJ Delorie June 11, 2014, 9:35 p.m. UTC 
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?

	* expmed.c (strict_volatile_bitfield_p): Fix off-by-one error.

Comments

Richard Biener June 12, 2014, 8:04 a.m. UTC | #1 
On Wed, Jun 11, 2014 at 11:35 PM, DJ Delorie <dj@redhat.com> 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
DJ Delorie June 17, 2014, 2:08 a.m. UTC | #2 
> 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.
Richard Biener June 17, 2014, 8:08 a.m. UTC | #3 
On Tue, Jun 17, 2014 at 4:08 AM, DJ Delorie <dj@redhat.com> 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.
diff mbox

Patch

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