[C++ PATCH] PR c++/91369 - Implement P0784R7: constexpr new

[Jakub Jelinek]

On Tue, Oct 01, 2019 at 05:56:00PM -0400, Jason Merrill wrote:
> > +		  else if (DECL_NAME (var) == heap_deleted_identifier)
> > +		    {
> > +		      if (!ctx->quiet)
> > +			error_at (loc, "deallocation of already deallocated "
> > +				       "storage");
> > +		      *non_constant_p = true;
> > +		      return t;
> > +		    }
> 
> Don't we need an error for trying to deallocate something that wasn't
> allocated within the constexpr evaluation?

We don't need it, but it might be a good idea.  Right now it will just fall
through into the non-constexpr call in a constexpr context diagnostics,
while a message that it isn't constexpr because it is trying to deallocate
something that hasn't been allocated might be clearer.  Will add.

> > @@ -3011,8 +3067,10 @@ initialized_type (tree t)
> >       {
> >         /* A constructor call has void type, so we need to look deeper.  */
> >         tree fn = get_function_named_in_call (t);
> > -      if (fn && TREE_CODE (fn) == FUNCTION_DECL
> > -	  && DECL_CXX_CONSTRUCTOR_P (fn))
> > +      if (fn
> > +	  && TREE_CODE (fn) == FUNCTION_DECL
> > +	  && (DECL_CXX_CONSTRUCTOR_P (fn)
> > +	      || (cxx_dialect >= cxx2a && DECL_CXX_DESTRUCTOR_P (fn))))
> >   	type = DECL_CONTEXT (fn);
> 
> Why is this needed?  A destructor doesn't initialize anything, so returning
> void seems appropriate.

This was needed initially, because the evaluate outermost expr if it sees
void type just returns early, does nothing.  But later I found out that while
the above worked for the normal dtor case, for the array dtor case it didn't
work anyway and I've added an extra argument.  So this is not needed anymore
and will delete.

> I think we want to factor this function more, so we don't have the same code
> in multiple places for handling an array, and an array member, and a pointer
> to array.  Do you want to take a look at bug 71504 while you're touching
> this code?

Patch posted separately.

> > +  if (!valp
> > +      && VAR_P (object)
> > +      && DECL_NAME (object) == heap_identifier)
> > +    {
> > +      tree ctor = build_constructor (type, NULL);
> > +      CONSTRUCTOR_NO_CLEARING (ctor) = true;
> > +      ctx->values->put (object, ctor);
> > +      valp = ctx->values->get (object);
> > +    }
> 
> Instead of this, how about giving the object NULL_TREE value when we create
> it in cxx_eval_call_expression?

Will try.
> > +	      if (tree fld1 = TYPE_FIELDS (var_type))
> > +		if (TREE_CODE (fld1) == FIELD_DECL
> > +		    && DECL_NAME (fld1) == NULL_TREE
> > +		    && DECL_ARTIFICIAL (fld1)
> > +		    && TREE_CODE (TREE_TYPE (fld1)) == ARRAY_TYPE
> > +		    && COMPLETE_TYPE_P (TREE_TYPE (fld1)))
> > +		  if (tree fld2 = DECL_CHAIN (fld1))
> > +		    if (TREE_CODE (fld2) == FIELD_DECL
> > +			&& DECL_NAME (fld2) == NULL_TREE
> > +			&& DECL_ARTIFICIAL (fld2)
> > +			&& TREE_CODE (TREE_TYPE (fld2)) == ARRAY_TYPE
> > +			&& TYPE_DOMAIN (TREE_TYPE (fld2)) == NULL_TREE
> > +			&& DECL_CHAIN (fld2) == NULL_TREE)
> 
> Maybe give the struct a magic name so you don't need to do as much checking
> of the FIELD_DECLs?

Ok.

> So here you're completing the type of the array member of the struct.
> 
> > +		TREE_TYPE (var) = var_type;
> > +		TREE_TYPE (TREE_OPERAND (op, 0))
> > +		  = build_pointer_type (var_type);
> > +	      }
> > +	  }
> 
> Let's factor out all of this code, too.

Yes, it is used twice, so factoring it out makes a lot of sense.

> > @@ -5525,16 +5762,23 @@ cxx_eval_outermost_constant_expr (tree t
> >     bool non_constant_p = false;
> >     bool overflow_p = false;
> >     hash_map<tree,tree> map;
> > +  auto_vec<tree, 16> heap_vars;
> >     HOST_WIDE_INT constexpr_ctx_count = 0;
> >     constexpr_ctx ctx = { NULL, &map, NULL, NULL, NULL, NULL,
> > -			&constexpr_ctx_count, allow_non_constant, strict,
> > -			manifestly_const_eval || !allow_non_constant };
> > +			&constexpr_ctx_count, &heap_vars, allow_non_constant,
> > +			strict, manifestly_const_eval || !allow_non_constant };
> 
> As we add more stuff to constexpr_ctx, creating new ones on the stack
> becomes more and more expensive.  We should really split off the parts that
> change frequently: Maybe just ctor/object, maybe also call/save_exprs/...?

I was thinking about creating a constexpr_outermost_ctx that would include
things that are global and don't change, at least the count and this vector.
Not sure about the bool fields, we access them way too often that it might
be too ugly to change all the ctx->quiet to ctx->outermost->quite or
similar.

> > @@ -5593,6 +5852,32 @@ cxx_eval_outermost_constant_expr (tree t
> >         non_constant_p = true;
> >       }
> > +  if (!heap_vars.is_empty ())
> > +    {
> > +      tree heap_var = cp_walk_tree_without_duplicates (&r, find_heap_var_refs,
> > +						       NULL);
> 
> Doesn't verify_constant already complain about remaining references to
> allocated objects?

I believe it doesn't, because the heap VAR_DECLs are TREE_STATIC (things
really don't work at all if they aren't).

> > @@ -6239,7 +6535,12 @@ potential_constant_expression_1 (tree t,
> >   		if (!DECL_DECLARED_CONSTEXPR_P (fun)
> >   		    /* Allow any built-in function; if the expansion
> >   		       isn't constant, we'll deal with that then.  */
> > -		    && !fndecl_built_in_p (fun))
> > +		    && !fndecl_built_in_p (fun)
> > +		    /* In C++2a, replaceable global allocation functions
> > +		       are constant expressions.  */
> > +		    && (cxx_dialect < cxx2a
> > +			|| !IDENTIFIER_NEWDEL_OP_P (DECL_NAME (fun))
> > +			|| CP_DECL_CONTEXT (fun) != global_namespace))
> 
> This is the second occurrence of this three-line test for a constexpr
> (de)allocation function, let's factor it out.

Ok, will factor that out.

> i.e.
> 
> struct {
>   size_t cookie[N];
>   elt array[];
> };
> 
> I guess you want an array for the cookie rather than a non-array size_t data
> member to handle ARM cookies that also include the element size. Please
> include this in the comment.

That is actually what I'm using.  When build_new_1 creates it, it is
struct { size_t cookie[N]; T array[]; }; and when constexpr cast
rewrites it it becomes struct { size_t cookie[N]; T array[M]; };
with the M bound computed from the allocation size.

> > @@ -1283,24 +1296,15 @@ process_subob_fn (tree fn, tree *spec_p,
> >         if (diag)
> >   	{
> >   	  inform (DECL_SOURCE_LOCATION (fn),
> > -		  "defaulted constructor calls non-%<constexpr%> %qD", fn);
> > +		  SFK_DTOR_P (sfk)
> > +		  ? G_("destructor calls non-%<constexpr%> %qD")
> 
> Not "defaulted"?

Will fix.

	Jakub