Re: [C++ PATCH] P0784R7 constexpr new fixes (PR c++/91369)
On 30/10/19 23:05 +0100, Jakub Jelinek wrote:
On Wed, Oct 30, 2019 at 04:26:17PM -0400, Jason Merrill wrote:
OK.
Thanks, committed.
> Now, for the accepts invalid issues.
> From what I understood, in constant evaluation
> ::operator new{,[]} or ::operator delete{,[]}
> can't be called from anywhere, only from new/delete expressions or
> std::allocator::{,de}allocate, is that correct?
> If so, perhaps we need some CALL_EXPR flag that we'd set on the call
> coming from new/delete expressions and disallow calls to
> replaceable global allocator functions in constexpr evaluation unless
> that flag is set or it is in std::allocator::{,de}allocate.
Looks like there used to be a TREE_CALLS_NEW flag in TREE_LANG_FLAG_1, but
that flag is now free for CALL_EXPR.
I'll try a CALL_EXPR flag first.
> Another thing is that even with that change,
>std::allocator a;
>auto p = a.allocate (1);
>*p = 1;
>a.deallocate (p, 1);
> would be accepted during constexpr evaluation, because allocate already
> has the cast which turns "heap uninit" variable into "heap " and assigns
> it a type, so there is nothing that will prevent the store from succeeding.
What's wrong with the store?
If it is fine, the better. I'd still think that
struct S { constexpr S () : s (0) {}; int s; };
std::allocator a;
auto p = a.allocate (1);
p->s = 1;
a.deallocate (p, 1);
would still be invalid though, because that type needs constructing and
the construction didn't happen in that case. Or:
Yes, I agree. EDG rejects that case and Daveed corrected one of my
tests which looked like the code above, so that it would be valid and
accepted by EDG.
struct S { constexpr S () : s (0) {}; int s; };
std::allocator a;
auto p = a.allocate (1);
std::construct_at (p);
p->s++; // ok
std::destruct_at (p);
p->s = 1; // shouldn't this be an error?
a.deallocate (p, 1);
but I admit I haven't tried to back that up by some standard wording, just a
general principle that objects with TYPE_ADDRESSABLE types need to be
constructed first and destructed last and accesses to it are only valid in
between those in normal code and constexpr should flag any UB as errors.
Jakub
Re: [C++ PATCH] P0784R7 constexpr new fixes (PR c++/91369)
On Wed, Oct 30, 2019 at 04:26:17PM -0400, Jason Merrill wrote:
> OK.
>
Thanks, committed.
> > Now, for the accepts invalid issues.
> > From what I understood, in constant evaluation
> > ::operator new{,[]} or ::operator delete{,[]}
> > can't be called from anywhere, only from new/delete expressions or
> > std::allocator::{,de}allocate, is that correct?
> > If so, perhaps we need some CALL_EXPR flag that we'd set on the call
> > coming from new/delete expressions and disallow calls to
> > replaceable global allocator functions in constexpr evaluation unless
> > that flag is set or it is in std::allocator::{,de}allocate.
>
> Looks like there used to be a TREE_CALLS_NEW flag in TREE_LANG_FLAG_1, but
> that flag is now free for CALL_EXPR.
I'll try a CALL_EXPR flag first.
> > Another thing is that even with that change,
> >std::allocator a;
> >auto p = a.allocate (1);
> >*p = 1;
> >a.deallocate (p, 1);
> > would be accepted during constexpr evaluation, because allocate already
> > has the cast which turns "heap uninit" variable into "heap " and assigns
> > it a type, so there is nothing that will prevent the store from succeeding.
>
> What's wrong with the store?
If it is fine, the better. I'd still think that
struct S { constexpr S () : s (0) {}; int s; };
std::allocator a;
auto p = a.allocate (1);
p->s = 1;
a.deallocate (p, 1);
would still be invalid though, because that type needs constructing and
the construction didn't happen in that case. Or:
struct S { constexpr S () : s (0) {}; int s; };
std::allocator a;
auto p = a.allocate (1);
std::construct_at (p);
p->s++; // ok
std::destruct_at (p);
p->s = 1; // shouldn't this be an error?
a.deallocate (p, 1);
but I admit I haven't tried to back that up by some standard wording, just a
general principle that objects with TYPE_ADDRESSABLE types need to be
constructed first and destructed last and accesses to it are only valid in
between those in normal code and constexpr should flag any UB as errors.
Jakub
Re: [C++ PATCH] P0784R7 constexpr new fixes (PR c++/91369)
On 10/24/19 5:11 AM, Jakub Jelinek wrote:
Hi!
Jonathan has showed me a testcase with std::allocator::{,de}allocate
and std::construct_at which FAILs with the current constexpr new
implementation.
There are two problems that make the testcase rejected, and further issues
(not addressed by this patch) where supposedly invalid C++20 code is
accepted.
The first problem was that cxx_replaceable_global_alloc_fn was actually
treating placement new as replaceable global allocation function, when it is
not.
The second problem is that std::construct_at uses placement new under the
hood. From what I understood, placement new is not allowed in C++20 in
constexpr context and it is unspecified whatever magic std::construct_at
uses to get similar effect.
The fix for the first problem is easy, just add the
DECL_IS_REPLACEABLE_OPERATOR_NEW_P || DECL_IS_OPERATOR_DELETE_P
checks to cxx_replaceable_global_alloc_fn, placement new nor some random
user added ::operator new (size_t, float, double) etc. should not have that
set.
For the second one, the patch is allowing placement new in constexpr
evaluation only in std::construct_at and not in other functions.
With this, Jonathan's testcase works fine. Ok for trunk if it passes
bootstrap/regtest?
OK.
Now, for the accepts invalid issues.
From what I understood, in constant evaluation
::operator new{,[]} or ::operator delete{,[]}
can't be called from anywhere, only from new/delete expressions or
std::allocator::{,de}allocate, is that correct?
If so, perhaps we need some CALL_EXPR flag that we'd set on the call
coming from new/delete expressions and disallow calls to
replaceable global allocator functions in constexpr evaluation unless
that flag is set or it is in std::allocator::{,de}allocate.
Looks like there used to be a TREE_CALLS_NEW flag in TREE_LANG_FLAG_1,
but that flag is now free for CALL_EXPR.
Or we could check whether the call is wrapped as expected by
maybe_wrap_new_for_constexpr, but that doesn't help delete.
Or we could build NEW_EXPR and DELETE_EXPR in non-dependent context and
lower them later.
Another thing is that even with that change,
std::allocator a;
auto p = a.allocate (1);
*p = 1;
a.deallocate (p, 1);
would be accepted during constexpr evaluation, because allocate already
has the cast which turns "heap uninit" variable into "heap " and assigns
it a type, so there is nothing that will prevent the store from succeeding.
What's wrong with the store?
Any thoughts on what to do with that? Even if the magic cast (perhaps
with some flag on it) is moved from whatever the first cast to non-void*
is to the placement new or start of corresponding constructor, would we
need to unmark it somehow if we say std::destroy_at it but allow
next std::construct_at to construct it again?
2019-10-24 Jakub Jelinek
PR c++/91369 - Implement P0784R7: constexpr new
* constexpr.c (cxx_replaceable_global_alloc_fn): Don't return true
for placement new.
(cxx_placement_new_fn, is_std_construct_at): New functions.
(cxx_eval_call_expression): Allow placement new in std::construct_at.
(potential_constant_expression_1): Likewise.
* g++.dg/cpp2a/constexpr-new5.C: New test.
--- gcc/cp/constexpr.c.jj 2019-10-23 20:37:59.981872274 +0200
+++ gcc/cp/constexpr.c 2019-10-24 10:20:57.127193138 +0200
@@ -1601,7 +1601,41 @@ cxx_replaceable_global_alloc_fn (tree fn
{
return (cxx_dialect >= cxx2a
&& IDENTIFIER_NEWDEL_OP_P (DECL_NAME (fndecl))
- && CP_DECL_CONTEXT (fndecl) == global_namespace);
+ && CP_DECL_CONTEXT (fndecl) == global_namespace
+ && (DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl)
+ || DECL_IS_OPERATOR_DELETE_P (fndecl)));
+}
+
+/* Return true if FNDECL is a placement new function that should be
+ useable during constant expression evaluation of std::construct_at. */
+
+static inline bool
+cxx_placement_new_fn (tree fndecl)
+{
+ if (cxx_dialect >= cxx2a
+ && IDENTIFIER_NEW_OP_P (DECL_NAME (fndecl))
+ && CP_DECL_CONTEXT (fndecl) == global_namespace
+ && !DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl)
+ && TREE_CODE (TREE_TYPE (fndecl)) == FUNCTION_TYPE)
+{
+ tree first_arg = TREE_CHAIN (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
+ if (TREE_VALUE (first_arg) == ptr_type_node
+ && TREE_CHAIN (first_arg) == void_list_node)
+ return true;
+}
+ return false;
+}
+
+/* Return true if FNDECL is std::construct_at. */
+
+static inline bool
+is_std_construct_at (tree fndecl)
+{
+ if (!decl_in_std_namespace_p (fndecl))
+return false;
+
+ tree name = DECL_NAME (fndecl);
+ return name && id_equal (name, "construct_at");
}
/* Subroutine of cxx_eval_constant_expression.
@@ -1738,6 +1772,27 @@ cxx_eval_call_expression (const constexp
return t;
}
}
+ /* Allow placement new in std::construct_at, just return the second
+argument. */
+ if
[C++ PATCH] P0784R7 constexpr new fixes (PR c++/91369)
Hi!
Jonathan has showed me a testcase with std::allocator::{,de}allocate
and std::construct_at which FAILs with the current constexpr new
implementation.
There are two problems that make the testcase rejected, and further issues
(not addressed by this patch) where supposedly invalid C++20 code is
accepted.
The first problem was that cxx_replaceable_global_alloc_fn was actually
treating placement new as replaceable global allocation function, when it is
not.
The second problem is that std::construct_at uses placement new under the
hood. From what I understood, placement new is not allowed in C++20 in
constexpr context and it is unspecified whatever magic std::construct_at
uses to get similar effect.
The fix for the first problem is easy, just add the
DECL_IS_REPLACEABLE_OPERATOR_NEW_P || DECL_IS_OPERATOR_DELETE_P
checks to cxx_replaceable_global_alloc_fn, placement new nor some random
user added ::operator new (size_t, float, double) etc. should not have that
set.
For the second one, the patch is allowing placement new in constexpr
evaluation only in std::construct_at and not in other functions.
With this, Jonathan's testcase works fine. Ok for trunk if it passes
bootstrap/regtest?
Now, for the accepts invalid issues.
From what I understood, in constant evaluation
::operator new{,[]} or ::operator delete{,[]}
can't be called from anywhere, only from new/delete expressions or
std::allocator::{,de}allocate, is that correct?
If so, perhaps we need some CALL_EXPR flag that we'd set on the call
coming from new/delete expressions and disallow calls to
replaceable global allocator functions in constexpr evaluation unless
that flag is set or it is in std::allocator::{,de}allocate.
Another thing is that even with that change,
std::allocator a;
auto p = a.allocate (1);
*p = 1;
a.deallocate (p, 1);
would be accepted during constexpr evaluation, because allocate already
has the cast which turns "heap uninit" variable into "heap " and assigns
it a type, so there is nothing that will prevent the store from succeeding.
Any thoughts on what to do with that? Even if the magic cast (perhaps
with some flag on it) is moved from whatever the first cast to non-void*
is to the placement new or start of corresponding constructor, would we
need to unmark it somehow if we say std::destroy_at it but allow
next std::construct_at to construct it again?
2019-10-24 Jakub Jelinek
PR c++/91369 - Implement P0784R7: constexpr new
* constexpr.c (cxx_replaceable_global_alloc_fn): Don't return true
for placement new.
(cxx_placement_new_fn, is_std_construct_at): New functions.
(cxx_eval_call_expression): Allow placement new in std::construct_at.
(potential_constant_expression_1): Likewise.
* g++.dg/cpp2a/constexpr-new5.C: New test.
--- gcc/cp/constexpr.c.jj 2019-10-23 20:37:59.981872274 +0200
+++ gcc/cp/constexpr.c 2019-10-24 10:20:57.127193138 +0200
@@ -1601,7 +1601,41 @@ cxx_replaceable_global_alloc_fn (tree fn
{
return (cxx_dialect >= cxx2a
&& IDENTIFIER_NEWDEL_OP_P (DECL_NAME (fndecl))
- && CP_DECL_CONTEXT (fndecl) == global_namespace);
+ && CP_DECL_CONTEXT (fndecl) == global_namespace
+ && (DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl)
+ || DECL_IS_OPERATOR_DELETE_P (fndecl)));
+}
+
+/* Return true if FNDECL is a placement new function that should be
+ useable during constant expression evaluation of std::construct_at. */
+
+static inline bool
+cxx_placement_new_fn (tree fndecl)
+{
+ if (cxx_dialect >= cxx2a
+ && IDENTIFIER_NEW_OP_P (DECL_NAME (fndecl))
+ && CP_DECL_CONTEXT (fndecl) == global_namespace
+ && !DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl)
+ && TREE_CODE (TREE_TYPE (fndecl)) == FUNCTION_TYPE)
+{
+ tree first_arg = TREE_CHAIN (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
+ if (TREE_VALUE (first_arg) == ptr_type_node
+ && TREE_CHAIN (first_arg) == void_list_node)
+ return true;
+}
+ return false;
+}
+
+/* Return true if FNDECL is std::construct_at. */
+
+static inline bool
+is_std_construct_at (tree fndecl)
+{
+ if (!decl_in_std_namespace_p (fndecl))
+return false;
+
+ tree name = DECL_NAME (fndecl);
+ return name && id_equal (name, "construct_at");
}
/* Subroutine of cxx_eval_constant_expression.
@@ -1738,6 +1772,27 @@ cxx_eval_call_expression (const constexp
return t;
}
}
+ /* Allow placement new in std::construct_at, just return the second
+argument. */
+ if (cxx_placement_new_fn (fun)
+ && ctx->call
+ && ctx->call->fundef
+ && is_std_construct_at (ctx->call->fundef->decl))
+ {
+ const int nargs = call_expr_nargs (t);
+ tree arg1 = NULL_TREE;
+ for (int i = 0; i < nargs; ++i)
+ {
+ tree arg = CALL_EXPR_ARG (t, i);
+ arg = cxx_eval_constant_expression (ctx, arg, false,
+
