| Message ID | 20190128120932.GF2135@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Add testcase for C++ placement new and the important difference between dtor clobbers and end of scope clobbers | expand |
On Mon, 28 Jan 2019, Jakub Jelinek wrote: > Hi! > > While thinking about PR59813, I came up with following testcase. > Is that valid C++? > > What I want to highlight here that we need to treat the clobbers created > for C++ destructors differently from clobbers added for when variables go > out of scope. > Right now we end up with: > char buf2[128]; > char buf1[128]; > > <bb 2> [local count: 1073741824]: > foo (&buf1); > MEM[(struct &)&buf1] ={v} {CLOBBER}; > memset (&MEM[(void *)&buf1], 32, 128); > bar (&buf1); > MEM[(struct &)&buf1] ={v} {CLOBBER}; > baz (&buf2); > buf2 ={v} {CLOBBER}; > buf1 ={v} {CLOBBER}; > return; > where the MEM[(struct &)&buf1] ={v} {CLOBBER}; is what is coming from > the inlined dtor (and the earlier one from inlined ctor), while > buf1 ={v} {CLOBBER}; is what is added by the gimplifier for vars that go out > of scope. The var conflict code only considers clobbers with VAR_DECLs on > the lhs and thus the testcase is not miscompiled. > > Now, if I manually change the second clobber in the function > MEM[(struct &)&buf1] ={v} {CLOBBER}; > memset (&MEM[(void *)&buf1], 32, 128); > bar (&buf1); > - MEM[(struct &)&buf1] ={v} {CLOBBER}; > + buf1 ={v} {CLOBBER}; > baz (&buf2); > buf2 ={v} {CLOBBER}; > buf1 ={v} {CLOBBER}; > the testcase is miscompiled, so that just points at the need to never fold > the clobbers with MEM_REF on the lhs into the form with VAR_DECL on the lhs. Eww :/ That means they are not semantically the same which IMHO is bad. If you make buf1 volatile does it trip over maybe_canonicalize_mem_ref_addr and miscompile? Or are we somehow doubly lucky and never share stack space of volatiles... Richard. > Tested on x86_64-linux and i686-linux, is the testcase ok for the trunk? > > 2019-01-28 Jakub Jelinek <jakub@redhat.com> > > * g++.dg/torture/alias-1.C: New test. > > --- gcc/testsuite/g++.dg/torture/alias-1.C.jj 2019-01-28 12:49:21.044341442 +0100 > +++ gcc/testsuite/g++.dg/torture/alias-1.C 2019-01-28 12:49:04.697612121 +0100 > @@ -0,0 +1,57 @@ > +// Verify we don't try to allocate the same stack slot for > +// buf1 and buf2 in qux. While there is a CLOBBER stmt for buf1 > +// from inlined destructor, the buf1 variable doesn't go out of scope > +// until after the baz call. > +// { dg-do run } > + > +#include <new> > +#include <cstring> > +#include <cstdlib> > + > +char *p; > +struct S { char buf[128]; S () { memset (buf, ' ', 128); }; ~S () {}; }; > + > +__attribute__((noipa)) void > +foo (char *x) > +{ > + p = x; > +} > + > +__attribute__((noipa)) int > +bar (S *x) > +{ > + return x->buf[12]; > +} > + > +__attribute__((noipa)) void > +baz (char *x) > +{ > + S *a = new (p) (S); > + S *b = new (x) (S); > + memset (a->buf, '0', 128); > + memset (b->buf, '1', 128); > + if (bar (a) != '0' || bar (b) != '1') > + abort (); > + b->~S (); > + a->~S (); > +} > + > +__attribute__((noipa)) void > +qux () > +{ > + char buf1[128]; > + foo (buf1); > + S *p = new (buf1) (S); > + bar (p); > + p->~S (); > + { > + char buf2[128]; > + baz (buf2); > + } > +} > + > +int > +main () > +{ > + qux (); > +} > > Jakub > >
On Mon, Jan 28, 2019 at 01:18:43PM +0100, Richard Biener wrote: > Eww :/ That means they are not semantically the same which IMHO is > bad. If you make buf1 volatile does it trip over > maybe_canonicalize_mem_ref_addr and miscompile? Or are we somehow If buf1 is volatile, then no clobbers are added for it for when it goes out of scope: if (VAR_P (t) && !is_global_var (t) && DECL_CONTEXT (t) == current_function_decl) { if (!DECL_HARD_REGISTER (t) && !TREE_THIS_VOLATILE (t) && !DECL_HAS_VALUE_EXPR_P (t) /* Only care for variables that have to be in memory. Others will be rewritten into SSA names, hence moved to the top-level. */ && !is_gimple_reg (t) && flag_stack_reuse != SR_NONE) { tree clobber = build_clobber (TREE_TYPE (t)); The clobbers from ctors and dtors are still there, but nothing folds them to non-MEM_REF anyway: volatile char buf2[128]; volatile char buf1[128]; <bb 2> [local count: 1073741824]: foo (&buf1); MEM[(struct &)&buf1] ={v} {CLOBBER}; memset (&MEM[(void *)&buf1], 32, 128); bar (&buf1); MEM[(struct &)&buf1] ={v} {CLOBBER}; baz (&buf2); return; in *.optimized. Jakub
On Mon, 28 Jan 2019, Jakub Jelinek wrote: > On Mon, Jan 28, 2019 at 01:18:43PM +0100, Richard Biener wrote: > > Eww :/ That means they are not semantically the same which IMHO is > > bad. If you make buf1 volatile does it trip over > > maybe_canonicalize_mem_ref_addr and miscompile? Or are we somehow > > If buf1 is volatile, then no clobbers are added for it for when it goes out > of scope: > if (VAR_P (t) > && !is_global_var (t) > && DECL_CONTEXT (t) == current_function_decl) > { > if (!DECL_HARD_REGISTER (t) > && !TREE_THIS_VOLATILE (t) > && !DECL_HAS_VALUE_EXPR_P (t) > /* Only care for variables that have to be in memory. Others > will be rewritten into SSA names, hence moved to the > top-level. */ > && !is_gimple_reg (t) > && flag_stack_reuse != SR_NONE) > { > tree clobber = build_clobber (TREE_TYPE (t)); > The clobbers from ctors and dtors are still there, but nothing folds them to > non-MEM_REF anyway: > volatile char buf2[128]; > volatile char buf1[128]; > > <bb 2> [local count: 1073741824]: > foo (&buf1); > MEM[(struct &)&buf1] ={v} {CLOBBER}; > memset (&MEM[(void *)&buf1], 32, 128); > bar (&buf1); > MEM[(struct &)&buf1] ={v} {CLOBBER}; > baz (&buf2); > return; > in *.optimized. Ah, OK. The clobber has record type, not array type. And we don't mark the LHS of clobbers TREE_THIS_VOLATILE but the RHS CONSTRUCTOR. So to have it folded you need to in-place dtor the declared type, not some other. The only reason I cannot get it folded it seems to be the if (/* Same volatile qualification. */ TREE_THIS_VOLATILE (*t) == TREE_THIS_VOLATILE (decl) /* Same TBAA behavior with -fstrict-aliasing. */ && !TYPE_REF_CAN_ALIAS_ALL (alias_type) && (TYPE_MAIN_VARIANT (TREE_TYPE (decl)) == TYPE_MAIN_VARIANT (TREE_TYPE (alias_type))) /* Same alignment. */ && TYPE_ALIGN (TREE_TYPE (decl)) == TYPE_ALIGN (TREE_TYPE (*t)) /* We have to look out here to not drop a required conversion from the rhs to the lhs if *t appears on the lhs or vice-versa if it appears on the rhs. Thus require strict type compatibility. */ && types_compatible_p (TREE_TYPE (*t), TREE_TYPE (decl))) check where TYPE_MAIN_VARIANT (TREE_TYPE (decl)) == TYPE_MAIN_VARIANT (TREE_TYPE (alias_type) fails. Oddly the two different main variants _do_ share their TYPE_FIELDs... So I guess in the end we're being lucky. Somehow. I've played with __attribute__((noipa)) void qux () { S buf1; foo ((char *)&buf1); S *p = new (&buf1) (S); bar (p); p->~S (); { char buf2[128]; baz (buf2); } } Richard.
On Mon, Jan 28, 2019 at 01:55:38PM +0100, Richard Biener wrote: > So I guess in the end we're being lucky. Somehow. I've played with > > __attribute__((noipa)) void > qux () > { > S buf1; > foo ((char *)&buf1); > S *p = new (&buf1) (S); > bar (p); > p->~S (); > { > char buf2[128]; > baz (buf2); > } > } I'd think the above is already invalid, by doing a placement new into a variable with non-trivial ctor and dtor while it is still constructed, then destruct the placement new created var in there and after a while destruct the original variable doesn't feel right to me, but I'm not a C++ language lawyer. I'd expect that usually either the whole var has char/std::byte etc. array type, or the placement new is into a field inside of some class (again char/std::byte etc. array type). Would could be valid is: __attribute__((noipa)) void qux () { S buf1; foo (buf1.buf); S *p = new (buf1.buf) (S); bar (p); p->~S (); { char buf2[128]; baz (buf2); } } but we don't really fold that into VAR_DECL lhs either, the type in the MEM_REF isn't really S even in this case, but CLASSTYPE_AS_BASE of the S type. Jakub
On 28/01/19 14:29 +0100, Jakub Jelinek wrote: >On Mon, Jan 28, 2019 at 01:55:38PM +0100, Richard Biener wrote: >> So I guess in the end we're being lucky. Somehow. I've played with >> >> __attribute__((noipa)) void >> qux () >> { >> S buf1; >> foo ((char *)&buf1); >> S *p = new (&buf1) (S); >> bar (p); >> p->~S (); >> { >> char buf2[128]; >> baz (buf2); >> } >> } > >I'd think the above is already invalid, by doing a placement new into >a variable with non-trivial ctor and dtor while it is still constructed, >then destruct the placement new created var in there and after a while >destruct the original variable doesn't feel right to me, but I'm not a C++ Right. When the second object is constructed in that location, the lifetime of the first one ends. When the destructor is automatically run at the end of the scope you're destroying something that is no longer alive, so undefined. >language lawyer. I'd expect that usually either the whole var has >char/std::byte etc. array type, or the placement new is into a field inside >of some class (again char/std::byte etc. array type). >Would could be valid is: Yeah I think the one below is OK. I'm still looking at the original testcase at the top of the thread. >__attribute__((noipa)) void >qux () >{ > S buf1; > foo (buf1.buf); > S *p = new (buf1.buf) (S); > bar (p); > p->~S (); > { > char buf2[128]; > baz (buf2); > } >} > >but we don't really fold that into VAR_DECL lhs either, the type in the >MEM_REF isn't really S even in this case, but CLASSTYPE_AS_BASE of the S >type. > > Jakub
On Mon, Jan 28, 2019 at 8:52 AM Jonathan Wakely <jwakely@redhat.com> wrote: > > On 28/01/19 14:29 +0100, Jakub Jelinek wrote: > >On Mon, Jan 28, 2019 at 01:55:38PM +0100, Richard Biener wrote: > >> So I guess in the end we're being lucky. Somehow. I've played with > >> > >> __attribute__((noipa)) void > >> qux () > >> { > >> S buf1; > >> foo ((char *)&buf1); > >> S *p = new (&buf1) (S); > >> bar (p); > >> p->~S (); > >> { > >> char buf2[128]; > >> baz (buf2); > >> } > >> } > > > >I'd think the above is already invalid, by doing a placement new into > >a variable with non-trivial ctor and dtor while it is still constructed, > >then destruct the placement new created var in there and after a while > >destruct the original variable doesn't feel right to me, but I'm not a C++ > > Right. When the second object is constructed in that location, the > lifetime of the first one ends. When the destructor is automatically > run at the end of the scope you're destroying something that is no > longer alive, so undefined. Indeed. > >language lawyer. I'd expect that usually either the whole var has > >char/std::byte etc. array type, or the placement new is into a field inside > >of some class (again char/std::byte etc. array type). > >Would could be valid is: > > Yeah I think the one below is OK. I'm still looking at the original > testcase at the top of the thread. The original testcase looks good to me. And I agree with Jakub's point, that destroying an object created in a buffer is necessarily different from destroying the buffer itself. Jason
Hi, On Mon, 28 Jan 2019, Richard Biener wrote: > > - MEM[(struct &)&buf1] ={v} {CLOBBER}; > > + buf1 ={v} {CLOBBER}; > > Eww :/ That means they are not semantically the same which IMHO is > bad. But they really are different. One is saying "these bytes contain nothing anymore", the other says "these bytes are gone". (in the original CLOBBER design there was only the latter, so perhaps both shouldn't have been conflated, but alas, afterwards it's always easy to say :) ). Ciao, Michael.
--- gcc/testsuite/g++.dg/torture/alias-1.C.jj 2019-01-28 12:49:21.044341442 +0100 +++ gcc/testsuite/g++.dg/torture/alias-1.C 2019-01-28 12:49:04.697612121 +0100 @@ -0,0 +1,57 @@ +// Verify we don't try to allocate the same stack slot for +// buf1 and buf2 in qux. While there is a CLOBBER stmt for buf1 +// from inlined destructor, the buf1 variable doesn't go out of scope +// until after the baz call. +// { dg-do run } + +#include <new> +#include <cstring> +#include <cstdlib> + +char *p; +struct S { char buf[128]; S () { memset (buf, ' ', 128); }; ~S () {}; }; + +__attribute__((noipa)) void +foo (char *x) +{ + p = x; +} + +__attribute__((noipa)) int +bar (S *x) +{ + return x->buf[12]; +} + +__attribute__((noipa)) void +baz (char *x) +{ + S *a = new (p) (S); + S *b = new (x) (S); + memset (a->buf, '0', 128); + memset (b->buf, '1', 128); + if (bar (a) != '0' || bar (b) != '1') + abort (); + b->~S (); + a->~S (); +} + +__attribute__((noipa)) void +qux () +{ + char buf1[128]; + foo (buf1); + S *p = new (buf1) (S); + bar (p); + p->~S (); + { + char buf2[128]; + baz (buf2); + } +} + +int +main () +{ + qux (); +}
Hi! While thinking about PR59813, I came up with following testcase. Is that valid C++? What I want to highlight here that we need to treat the clobbers created for C++ destructors differently from clobbers added for when variables go out of scope. Right now we end up with: char buf2[128]; char buf1[128]; <bb 2> [local count: 1073741824]: foo (&buf1); MEM[(struct &)&buf1] ={v} {CLOBBER}; memset (&MEM[(void *)&buf1], 32, 128); bar (&buf1); MEM[(struct &)&buf1] ={v} {CLOBBER}; baz (&buf2); buf2 ={v} {CLOBBER}; buf1 ={v} {CLOBBER}; return; where the MEM[(struct &)&buf1] ={v} {CLOBBER}; is what is coming from the inlined dtor (and the earlier one from inlined ctor), while buf1 ={v} {CLOBBER}; is what is added by the gimplifier for vars that go out of scope. The var conflict code only considers clobbers with VAR_DECLs on the lhs and thus the testcase is not miscompiled. Now, if I manually change the second clobber in the function MEM[(struct &)&buf1] ={v} {CLOBBER}; memset (&MEM[(void *)&buf1], 32, 128); bar (&buf1); - MEM[(struct &)&buf1] ={v} {CLOBBER}; + buf1 ={v} {CLOBBER}; baz (&buf2); buf2 ={v} {CLOBBER}; buf1 ={v} {CLOBBER}; the testcase is miscompiled, so that just points at the need to never fold the clobbers with MEM_REF on the lhs into the form with VAR_DECL on the lhs. Tested on x86_64-linux and i686-linux, is the testcase ok for the trunk? 2019-01-28 Jakub Jelinek <jakub@redhat.com> * g++.dg/torture/alias-1.C: New test. Jakub