On 8/18/19 6:52 AM, Marek Polacek wrote:
On Fri, Aug 16, 2019 at 05:40:39PM -0700, Jason Merrill wrote:
On 8/16/19 5:11 AM, Marek Polacek wrote:
On Thu, Aug 15, 2019 at 08:21:25PM -0400, Jason Merrill wrote:
On 8/15/19 5:34 PM, Marek Polacek wrote:
On Wed, Aug 14, 2019 at 02:50:13PM -0400, Jason Merrill wrote:
On Thu, Aug 8, 2019 at 3:25 PM Marek Polacek <pola...@redhat.com> wrote:
On Thu, Aug 08, 2019 at 11:06:17AM -0400, Jason Merrill wrote:
On 8/6/19 3:20 PM, Marek Polacek wrote:
On Mon, Aug 05, 2019 at 03:54:19PM -0400, Jason Merrill wrote:
On 7/31/19 3:26 PM, Marek Polacek wrote:
One of the features of constexpr is that it doesn't allow UB; and such UB must
be detected at compile-time. So running your code in a context that requires
a constant expression should ensure that the code in question is free of UB.
In effect, constexpr can serve as a sanitizer. E.g. this article describes in
in more detail:
<https://shafik.github.io/c++/undefined%20behavior/2019/05/11/explporing_undefined_behavior_using_constexpr.html>
[dcl.type.cv]p4 says "Any attempt to modify a const object during its lifetime
results in undefined behavior." However, as the article above points out, we
aren't detecting that case in constexpr evaluation.
This patch fixes that. It's not that easy, though, because we have to keep in
mind [class.ctor]p5:
"A constructor can be invoked for a const, volatile or const volatile object.
const and volatile semantics are not applied on an object under construction.
They come into effect when the constructor for the most derived object ends."
I handled this by keeping a hash set which tracks objects under construction.
I considered other options, such as going up call_stack, but that wouldn't
work with trivial constructor/op=. It was also interesting to find out that
the definition of TREE_HAS_CONSTRUCTOR says "When appearing in a FIELD_DECL,
it means that this field has been duly initialized in its constructor" though
nowhere in the codebase do we set TREE_HAS_CONSTRUCTOR on a FIELD_DECL as far
as I can see. Unfortunately, using this bit proved useless for my needs here.
Also, be mindful of mutable subobjects.
Does this approach look like an appropriate strategy for tracking objects'
construction?
For scalar objects, we should be able to rely on INIT_EXPR vs. MODIFY_EXPR
to distinguish between initialization and modification; for class objects, I
This is already true: only class object go into the hash set.
wonder about setting a flag on the CONSTRUCTOR after initialization is
complete to indicate that the value is now constant.
But here we're not dealing with CONSTRUCTORs in the gcc sense (i.e. exprs with
TREE_CODE == CONSTRUCTOR). We have a CALL_EXPR like Y::Y ((struct Y *) &y),
which initializes the object "y". Setting a flag on the CALL_EXPR or its
underlying
function decl wouldn't help.
Am I missing something?
I was thinking that where in your current patch you call
remove_object_under_construction, we could instead mark the object's value
CONSTRUCTOR as immutable.
Ah, what you meant was to look at DECL_INITIAL of the object we're
constructing, which could be a CONSTRUCTOR. Unfortunately, this
DECL_INITIAL is null (in all the new tests when doing
remove_object_under_construction), so there's nothing to mark as TREE_READONLY
:/.
There's a value in ctx->values, isn't there?
Doesn't seem to be the case for e.g.
struct A {
int n;
constexpr A() : n(1) { n = 2; }
};
struct B {
const A a;
constexpr B(bool b) {
if (b)
const_cast<A &>(a).n = 3; // { dg-error "modifying a const object" }
}
};
constexpr B b(false);
static_assert(b.a.n == 2, "");
Here we're constructing "b", its ctx->values->get(new_obj) is initially
"{}". In the middle of constructing "b", we construct "b.a", but that
has nothing in ctx->values.
Right, subobjects aren't in ctx->values. In cxx_eval_call_expression we
have
if (DECL_CONSTRUCTOR_P (fun))
/* This can be null for a subobject constructor call, in
which case what we care about is the initialization
side-effects rather than the value. We could get at the
value by evaluating *this, but we don't bother; there's
no need to put such a call in the hash table. */
result = lval ? ctx->object : ctx->ctor;
Your patch already finds *this (b.a) and puts it in new_obj; if it's const
we can evaluate it to get the CONSTRUCTOR to set TREE_READONLY on.
Ah, got it! This patch uses setting TREE_READONLY to achieve what I was after.
I also needed to set TREE_READONLY in cxx_eval_constant_expression/DECL_EXPR.
The additional evaluating will only happen for const-qual objects so I hope not
very often.
Any further comments? Thanks,
@@ -1910,6 +1958,29 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree
t,
+ /* At this point, the object's constructor will have run, so
+ the object is no longer under construction, and its possible
+ 'const' semantics now apply. Make a note of this fact by
+ marking the CONSTRUCTOR TREE_READONLY. */
+ if (new_obj
+ && CLASS_TYPE_P (TREE_TYPE (new_obj))
+ && CP_TYPE_CONST_P (TREE_TYPE (new_obj)))
+ {
+ tree *ctor = ctx->values->get (new_obj);
I don't think trying ctx->values->get first is a win, let's go straight to
cxx_eval_constant_expression.
Done.
+/* Return true if we are modifying something that is const during constant
+ expression evaluation. CODE is the code of the statement, OBJ is the
+ object in question, MUTABLE_P is true if one of the subobjects were
+ declared mutable. */
+
+static bool
+modifying_const_object_p (const constexpr_ctx *ctx, tree_code code, tree obj,
+ bool mutable_p)
+{
+ /* If this is initialization, there's no problem. */
+ if (code != MODIFY_EXPR)
+ return false;
+
+ tree type = TREE_TYPE (obj);
+
+ /* [basic.type.qualifier] "A const object is an object of type
+ const T or a non-mutable subobject of a const object." */
+ return ((TREE_READONLY (obj) || CP_TYPE_CONST_P (type)
+ /* If it's an aggregate and any field is const, then it is
+ effectively const. */
+ || (CLASS_TYPE_P (type) && C_TYPE_FIELDS_READONLY (type)))
This seems wrong; if one field is const, we can still modify other fields. I
don't see a test for that case.
I took it from cp_build_modify_expr but it's not really needed, so dropped.
@@ -3783,6 +3885,17 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
tree t,
{
tree ob = TREE_OPERAND (probe, 0);
tree elt = TREE_OPERAND (probe, 1);
+ if (DECL_P (elt) && DECL_MUTABLE_P (elt))
+ mutable_p = true;
+ if (evaluated
+ && modifying_const_object_p (ctx, TREE_CODE (t), probe,
+ mutable_p))
+ {
+ if (!ctx->quiet)
+ modifying_const_object_error (t, probe);
+ *non_constant_p = true;
+ return t;
+ }
What if there's a mutable member further down, i.e.
struct A { mutable int i; };
struct B { A a; };
const B b;
b.a.i = 42;
? And also...
This works. We check mutable going from the innermost object to the outermost,
so if we have b.a.n, we check n first. Some of the new tests are exercising
this.
@@ -3811,6 +3924,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
tree t,
+ if (modifying_const_object_p (ctx, TREE_CODE (t), object, mutable_p))
+ {
+ if (!ctx->quiet)
+ modifying_const_object_error (t, object);
+ *non_constant_p = true;
+ return t;
+ }
...we are already collecting the CONSTRUCTORs that we're dealing with in the
"ctors" stack, we shouldn't need to evaluate object at this point. I'd
expect the topmost class-type CONSTRUCTOR on the stack (if any) to be the
one we want to look at. I'd think you could do away with much of
modifying_const_object_p.
Oh, got it; wish I'd thought of that sooner. We can't just check the topmost
CONSTRUCTOR though, the yuge comment I've added has an example when that would
not be enough.
And yes, I could 86 most of modifying_const_object_p.
@@ -4650,6 +4772,11 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx,
tree t,
non_constant_p, overflow_p);
/* Don't share a CONSTRUCTOR that might be changed. */
init = unshare_constructor (init);
+ /* Remember that a constant object's constructor has already
+ ran. */
"has...run"
Yikes.
Thanks for the review, it allowed me to make the patch more concise.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
Looking pretty good, just a couple of nits:
+ We track the "under construction" state by using the TREE_READONLY
+ bit in the object's CONSTRUCTOR.
Only incidentally; what we track using the TREE_READONLY bit is whether
the object is read-only.
+ /* [class.ctor]p5 "A constructor can be invoked for a const,
+ volatile, or const volatile object. const and volatile
+ semantics are not applied on an object under construction.
+ They come into effect when the constructor for the most
+ derived object ends." */
+ tree elt;
+ unsigned int i;
+ FOR_EACH_VEC_ELT (*ctors, i, elt)
+ if (CLASS_TYPE_P (TREE_TYPE (elt)) && TREE_READONLY (elt))
Would it make sense to look here for the CONSTRUCTOR with the same type
as const_object_being_modified and set "fail" (or not) based only on
that CONSTRUCTOR?
Jason
