[Bug c++/102490] New: Erroneous optimization of default constexpr operator== of struct with bitfields

[Bug c++/102490] New: Erroneous optimization of default constexpr operator== of struct with bitfields

[luc.briand35 at gmail dot com]

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

            Bug ID: 102490
           Summary: Erroneous optimization of default constexpr operator==
                    of struct with bitfields
           Product: gcc
           Version: 10.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: luc.briand35 at gmail dot com
  Target Milestone: ---

Hello,

For the following code, gcc version 10 and up wrongly optimizes the default
operator==().
This occurs for O1 and up.
Removing the 'constexpr' qualifier fixes everything.
The size of the bitfields doesn't matter.
No warnings are appear with "-Wall -Wextra".


Godbolt link: https://gcc.godbolt.org/z/j4fG3sKze


struct A
{
    unsigned char foo : 1;
    unsigned char bar : 1;

    constexpr bool operator==(const A&) const = default;
};


int main()
{
    A a{}, b{};

    a.bar = 0b1;

    return a == b;
}



With the options "-std=c++2a -O1", the assembly generated is simply:
main:
        mov     eax, 1
        ret



In this similar example, we can see that the generated assembly for the
equality operator ignores the 'bar' bitfield (Godbolt link:
https://gcc.godbolt.org/z/3K75xx1on) :


struct A
{
    unsigned char foo : 3;
    unsigned char bar : 1;

    constexpr bool operator==(const A&) const = default;
};

void change(A& a);

int main()
{
    A a{}, b{};

    change(a);

    return a == b;
}


The assembly for gcc version 10.X and 11.X is a bit different, but have the
same problem.

[Bug c++/102490] Erroneous optimization of default constexpr operator== of struct with bitfields

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
                 CC|                            |jakub at gcc dot gnu.org
   Last reconfirmed|                            |2021-09-27
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Before r10-4397-gb7689b962dd6536bbb2567bfdec52e35109af7ab , this has been
rejected with:
pr102490.C:5:48: error: ‘constexpr bool A::operator==(const A&) const’ cannot
be defaulted
    5 |   constexpr bool operator== (const A&) const = default;
      |                                                ^~~~~~~
pr102490.C:5:18: warning: inline function ‘constexpr bool A::operator==(const
A&) const’ used but never defined
    5 |   constexpr bool operator== (const A&) const = default;
      |                  ^~~~~~~~
but since then it emits:
pr102490.C: In member function ‘constexpr bool A::operator==(const A&) const’:
pr102490.C:5:18: error: type mismatch in ‘component_ref’
    5 |   constexpr bool operator== (const A&) const = default;
      |                  ^~~~~~~~
unsigned char

<unnamed-unsigned:1>

_1 = this->foo;
pr102490.C:5:18: error: type mismatch in ‘component_ref’
unsigned char

<unnamed-unsigned:1>

_2 = D.2080->foo;
pr102490.C:5:18: error: type mismatch in ‘component_ref’
unsigned char

<unnamed-unsigned:1>

_3 = this->bar;
pr102490.C:5:18: error: type mismatch in ‘component_ref’
unsigned char

<unnamed-unsigned:1>

_4 = D.2080->bar;
pr102490.C:5:18: internal compiler error: ‘verify_gimple’ failed
at -O0 or miscompiles it at -O1.

[Bug c++/102490] Erroneous optimization of default constexpr operator== of struct with bitfields

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The bug is that the operator== is synthetized before the bitfields are adjusted
to the middle-end way, but assumes they are already adjusted that way.
In particular, the synthetization is done from:
#8  0x0000000000c8fe64 in build_comparison_op (fndecl=<function_decl
0x7fffea1a5500 operator==>, complain=0) at ../../gcc/cp/method.c:1454
#9  0x0000000000c91895 in synthesize_method (fndecl=<function_decl
0x7fffea1a5500 operator==>) at ../../gcc/cp/method.c:1779
#10 0x0000000000c98e1a in defaulted_late_check (fn=<function_decl
0x7fffea1a5500 operator==>) at ../../gcc/cp/method.c:3168
#11 0x0000000000b10439 in check_bases_and_members (t=<record_type
0x7fffea1a1b28 A>) at ../../gcc/cp/class.c:6136
#12 0x0000000000b161c8 in finish_struct_1 (t=<record_type 0x7fffea1a1b28 A>) at
../../gcc/cp/class.c:7372
#13 0x0000000000b17d3d in finish_struct (t=<record_type 0x7fffea1a1b28 A>,
attributes=<tree 0x0>) at ../../gcc/cp/class.c:7668
but the bitfields are adjusted in
#0  layout_class_type (t=<record_type 0x7fffea1a1b28 A>,
virtuals_p=0x7fffffffcef0) at ../../gcc/cp/class.c:6711
#1  0x0000000000b162ae in finish_struct_1 (t=<record_type 0x7fffea1a1b28 A>) at
../../gcc/cp/class.c:7397
#2  0x0000000000b17d3d in finish_struct (t=<record_type 0x7fffea1a1b28 A>,
attributes=<tree 0x0>) at ../../gcc/cp/class.c:7668
So, we emit e.g. the COMPONENT_REFs with the underlying bitfield type, but then
layout_class_type changes the type of the FIELD_DECLs to something else and
we'd 
instead want to either compare in the actual bitfield types, or on NOP_EXPRs
from the COMPONENT_REFs to the underlying type.

