[Bug c/52979] New: likely wrong code bug w/packed bitfields

[Bug c/52979] New: likely wrong code bug w/packed bitfields

[regehr at cs dot utah.edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

             Bug #: 52979
           Summary: likely wrong code bug w/packed bitfields
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: regehr@cs.utah.edu
                CC: chenyang@cs.utah.edu


[regehr@dyson r30]$ current-gcc -O2 small.c ; ./a.out 
0
[regehr@dyson r30]$ current-gcc -O3 small.c ; ./a.out 
1
[regehr@dyson r30]$ cat small.c
int printf (const char *, ...);
int c, d, e;

void fn1 ()
{
}

#pragma pack(1)
struct S1
{
    int f0:31;
    int f1:6;
} 
a = { 1 };

static struct S1 b = { 1 };

void fn2 ()
{
    for (;;)
    {
        a.f1 = 1;
        struct S1 f = { };
        b = f;
        e = 0;
        if (d)
            c = a.f0;
        break;
    }
}

void fn3 ()
{
    fn2 ();
    a = b;
}

int main (void)
{
    fn3 ();
    printf ("%d\n", a.f0);
    return 0;
}
[regehr@dyson r30]$ current-gcc -v
Using built-in specs.
COLLECT_GCC=current-gcc
COLLECT_LTO_WRAPPER=/uusoc/exports/scratch/regehr/z/compiler-install/gcc-r186403-install/bin/../libexec/gcc/x86_64-unknown-linux-gnu/4.8.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: ../configure --with-libelf=/usr/local --enable-lto
--prefix=/home/regehr/z/compiler-install/gcc-r186403-install
--program-prefix=r186403- --enable-languages=c,c++
Thread model: posix
gcc version 4.8.0 20120413 (experimental) (GCC)

[Bug tree-optimization/52979] [4.7/4.8 Regression] likely wrong code bug w/packed bitfields

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
      Known to work|                            |4.6.3
           Keywords|                            |wrong-code
   Last reconfirmed|                            |2012-04-16
          Component|c                           |tree-optimization
     Ever Confirmed|0                           |1
            Summary|likely wrong code bug       |[4.7/4.8 Regression] likely
                   |w/packed bitfields          |wrong code bug w/packed
                   |                            |bitfields
   Target Milestone|---                         |4.7.1
      Known to fail|                            |4.7.0, 4.8.0

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-04-16 09:46:28 UTC ---
Confirmed with -O2 -ftree-vectorize.

[Bug tree-optimization/52979] [4.7/4.8 Regression] likely wrong code bug w/packed bitfields

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-04-23 17:38:43 UTC ---
Doesn't reproduce with r186687 and above (supposedly just went latent with this
testcase).

[Bug tree-optimization/52979] [4.7/4.8 Regression] likely wrong code bug w/packed bitfields

[regehr at cs dot utah.edu]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

--- Comment #3 from John Regehr <regehr at cs dot utah.edu> 2012-04-23 17:47:42 UTC ---
I'll see if I can find another test case for this one.

[Bug tree-optimization/52979] [4.7/4.8 Regression] likely wrong code bug w/packed bitfields

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-04-24 10:47:04 UTC ---
Doesn't apparently need a loop, the following fails on 4.7 branch and older
trunk with -O2 -ftree-vectorize as well.  The dummy foo function is important
to reproduce it, apparently otherwise notice_global_symbol is called on a and
creates DECL_RTL for it while it still has DECL_ALIGN 8.  Later on
vect_compute_data_ref_alignment increases the alignment of a to 128 and sets
DECL_USER_ALIGN.

extern void abort (void);
int c, d, e;

void
foo (void)
{
}

struct __attribute__((packed)) S { int g : 31; int h : 6; };
struct S a = { 1 };
static struct S b = { 1 };

void
bar (void)
{
  a.h = 1;
  struct S f = { };
  b = f;
  e = 0;
  if (d)
    c = a.g;
}

void
baz (void)
{
  bar ();
  a = b;
}

int
main ()
{
  baz ();
  if (a.g)
    abort ();
  return 0;
}

[Bug tree-optimization/52979] [4.7/4.8 Regression] likely wrong code bug w/packed bitfields

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-04-24 11:41:47 UTC ---
This is a kind of dup of PR48124, certainly on the 4.7 branch that doesn't even
try to handle it, and on the trunk (after Honza's changes one needs to swap
struct S a = { 1 };
static struct S b = { 1 };
lines to reproduce) the problem is I guess that DECL_BIT_FIELD_REPRESENTATIVE
is still an int, I guess in packed bitfields we need to use 8-bit type as bit
field representative, or handle those specially in the expansion.

[Bug tree-optimization/52979] [4.7/4.8 Regression] likely wrong code bug w/packed bitfields

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

