On Wed, 18 Mar 2020, Jason Merrill wrote:
> On 3/18/20 11:58 AM, Patrick Palka wrote:
> > On Wed, 18 Mar 2020, Patrick Palka wrote:
> >
> > > On Tue, 17 Mar 2020, Jason Merrill wrote:
> > >
> > > > On 3/16/20 1:39 PM, Patrick Palka wrote:
> > > > > In this PR, we are performing constexpr evaluation of a CONSTRUCTOR of
> > > > > type
> > > > > union U which looks like
> > > > >
> > > > > {.a=foo (&<PLACEHOLDER_EXPR union U>)}.
> > > > >
> > > > > Since the function foo takes a reference to the CONSTRUCTOR we're
> > > > > building,
> > > > > it
> > > > > could potentially modify the CONSTRUCTOR from under us. In particular
> > > > > since
> > > > > U
> > > > > is a union, the evaluation of a's initializer could change the active
> > > > > member
> > > > > from a to another member -- something which cxx_eval_bare_aggregate
> > > > > doesn't
> > > > > expect to happen.
> > > > >
> > > > > Upon further investigation, it turns out this issue is not limited to
> > > > > constructors of UNION_TYPE and not limited to cxx_eval_bare_aggregate
> > > > > either.
> > > > > For example, within cxx_eval_store_expression we may be evaluating an
> > > > > assignment
> > > > > such as (this comes from the test pr94066-2.C):
> > > > >
> > > > > ((union U *) this)->a = TARGET_EXPR <D.2163, foo ((union U *)
> > > > > this)>;
> > > >
> > > > I assume this is actually an INIT_EXPR, or we would have preevaluated
> > > > and not
> > > > had this problem.
> > >
> > > Yes exactly, I should have specified that the above is an INIT_EXPR and
> > > not a MODIFY_EXPR.
> > >
> > > >
> > > > > where evaluation of foo could change the active member of *this, which
> > > > > was
> > > > > set
> > > > > earlier in cxx_eval_store_expression to 'a'. And if U is a
> > > > > RECORD_TYPE,
> > > > > then
> > > > > evaluation of foo could add new fields to *this, thereby making stale
> > > > > the
> > > > > 'valp'
> > > > > pointer to the target constructor_elt through which we're later
> > > > > assigning.
> > > > >
> > > > > So in short, it seems that both cxx_eval_bare_aggregate and
> > > > > cxx_eval_store_expression do not anticipate that a constructor_elt's
> > > > > initializer
> > > > > could modify the underlying CONSTRUCTOR as a side-effect.
> > > >
> > > > Oof. If this is well-formed, it's because initialization of a doesn't
> > > > actually start until the return statement of foo, so we're probably
> > > > wrong to
> > > > create a CONSTRUCTOR to hold the value of 'a' before evaluating foo.
> > > > Perhaps
> > > > init_subob_ctx shouldn't preemptively create a CONSTRUCTOR, and
> > > > similarly for
> > > > the cxx_eval_store_expression !preeval code.
> > >
> > > Hmm, I think I see what you mean. I'll look into this.
> >
> > In cpp0x/constexpr-array12.C we have
> >
> > struct A { int ar[3]; };
> > constexpr A a1 = { 0, a1.ar[0] };
> >
> > the initializer for a1 is a CONSTRUCTOR with the form
> >
> > {.ar={0, (int) VIEW_CONVERT_EXPR<const struct A>(a1).ar[0]}}
> >
> > If we don't preemptively create a CONSTRUCTOR in cxx_eval_bare_aggregate
> > to hold the value of 'ar' before evaluating its initializer, then we
> > won't be able to resolve the 'a1.ar[0]' later on, and we will reject
> > this otherwise valid test case with an "accessing an uninitialized array
> > element" diagnostic. So it seems we need to continue creating a
> > CONSTRUCTOR in cxx_eval_bare_aggregate before evaluating the initializer
> > of an aggregate sub-object to handle self-referential CONSTRUCTORs like
> > the one above.
> >
> > Then again, clang is going with rejecting the original testcase with the
> > following justification: https://bugs.llvm.org/show_bug.cgi?id=45133#c1
> > Should we follow suit?
>
> Yes, let's. There's no need to bend over backward to allow this kind of
> pathological testcase. Hubert is proposing that the initialization of u.a
> starts with the call to foo, and the testcase has undefined behavior because
> it ends the lifetime of u.a in the middle of its initialization.
Got it. I filed PR c++/94219 to track the testcase that uses a struct
in place of the union, on which we also ICE but this ICE is not a
regression from what I can tell. For GCC 10 I'll work on a minimal
patch to reject the original testcase from PR c++/94066.
