Re: C++ PATCH for c++/91264 - detect modifying const objects in constexpr

[Jason Merrill <jason@redhat.com>]

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.

Jason