--- Comment #6 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-04-24 11:57:53 UTC ---
But it uses TYPE_SIZE_UNIT to constrain it which is 5, and maxbitsize 40.
Which then leads to a representative of BLKmode via

  if (mode == VOIDmode)
    {
      /* We really want a BLKmode representative only as a last resort,
         considering the member b in
           struct { int a : 7; int b : 17; int c; } __attribute__((packed));
         Otherwise we simply want to split the representative up
         allowing for overlaps within the bitfield region as required for
           struct { int a : 7; int b : 7;
                    int c : 10; int d; } __attribute__((packed));
         [0, 15] HImode for a and b, [8, 23] HImode for c.  */
      DECL_SIZE (repr) = bitsize_int (bitsize);
      DECL_SIZE_UNIT (repr) = size_int (bitsize / BITS_PER_UNIT);
      DECL_MODE (repr) = BLKmode;
      TREE_TYPE (repr) = build_array_type_nelts (unsigned_char_type_node,
                                                 bitsize / BITS_PER_UNIT);

so - where does it go wrong then?

[Bug tree-optimization/52979] [4.7/4.8 Regression] likely wrong code bug w/packed bitfields

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

--- Comment #7 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-04-24 12:28:24 UTC ---
Btw, get_best_mode for

Breakpoint 6, get_best_mode (bitsize=6, bitpos=31, bitregion_start=0, 
    bitregion_end=39, align=128, largest_mode=DImode, volatilep=0)

happily returns DImode which has more than maxbits (40) bits of size.

I think the C++ memory model work was not sufficiently completed to
handle this case during expansion.

At least

  /* Find the narrowest integer mode that contains the bit field.  */
  for (mode = GET_CLASS_NARROWEST_MODE (MODE_INT); mode != VOIDmode;
       mode = GET_MODE_WIDER_MODE (mode))
    {
      unit = GET_MODE_BITSIZE (mode);
      if (unit == GET_MODE_PRECISION (mode)
          && (bitpos % unit) + bitsize <= unit)
        break;
    }

is overly pessimistic in the (bitpos % unit) + bitsize <= unit check and
does not consider bitpos adjustments.

[Bug tree-optimization/52979] [4.7/4.8 Regression] likely wrong code bug w/packed bitfields

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |jakub at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-04-24 15:34:01 UTC ---
Created attachment 27230
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27230
gcc48-pr52979.patch

Untested fix.

[Bug tree-optimization/52979] [4.7/4.8 Regression] likely wrong code bug w/packed bitfields

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-04-25 14:27:15 UTC ---
Author: jakub
Date: Wed Apr 25 14:27:08 2012
New Revision: 186819

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=186819
Log:
    PR middle-end/52979
    * stor-layout.c (get_best_mode): Don't return mode with bitsize
    larger than maxbits.  Don't compute maxbits modulo align.
    Also check that unit bytes long store at bitpos / unit * unit
    doesn't affect bits beyond bitregion_end.
    * expmed.c (store_bit_field_1): Avoid trying insv if OP_MODE MEM
    would not fit into bitregion_start ... bitregion_end + 1 bit
    region.
    (store_split_bit_field): Decrease unit close to end of bitregion_end
    if access is restricted in order to avoid mutual recursion.

    * gcc.c-torture/compile/pr52979-1.c: New test.
    * gcc.c-torture/execute/pr52979-1.c: New test.
    * gcc.c-torture/execute/pr52979-2.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/compile/pr52979-1.c
    trunk/gcc/testsuite/gcc.c-torture/execute/pr52979-1.c
    trunk/gcc/testsuite/gcc.c-torture/execute/pr52979-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/expmed.c
    trunk/gcc/stor-layout.c
    trunk/gcc/testsuite/ChangeLog

[Bug tree-optimization/52979] [4.7 Regression] likely wrong code bug w/packed bitfields

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |4.8.0
            Summary|[4.7/4.8 Regression] likely |[4.7 Regression] likely
                   |wrong code bug w/packed     |wrong code bug w/packed
                   |bitfields                   |bitfields
      Known to fail|4.8.0                       |

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-04-25 18:36:10 UTC ---
Should be fixed on the trunk.  On the 4.7 branch it needs PR48124 backport.

[Bug tree-optimization/52979] [4.7 Regression] likely wrong code bug w/packed bitfields

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug tree-optimization/52979] [4.7 Regression] likely wrong code bug w/packed bitfields

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

--- Comment #11 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-06-04 10:19:49 UTC ---
Author: rguenth
Date: Mon Jun  4 10:19:44 2012
New Revision: 188176

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=188176
Log:
2012-06-04  Richard Guenther  <rguenther@suse.de>

    Backport from mainline
    2012-04-25  Jakub Jelinek  <jakub@redhat.com>

    PR middle-end/52979
    * stor-layout.c (get_best_mode): Don't return mode with bitsize
    larger than maxbits.  Don't compute maxbits modulo align.
    Also check that unit bytes long store at bitpos / unit * unit
    doesn't affect bits beyond bitregion_end.
    * expmed.c (store_bit_field_1): Avoid trying insv if OP_MODE MEM
    would not fit into bitregion_start ... bitregion_end + 1 bit
    region.
    (store_split_bit_field): Decrease unit close to end of bitregion_end
    if access is restricted in order to avoid mutual recursion.

    * gcc.c-torture/compile/pr52979-1.c: New test.
    * gcc.c-torture/execute/pr52979-1.c: New test.
    * gcc.c-torture/execute/pr52979-2.c: New test.

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.c-torture/compile/pr52979-1.c
    branches/gcc-4_7-branch/gcc/testsuite/gcc.c-torture/execute/pr52979-1.c
    branches/gcc-4_7-branch/gcc/testsuite/gcc.c-torture/execute/pr52979-2.c
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/expmed.c
    branches/gcc-4_7-branch/gcc/stor-layout.c
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog

[Bug tree-optimization/52979] [4.7 Regression] likely wrong code bug w/packed bitfields

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52979

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
      Known to work|                            |4.7.1
         Resolution|                            |FIXED

--- Comment #12 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-06-04 10:23:51 UTC ---
Fixed.