[PATCH] c++: Handle CONSTRUCTORs without indexes in find_array_ctor_elt [PR93549]

[Jakub Jelinek]

On Wed, Feb 05, 2020 at 01:31:30PM -0500, Jason Merrill wrote:
> > from the constexpr new change apparently broke the following testcase.
> > When handling COND_EXPR, we build_vector_from_val, however as the argument we
> > pass to it is not an INTEGER_CST/REAL_CST, but that wrapped in a
> > NON_LVALUE_EXPR location wrapper, we end up with a CONSTRUCTOR and as it is
> > middle-end that builds it, it doesn't bother with indexes.  The
> > cp_fully_fold_init call used to fold it into VECTOR_CST in the past, but as
> > we intentionally don't invoke it anymore as it might fold away something
> > that needs to be diagnosed during constexpr evaluation, we end up evaluating
> > ARRAY_REF into the index-less CONSTRUCTOR.  The following patch fixes the
> > ICE by teaching find_array_ctor_elt to handle CONSTRUCTORs without indexes
> > (that itself could be still very efficient) and CONSTRUCTORs with some
> > indexes present and others missing (the rules are that if the index on the
> > first element is missing, then it is the array's lowest index (in C/C++ 0)
> > and if other indexes are missing, they are the index of the previous element
> > + 1).
> 
> Is it currently possible to get a CONSTRUCTOR with non-init-list type that
> has some indexes present and others missing?  Other than from the new code
> in your patch that sets some indexes?

I don't know, can try to add some instrumentation and do bootstrap/regtest
with it.  The handling of the CONSTRUCTORs with missing or present or mixed
indexes is what I found in various middle-end routines.
The only thing I see in our verifiers is that in GIMPLE function bodies,
we don't allow non-VECTOR_TYPE CONSTRUCTORs with any elements, and for
VECTOR_TYPE CONSTRUCTORs we require that indexes are NULL for elements with
VECTOR_TYPE and for others require that it is either NULL or INTEGER_CST
matching the position (so effectively for those direct access is still
possible).
The question might not be just what we do emit right now, but also what we'd
like to emit in the future, because as has been noted several times, for
large initializers those explicit indexes consume huge amounts of memory.
In C with designated initializers, I can see us not emitting indexes from
the start because we'd want to avoid the memory overhead for normal
sequential initializers, but then much later we can find a designated
initializer that wants to skip over some elements and thus add an index at
that point (or range designator for which we want RANGE_EXPR); shall we add
indexes to all elements at that point?
In C++, I think we don't allow non-useless array designated initializers, so
there is no way to skip elements using that or go backwards, but still,
don't we emit RANGE_EXPRs if we see the same initializer for many elements?
I guess right now we emit indexes for all elements for those, but if we
choose to optimize?

> Is it unreasonable to assume that if the first element has no index, none of
> the elements do?

Not sure, see above.  Depends on what we want to guarantee.

> > +	    else if (i == j + (middle - begin))
> > +	      {
> > +		(*elts)[middle].index = dindex;
> 
> Why set this index?

Because the caller asserts or relies that it has one.
      constructor_elt *cep = NULL;
      if (code == ARRAY_TYPE)
        {
          HOST_WIDE_INT i
            = find_array_ctor_elt (*valp, index, /*insert*/true);
          gcc_assert (i >= 0);
          cep = CONSTRUCTOR_ELT (*valp, i);
          gcc_assert (TREE_CODE (cep->index) != RANGE_EXPR);

Now, ATM we are aware of just small CONSTRUCTORs that can appear this way
(VECTOR_TYPE and so generally not too many elements in real-world
testcases), so if you prefer, the function when seeing NULL index could just
add indexes to all elements and retry and defer deciding if and how we
optimize large constructors for later.

	Jakub