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

[Jason Merrill <jason@redhat.com>]

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.

>   (Also, all 6 TREE_LANG_FLAGs for a CONSTRUCTOR are used.)

TREE_READONLY seems suitable.

Jason