[Bug c++/113599] New: Wrong computation of member offset through pointer-to-member

[Bug c++/113599] New: Wrong computation of member offset through pointer-to-member

[simon.marchi at polymtl dot ca]

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

            Bug ID: 113599
           Summary: Wrong computation of member offset through
                    pointer-to-member
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: simon.marchi at polymtl dot ca
  Target Milestone: ---

This commit:

  c++: non-dependent .* operand folding [PR112427] 
  https://gitlab.com/gnutools/gcc/-/commit/d3f48f68227

seems to cause a regression in some cases when computing the address of a
member using a pointer-to-member.

Origin: https://sourceware.org/bugzilla/show_bug.cgi?id=31281

I made this reproducer, which was then made more minimal by Tom de Vries.  We
get the address of the thread_info::node field in two different ways.  One
using through a pointer-to-member, the other is getting its address directly. 
We expect both to be equal.  gcc 14/trunk gets it wrong.

...
struct intrusive_list_node {
  void *next;
};

struct dummy {
  void *base;
};

struct thread_info : public dummy, public intrusive_list_node {
  intrusive_list_node node;
};

static thread_info ti;

int main (void) {
  auto thread_info::*MemberNode = &thread_info::node;
  auto node_ptr_1 = &(ti.*MemberNode);
  auto node_ptr_2 = &ti.node;
  return !(node_ptr_1 == node_ptr_2);
}
...

gcc-13:
...
$ g++ test.cpp; ./a.out; echo $?
0
...

gcc-14:
...
$ g++ test.cpp; ./a.out; echo $?
1
...

[Bug c++/113599] [14 Regression] Wrong computation of member offset through pointer-to-member since r14-5503

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2024-01-25
   Target Milestone|---                         |14.0
           Priority|P3                          |P1
            Version|unknown                     |14.0
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
            Summary|Wrong computation of member |[14 Regression] Wrong
                   |offset through              |computation of member
                   |pointer-to-member           |offset through
                   |                            |pointer-to-member since
                   |                            |r14-5503
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |ppalka at gcc dot gnu.org

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r14-5503-gd3f48f682271ed94ab6e9f6bc62418a62bd8ff26
Slightly shortened testcase which aborts on error:
struct A { void *a; };
struct B { void *b; };
struct C : public B, public A { A c; };
static C d;

int
main ()
{
  auto C::*e = &C::c;
  auto f = &(d.*e);
  auto g = &d.c;
  if (f != g)
    __builtin_abort ();
}

[Bug c++/113599] [14 Regression] Wrong computation of member offset through pointer-to-member since r14-5503

[vries at gcc dot gnu.org]

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

Tom de Vries <vries at gcc dot gnu.org> changed:

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

--- Comment #2 from Tom de Vries <vries at gcc dot gnu.org> ---
FWIW, the inherit order is relevant, after applying this change we get the
expected result:
...
-struct thread_info : public dummy, public intrusive_list_node {
+struct thread_info : public intrusive_list_node, public dummy {
...

This could be used as workaround.

[Bug c++/113599] [14 Regression] Wrong computation of member offset through pointer-to-member since r14-5503

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The problem is in that typeck2.cc change:
-      datum = fold_build_pointer_plus (fold_convert (ptype, datum),
component);
+      datum = cp_convert (ptype, datum, complain);
+      if (!processing_template_decl)
+       datum = build2 (POINTER_PLUS_EXPR, ptype,
+                       datum, convert_to_ptrofftype (component));
datum is &d and ptype is A * from the #c1 testcase.
So, previously, we would correctly build (and fold) a POINTER_PLUS_EXPR of
(A *) &d and (sizetype) e (where e is 16, offsetof C::c in C).
But, because C has A base, cp_convert of &d to A * doesn't create (A *) &d, but
returns &d.D.2800 where D.2800 is the A base in C, which is at offset 8 in the
structure.  If we add to that 16, we get something at offset 24 rather than the
offset 16.
I understand the desire to get some error checking on the pointer to pointer
conversion, but cp_convert in this case calls cp_convert_pointer which does the
base lookups and build_base_path etc.
If no checking is needed, then it could be just datum = build_nop (ptype,
datum);
if we don't want folding.

[Bug c++/113599] [14 Regression] Wrong computation of member offset through pointer-to-member since r14-5503

[jakub at gcc dot gnu.org]

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

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

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

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

I'd go with this.

[Bug c++/113599] [14 Regression] Wrong computation of member offset through pointer-to-member since r14-5503

[ppalka at gcc dot gnu.org]

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

--- Comment #5 from Patrick Palka <ppalka at gcc dot gnu.org> ---
D'oh, sorry for the breakage.

(In reply to Jakub Jelinek from comment #3)
> If no checking is needed, then it could be just datum = build_nop (ptype,
> datum);
> if we don't want folding.

Makes sense to me.  We already checked that the object type types is compatible
with the poniter-to-member class type via lookup_base, so a simple cast without
any extra checking should suffice here.  That's what calling 'convert' instead
of 'cp_convert' would have done too.

[Bug c++/113599] [14 Regression] Wrong computation of member offset through pointer-to-member since r14-5503

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from GCC 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:fd620bd3351c6b9821c299035ed17e655d7954b5

commit r14-8439-gfd620bd3351c6b9821c299035ed17e655d7954b5
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Jan 26 00:08:36 2024 +0100

    c++: Fix up build_m_component_ref [PR113599]

    The following testcase reduced from GDB is miscompiled starting with
    r14-5503 PR112427 change.
    The problem is in the build_m_component_ref hunk, which changed
    -      datum = fold_build_pointer_plus (fold_convert (ptype, datum),
component);
    +      datum = cp_convert (ptype, datum, complain);
    +      if (!processing_template_decl)
    +       datum = build2 (POINTER_PLUS_EXPR, ptype,
    +                       datum, convert_to_ptrofftype (component));
    +      datum = cp_fully_fold (datum);
    Component is e, (sizetype) e is 16, offset of c inside of C.
    ptype is A *, pointer to type of C::c and datum is &d.
    Now, previously the above created ((A *) &d) p+ (sizetype) e which is
correct,
    but in the new code cp_convert sees that C has A as base class and
    instead of returning (A *) &d, it returns &d.D.2800 where D.2800 is
    the FIELD_DECL for the A base at offset 8 into C.
    So, instead of computing ((A *) &d) p+ (sizetype) e it computes
    &d.D.2800 p+ (sizetype) e, which is ((A *) &d) p+ 24.

    The following patch fixes it by using convert instead of cp_convert which
    eventually calls build_nop (ptype, datum).

    2024-01-26  Jakub Jelinek  <jakub@redhat.com>

            PR c++/113599
            * typeck2.cc (build_m_component_ref): Use convert instead of
            cp_convert for pointer conversion.

            * g++.dg/expr/ptrmem11.C: New test.

[Bug c++/113599] [14 Regression] Wrong computation of member offset through pointer-to-member since r14-5503

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.