So, either build_comparison_op should check for DECL_C_BIT_FIELD fields whether
TREE_TYPE of the field has the DECL_SIZE precision or not (but we'd also need
to handle there e.g. the
      /* If this field is a bit-field whose width is greater than its
         type, then there are some special rules for allocating
         it.  */
      if (DECL_C_BIT_FIELD (field)
          && tree_int_cst_lt (TYPE_SIZE (type), DECL_SIZE (field)))
stuff from layout_class_type (as well as the:
      /* The middle end uses the type of expressions to determine the
         possible range of expression values.  In order to optimize
         "x.i > 7" to "false" for a 2-bit bitfield "i", the middle end
         must be made aware of the width of "i", via its type.

         Because C++ does not have integer types of arbitrary width,
         we must (for the purposes of the front end) convert from the
         type assigned here to the declared type of the bitfield
         whenever a bitfield expression is used as an rvalue.
         Similarly, when assigning a value to a bitfield, the value
         must be converted to the type given the bitfield here.  */
      if (DECL_C_BIT_FIELD (field))
), or we need to somehow make sure that the == and <=> methods are synthetized
only after that happens.  Jason, thoughts on that?

Also, wonder about :0 bitfields in classes that have defaulted comparisons...

[Bug c++/102490] Erroneous optimization of default constexpr operator== of struct with bitfields

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 102488 has been marked as a duplicate of this bug. ***

[Bug c++/102490] Erroneous optimization of default constexpr operator== of struct with bitfields

[jakub at gcc dot gnu.org]

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

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

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

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

Untested fix.

[Bug c++/102490] Erroneous optimization of default constexpr operator== of struct with bitfields

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Latest patch version
https://gcc.gnu.org/pipermail/gcc-patches/2021-October/580706.html
Jason was kind enough to take it over as more work needs to be done, e.g. to
make  the synthesization work on binfos rather than base FIELD_DECLs that might
not exist yet.

[Bug c++/102490] Erroneous optimization of default constexpr operator== of struct with bitfields

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:09d886e671f2230acca082e6579a69b8df8fb202

commit r12-4202-g09d886e671f2230acca082e6579a69b8df8fb202
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Oct 1 17:07:17 2021 +0200

    c++: defaulted <=> with bitfields [PR102490]

    The testcases in the patch are either miscompiled or ICE with checking,
    because the defaulted operator== is synthesized too early (but only if
    constexpr), when the corresponding class type is still incomplete type. 
The
    problem is that at that point the bitfield FIELD_DECLs still have as
    TREE_TYPE their underlying type rather than integral type with their
    precision and when layout_class_type is called for the class soon after
    that, it changes those types but the COMPONENT_REFs type stay the way that
    they were during the operator== synthesize_method type and the middle-end
is
    then upset by the mismatch of types.  As what exact type will be given
isn't
    just a one liner but quite long code especially for over-sized bitfields, I
    think it is best to just not synthesize the comparison operators so early
    and call defaulted_late_check for them once again as soon as the class is
    complete.

    This is also a problem for virtual operator<=>, where we need to compare
the
    noexcept-specifier to validate the override before the class is complete.
    Rather than try to defer that comparison, maybe_instantiate_noexcept now
    calls maybe_synthesize_method, which calls build_comparison_op in
    non-defining mode if the class isn't complete yet.  In that situation we
    also might not have base fields yet, so we look in the binfo for the bases.

    Co-authored-by: Jason Merrill <jason@redhat.com>

    2021-10-01  Jakub Jelinek  <jakub@redhat.com>

            PR c++/98712
            PR c++/102490
            * cp-tree.h (maybe_synthesize_method): Declare.
            * method.c (genericize_spaceship): Use
            LOOKUP_NORMAL | LOOKUP_NONVIRTUAL | LOOKUP_DEFAULTED instead of
            LOOKUP_NORMAL for flags.
            (comp_info): Remove defining member.  Add complain, code, retcat.
            (comp_info::comp_info): Adjust.
            (do_one_comp): Split out from build_comparison_op.   Use
            LOOKUP_NORMAL | LOOKUP_NONVIRTUAL | LOOKUP_DEFAULTED instead of
            LOOKUP_NORMAL for flags.
            (build_comparison_op): Add defining argument. Adjust comp_info
            construction.  Use defining instead of info.defining.  Assert that
            if defining, ctype is a complete type.  Walk base binfos.
            (synthesize_method, maybe_explain_implicit_delete,
            explain_implicit_non_constexpr): Adjust build_comparison_op
callers.
            (maybe_synthesize_method): New function.
            * class.c (check_bases_and_members): Don't call
defaulted_late_check
            for sfk_comparison.
            (finish_struct_1): Call it here instead after class has been
            completed.
            * pt.c (maybe_instantiate_noexcept): Call maybe_synthesize_method
            instead of synthesize_method.

            * g++.dg/cpp2a/spaceship-synth8.C (std::strong_ordering): Provide
            more complete definition.
            (std::strong_ordering::less, std::strong_ordering::equal,
            std::strong_ordering::greater): Define.
            * g++.dg/cpp2a/spaceship-synth12.C: New test.
            * g++.dg/cpp2a/spaceship-synth13.C: New test.
            * g++.dg/cpp2a/spaceship-synth14.C: New test.
            * g++.dg/cpp2a/spaceship-eq11.C: New test.
            * g++.dg/cpp2a/spaceship-eq12.C: New test.
            * g++.dg/cpp2a/spaceship-eq13.C: New test.

[Bug c++/102490] Erroneous optimization of default constexpr operator== of struct with bitfields

[ppalka at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
                 CC|                            |ppalka at gcc dot gnu.org
         Resolution|---                         |FIXED
   Target Milestone|---                         |12.0

--- Comment #7 from Patrick Palka <ppalka at gcc dot gnu.org> ---
Thus fixed since GCC 12