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