This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]