| Message ID | 01020195ce4ffc07-913ce014-8f74-47e8-ab3f-864232e6cb03-000000@eu-west-1.amazonses.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Properly fold <COND_EXPR>.*<COMPONENT> [PR114525] | expand |
On 3/25/25 1:18 PM, 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)? OK, and yes. > 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. > > --- > 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; > +}
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
On 3/25/25 1:50 PM, Marek Polacek wrote: > 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. Sure, that does sound better. Jason
Hi, On Tue Mar 25, 2025 at 6:52 PM CET, Jason Merrill wrote: > On 3/25/25 1:50 PM, Marek Polacek wrote: >> 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. Good call out, thanks Marek. I've merged the patch with the suggested test rename as r15-8911-g35ce9afc84a63f. I'll reply to this thread in 2-3 weeks when I've backported to 13 and 14. Simon
Hi, On Tue Mar 25, 2025 at 8:33 PM CET, Simon Martin wrote: > Hi, > > On Tue Mar 25, 2025 at 6:52 PM CET, Jason Merrill wrote: >> On 3/25/25 1:50 PM, Marek Polacek wrote: >>> 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. > Good call out, thanks Marek. > > I've merged the patch with the suggested test rename as > r15-8911-g35ce9afc84a63f. > > I'll reply to this thread in 2-3 weeks when I've backported to 13 and > 14. For information I have just backported this to 14 (r14-11602) and 13 (r13-9523). Simon
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; +}
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. --- 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