[Bug c++/109039] New: Miscompilation with no_unique_address and bitfields

[Bug c++/109039] New: Miscompilation with no_unique_address and bitfields

[coillol at yandex dot ru]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109039

            Bug ID: 109039
           Summary: Miscompilation with no_unique_address and bitfields
           Product: gcc
           Version: 12.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: coillol at yandex dot ru
  Target Milestone: ---

g++ miscompiles the following code starting from version 12.1 (-std=c++20, any
optimization mode).


```
#include <cassert>

struct X {
    short x0 : 7;
    short x1 : 8;

    X() { init(); assert(get() == 3); }

    void init() { x0 = 1; x1 = 2; }

    int get() { return x0 + x1; }
};

struct S {
    [[no_unique_address]] X x;
    char c = 0;
};

int main() {
    S s;
    assert(s.x.get() == 3);
    return 0;
}
```

Program hits assertion in main when executed. Links to godbolt:
https://godbolt.org/z/xG186673P (12.1), https://godbolt.org/z/MaP56rfhe
(trunk).

[Bug c++/109039] [12/13 Regression] Miscompilation with no_unique_address and bitfields

[ppalka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109039

Patrick Palka <ppalka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Miscompilation with         |[12/13 Regression]
                   |no_unique_address and       |Miscompilation with
                   |bitfields                   |no_unique_address and
                   |                            |bitfields
      Known to fail|                            |12.2.0, 13.0
                 CC|                            |jason at gcc dot gnu.org,
                   |                            |ppalka at gcc dot gnu.org
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=103681
   Target Milestone|---                         |12.3
      Known to work|                            |11.3.0

--- Comment #1 from Patrick Palka <ppalka at gcc dot gnu.org> ---
Started with r12-6028-ga37e8ce3b66325, before which sizeof(S) == 4 and now
sizeof(S) == 2

[Bug c++/109039] [12/13 Regression] Miscompilation with no_unique_address and bitfields

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109039

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

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

[Bug c++/109039] [12/13 Regression] Miscompilation with no_unique_address and bitfields

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109039

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
And there doesn't even have to be any NSDMI, even
struct X {
  short x0 : 7;
  short x1 : 8;
  X () : x0 (1), x1 (2) {}
  int get () { return x0 + x1; }
};

struct S {
  [[no_unique_address]] X x;
  char c;
  S () : c (0) {}
};

S s;

int
main ()
{
  if (s.x.x0 != 1 || s.x.x1 != 2 || s.c != 0)
    __builtin_abort ();
}
is miscompiled since that revision.  With sizeof (s) == 2 there aren't enough
bytes to
fit the 15 bits of bitfields and another byte.  S::x.x0 is on x86_64 at offset
0, low 7
bits, then S::x.x1 next 8 bits, and S::c is at offset 1, so overlapping the
high 7 bits of S::x.x1.

[Bug c++/109039] [12/13 Regression] Miscompilation with no_unique_address and bitfields

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109039

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think the bug is in class.cc (end_of_class), which does:
        offset = size_binop (PLUS_EXPR, byte_position (field), size);
That works fine for non bit-fields, but for bit-fields it is sometimes
incorrect.
In particular, X::x1 has DECL_SIZE 8, DECL_SIZE_UNIT 1, DECL_FIELD_OFFSET 0,
DECL_FIELD_BIT_OFFSET 7.  So, byte_position (field) is still 0, and size 1,
therefore offset is set to 1, even when the bit-field occupies just 1 bit in
the first
byte and 7 in the second byte, so I think we want to set offset to 2 in this
case.

The Itanium ABI says:
In either case, update dsize(C) to include the last byte containing (part of)
the bit-field, and update sizeof(C) to max(sizeof(C),dsize(C)). 
So, I think for bit-fields we instead want to sum bit_position (field) and
DECL_SIZE and CEIL_DIV_EXPR it by bitsize_unit_node and cast to sizetype.

[Bug c++/109039] [12/13 Regression] Miscompilation with no_unique_address and bitfields

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109039

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-03-09
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 54625
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54625&action=edit
gcc13-pr109039.patch

Untested fix.

[Bug c++/109039] [12/13 Regression] Miscompilation with no_unique_address and bitfields

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109039

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:991f9eb2da3a268b7b08346761fa0078ab55f506

commit r13-6596-g991f9eb2da3a268b7b08346761fa0078ab55f506
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Mar 10 20:36:33 2023 +0100

    c++, abi: Fix up class layout with bitfields [PR109039]

    The following testcase FAILs, because starting with r12-6028
    the S class has only 2 bytes, not enough to hold one 7-bit bitfield, one
8-bit
    bitfield and one 8-bit char field.

    The reason is that when end_of_class attempts to compute dsize, it simply
    adds byte_position of the field and DECL_SIZE_UNIT (and uses maximum from
    those offsets).
    The problematic bit-field in question has bit_position 7, byte_position 0,
    DECL_SIZE 8 and DECL_SIZE_UNIT 1.  So, byte_position + DECL_SIZE_UNIT is
    1, even when the bitfield only has a single bit in the first byte and 7
    further bits in the second byte, so per the Itanium ABI it should be 2:
    "In either case, update dsize(C) to include the last byte
    containing (part of) the bit-field, and update sizeof(C) to
    max(sizeof(C),dsize(C))."

    The following patch fixes it by computing bitsize of the end and using
    CEIL_DIV_EXPR division to round it to next byte boundary and convert
    from bits to bytes.

    While this is an ABI change, classes with such incorrect layout couldn't
    have worked properly, so I doubt anybody is actually running it often
    in the wild.  Thus I think adding some ABI warning for it is unnecessary.

    2023-03-10  Jakub Jelinek  <jakub@redhat.com>

            PR c++/109039
            * class.cc (end_of_class): For bit-fields, instead of computing
            offset as sum of byte_position (field) and DECL_SIZE_UNIT (field),
            compute it as sum of bit_position (field) and DECL_SIZE (field)
            divided by BITS_PER_UNIT rounded up.

            * g++.dg/abi/no_unique_address7.C: New test.

[Bug c++/109039] [12 Regression] Miscompilation with no_unique_address and bitfields

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109039

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[12/13 Regression]          |[12 Regression]
                   |Miscompilation with         |Miscompilation with
                   |no_unique_address and       |no_unique_address and
                   |bitfields                   |bitfields

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.

[Bug c++/109039] [12 Regression] Miscompilation with no_unique_address and bitfields

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109039

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:794e4f69cf2434c82388820f78c0e9e86fdd677b

commit r12-9288-g794e4f69cf2434c82388820f78c0e9e86fdd677b
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Mar 10 20:36:33 2023 +0100

    c++, abi: Fix up class layout with bitfields [PR109039]

    The following testcase FAILs, because starting with r12-6028
    the S class has only 2 bytes, not enough to hold one 7-bit bitfield, one
8-bit
    bitfield and one 8-bit char field.

    The reason is that when end_of_class attempts to compute dsize, it simply
    adds byte_position of the field and DECL_SIZE_UNIT (and uses maximum from
    those offsets).
    The problematic bit-field in question has bit_position 7, byte_position 0,
    DECL_SIZE 8 and DECL_SIZE_UNIT 1.  So, byte_position + DECL_SIZE_UNIT is
    1, even when the bitfield only has a single bit in the first byte and 7
    further bits in the second byte, so per the Itanium ABI it should be 2:
    "In either case, update dsize(C) to include the last byte
    containing (part of) the bit-field, and update sizeof(C) to
    max(sizeof(C),dsize(C))."

    The following patch fixes it by computing bitsize of the end and using
    CEIL_DIV_EXPR division to round it to next byte boundary and convert
    from bits to bytes.

    While this is an ABI change, classes with such incorrect layout couldn't
    have worked properly, so I doubt anybody is actually running it often
    in the wild.  Thus I think adding some ABI warning for it is unnecessary.

    2023-03-10  Jakub Jelinek  <jakub@redhat.com>

            PR c++/109039
            * class.cc (end_of_class): For bit-fields, instead of computing
            offset as sum of byte_position (field) and DECL_SIZE_UNIT (field),
            compute it as sum of bit_position (field) and DECL_SIZE (field)
            divided by BITS_PER_UNIT rounded up.

            * g++.dg/abi/no_unique_address7.C: New test.

    (cherry picked from commit 991f9eb2da3a268b7b08346761fa0078ab55f506)

[Bug c++/109039] [12 Regression] Miscompilation with no_unique_address and bitfields

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109039

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 12.3 too.