diff mbox series

c++: Fix up build_m_component_ref [PR113599]

Message ID ZbKzMNlUUNpt/SbL@tucnak
State Unresolved
Headers
Series c++: Fix up build_m_component_ref [PR113599] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Jan. 25, 2024, 7:14 p.m. UTC 
  Hi!

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).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2024-01-25  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.


	Jakub

Comments

Patrick Palka Jan. 25, 2024, 7:33 p.m. UTC | #1 
On Thu, 25 Jan 2024, Jakub Jelinek wrote:

> Hi!
> 
> 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).

LGTM.  IIUC using 'convert' would work here too thanks to its special
case for indirect types, but I naively went with cp_convert since it has
a complain parameter :/

> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2024-01-25  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.
> 
> --- gcc/cp/typeck2.cc.jj	2024-01-03 12:01:23.672476417 +0100
> +++ gcc/cp/typeck2.cc	2024-01-25 14:11:40.656361310 +0100
> @@ -2378,7 +2378,7 @@ build_m_component_ref (tree datum, tree
>        /* Build an expression for "object + offset" where offset is the
>  	 value stored in the pointer-to-data-member.  */
>        ptype = build_pointer_type (type);
> -      datum = cp_convert (ptype, datum, complain);
> +      datum = convert (ptype, datum);
>        if (!processing_template_decl)
>  	datum = build2 (POINTER_PLUS_EXPR, ptype,
>  			datum, convert_to_ptrofftype (component));
> --- gcc/testsuite/g++.dg/expr/ptrmem11.C.jj	2024-01-25 14:13:11.736089567 +0100
> +++ gcc/testsuite/g++.dg/expr/ptrmem11.C	2024-01-25 14:18:47.720398222 +0100
> @@ -0,0 +1,17 @@
> +// PR c++/113599
> +// { dg-do run }
> +
> +struct A { void *a; };
> +struct B { void *b; };
> +struct C : public B, public A { A c; };
> +static C d;
> +
> +int
> +main ()
> +{
> +  A C::*e = &C::c;
> +  A *f = &(d.*e);
> +  A *g = &d.c;
> +  if (f != g)
> +    __builtin_abort ();
> +}
> 
> 	Jakub
> 
>
Patrick Palka Jan. 25, 2024, 7:37 p.m. UTC | #2 
On Thu, 25 Jan 2024, Patrick Palka wrote:

> On Thu, 25 Jan 2024, Jakub Jelinek wrote:
> 
> > Hi!
> > 
> > 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).
> 
> LGTM.  IIUC using 'convert' would work here too thanks to its special
> case for indirect types, but I naively went with cp_convert since it has
> a complain parameter :/

D'oh, I completely overlooked this version of the patch already uses
convert instead of build_nop directly :)  LGTM either way

> 
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > 2024-01-25  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.
> > 
> > --- gcc/cp/typeck2.cc.jj	2024-01-03 12:01:23.672476417 +0100
> > +++ gcc/cp/typeck2.cc	2024-01-25 14:11:40.656361310 +0100
> > @@ -2378,7 +2378,7 @@ build_m_component_ref (tree datum, tree
> >        /* Build an expression for "object + offset" where offset is the
> >  	 value stored in the pointer-to-data-member.  */
> >        ptype = build_pointer_type (type);
> > -      datum = cp_convert (ptype, datum, complain);
> > +      datum = convert (ptype, datum);
> >        if (!processing_template_decl)
> >  	datum = build2 (POINTER_PLUS_EXPR, ptype,
> >  			datum, convert_to_ptrofftype (component));
> > --- gcc/testsuite/g++.dg/expr/ptrmem11.C.jj	2024-01-25 14:13:11.736089567 +0100
> > +++ gcc/testsuite/g++.dg/expr/ptrmem11.C	2024-01-25 14:18:47.720398222 +0100
> > @@ -0,0 +1,17 @@
> > +// PR c++/113599
> > +// { dg-do run }
> > +
> > +struct A { void *a; };
> > +struct B { void *b; };
> > +struct C : public B, public A { A c; };
> > +static C d;
> > +
> > +int
> > +main ()
> > +{
> > +  A C::*e = &C::c;
> > +  A *f = &(d.*e);
> > +  A *g = &d.c;
> > +  if (f != g)
> > +    __builtin_abort ();
> > +}
> > 
> > 	Jakub
> > 
> > 
>
Jason Merrill Jan. 25, 2024, 11:04 p.m. UTC | #3 
On 1/25/24 14:14, Jakub Jelinek wrote:
> Hi!
> 
> 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).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK

> 2024-01-25  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.
> 
> --- gcc/cp/typeck2.cc.jj	2024-01-03 12:01:23.672476417 +0100
> +++ gcc/cp/typeck2.cc	2024-01-25 14:11:40.656361310 +0100
> @@ -2378,7 +2378,7 @@ build_m_component_ref (tree datum, tree
>         /* Build an expression for "object + offset" where offset is the
>   	 value stored in the pointer-to-data-member.  */
>         ptype = build_pointer_type (type);
> -      datum = cp_convert (ptype, datum, complain);
> +      datum = convert (ptype, datum);
>         if (!processing_template_decl)
>   	datum = build2 (POINTER_PLUS_EXPR, ptype,
>   			datum, convert_to_ptrofftype (component));
> --- gcc/testsuite/g++.dg/expr/ptrmem11.C.jj	2024-01-25 14:13:11.736089567 +0100
> +++ gcc/testsuite/g++.dg/expr/ptrmem11.C	2024-01-25 14:18:47.720398222 +0100
> @@ -0,0 +1,17 @@
> +// PR c++/113599
> +// { dg-do run }
> +
> +struct A { void *a; };
> +struct B { void *b; };
> +struct C : public B, public A { A c; };
> +static C d;
> +
> +int
> +main ()
> +{
> +  A C::*e = &C::c;
> +  A *f = &(d.*e);
> +  A *g = &d.c;
> +  if (f != g)
> +    __builtin_abort ();
> +}
> 
> 	Jakub
>
diff mbox series

Patch

--- gcc/cp/typeck2.cc.jj	2024-01-03 12:01:23.672476417 +0100
+++ gcc/cp/typeck2.cc	2024-01-25 14:11:40.656361310 +0100
@@ -2378,7 +2378,7 @@  build_m_component_ref (tree datum, tree
       /* Build an expression for "object + offset" where offset is the
 	 value stored in the pointer-to-data-member.  */
       ptype = build_pointer_type (type);
-      datum = cp_convert (ptype, datum, complain);
+      datum = convert (ptype, datum);
       if (!processing_template_decl)
 	datum = build2 (POINTER_PLUS_EXPR, ptype,
 			datum, convert_to_ptrofftype (component));
--- gcc/testsuite/g++.dg/expr/ptrmem11.C.jj	2024-01-25 14:13:11.736089567 +0100
+++ gcc/testsuite/g++.dg/expr/ptrmem11.C	2024-01-25 14:18:47.720398222 +0100
@@ -0,0 +1,17 @@ 
+// PR c++/113599
+// { dg-do run }
+
+struct A { void *a; };
+struct B { void *b; };
+struct C : public B, public A { A c; };
+static C d;
+
+int
+main ()
+{
+  A C::*e = &C::c;
+  A *f = &(d.*e);
+  A *g = &d.c;
+  if (f != g)
+    __builtin_abort ();
+}