On Tue, Mar 25, 2025 at 05:18:23PM +0000, Simon Martin wrote:
> We've been miscompiling the following since r0-51314-gd6b4ea8592e338 (I
> did not go compile something that old, and identified this change via
> git blame, so might be wrong)
>
> === cut here ===
> struct Foo { int x; };
> Foo& get (Foo &v) { return v; }
> void bar () {
> Foo v; v.x = 1;
> (true ? get (v) : get (v)).*(&Foo::x) = 2;
> // v.x still equals 1 here...
> }
> === cut here ===
>
> The problem lies in build_m_component_ref, that computes the address of
> the COND_EXPR using build_address to build the representation of
> (true ? get (v) : get (v)).*(&Foo::x);
> and gets something like
> &(true ? get (v) : get (v)) // #1
> instead of
> (true ? &get (v) : &get (v)) // #2
> and the write does not go where want it to, hence the miscompile.
>
> This patch replaces the call to build_address by a call to
> cp_build_addr_expr, which gives #2, that is properly handled.
>
> Successfully tested on x86_64-pc-linux-gnu. OK for trunk? And for active
> branches after 2-3 weeks since it's a nasty one (albeit very old)?
>
> PR c++/114525
>
> gcc/cp/ChangeLog:
>
> * typeck2.cc (build_m_component_ref): Call cp_build_addr_expr
> instead of build_address.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/parse/pr114525.C: New test.
g++.dg/expr/cond18.C seems like a more appropriate place, but the
patch itself LGTM.
>
> ---
> gcc/cp/typeck2.cc | 2 +-
> gcc/testsuite/g++.dg/parse/pr114525.C | 36 +++++++++++++++++++++++++++
> 2 files changed, 37 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.dg/parse/pr114525.C
>
> diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc
> index 1adc05aa86d..45edd180173 100644
> --- a/gcc/cp/typeck2.cc
> +++ b/gcc/cp/typeck2.cc
> @@ -2387,7 +2387,7 @@ build_m_component_ref (tree datum, tree component,
> tsubst_flags_t complain)
> (cp_type_quals (type)
> | cp_type_quals (TREE_TYPE (datum))));
>
> - datum = build_address (datum);
> + datum = cp_build_addr_expr (datum, complain);
>
> /* Convert object to the correct base. */
> if (binfo)
> diff --git a/gcc/testsuite/g++.dg/parse/pr114525.C
> b/gcc/testsuite/g++.dg/parse/pr114525.C
> new file mode 100644
> index 00000000000..326985eed50
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/parse/pr114525.C
> @@ -0,0 +1,36 @@
> +/* PR c++/114525 */
> +/* { dg-do run } */
> +
> +struct Foo {
> + int x;
> +};
> +
> +Foo& get (Foo& v) {
> + return v;
> +}
> +
> +int main () {
> + bool cond = true;
> +
> + /* Testcase from PR; v.x would wrongly remain equal to 1. */
> + Foo v_ko;
> + v_ko.x = 1;
> + (cond ? get (v_ko) : get (v_ko)).*(&Foo::x) = 2;
> + if (v_ko.x != 2)
> + __builtin_abort ();
> +
> + /* Those would already work, i.e. x be changed to 2. */
> + Foo v_ok_1;
> + v_ok_1.x = 1;
> + (cond ? get (v_ok_1) : get (v_ok_1)).x = 2;
> + if (v_ok_1.x != 2)
> + __builtin_abort ();
> +
> + Foo v_ok_2;
> + v_ok_2.x = 1;
> + get (v_ok_2).*(&Foo::x) = 2;
> + if (v_ok_2.x != 2)
> + __builtin_abort ();
> +
> + return 0;
> +}
> --
> 2.44.0
>
Marek
