From: Jason Merrill <jason@redhat.com>
To: Marek Polacek <polacek@redhat.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: C++ PATCH for c++/91264 - detect modifying const objects in constexpr
Date: Sat, 17 Aug 2019 00:51:00 -0000 [thread overview]
Message-ID: <b187bd4a-f7f8-29e9-244a-e5ad33093bb1@redhat.com> (raw)
In-Reply-To: <20190816121106.GT14737@redhat.com>
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 <polacek@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.
> +/* 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.
> @@ -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...
> @@ -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.
> @@ -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"
Jason
next prev parent reply other threads:[~2019-08-17 0:40 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-31 19:39 Marek Polacek
2019-08-05 20:37 ` Jason Merrill
2019-08-06 19:35 ` Marek Polacek
2019-08-08 15:18 ` Jason Merrill
2019-08-08 19:48 ` Marek Polacek
2019-08-14 19:51 ` Jason Merrill
2019-08-15 22:02 ` Marek Polacek
2019-08-16 0:28 ` Jason Merrill
2019-08-16 12:33 ` Marek Polacek
2019-08-17 0:51 ` Jason Merrill [this message]
2019-08-18 16:52 ` Marek Polacek
2019-08-19 1:19 ` Jason Merrill
2019-08-19 1:21 ` Marek Polacek
2019-08-19 2:31 ` Marek Polacek
2019-08-19 8:39 ` Jason Merrill
2019-08-06 20:01 ` Paolo Carlini
2019-08-06 20:04 ` Marek Polacek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b187bd4a-f7f8-29e9-244a-e5ad33093bb1@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=polacek@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).