Re: [PATCH v2] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]

[Marek Polacek <polacek@redhat.com>]

On Wed, Mar 11, 2020 at 04:17:02PM -0400, Jason Merrill via Gcc-patches wrote:
> On 3/11/20 1:59 PM, Marek Polacek wrote:
> > On Tue, Mar 10, 2020 at 03:46:03PM -0400, Jason Merrill wrote:
> > > On 3/9/20 4:34 PM, Marek Polacek wrote:
> > > > On Mon, Mar 09, 2020 at 04:25:00PM -0400, Marek Polacek wrote:
> > > > > On Mon, Mar 09, 2020 at 03:37:56PM -0400, Jason Merrill wrote:
> > > > > > On 3/9/20 9:40 AM, Marek Polacek wrote:
> > > > > > > On Mon, Mar 09, 2020 at 09:19:30AM -0400, Jason Merrill wrote:
> > > > > > > > On 3/9/20 8:58 AM, Jakub Jelinek wrote:
> > > > > > > > > On Fri, Mar 06, 2020 at 07:43:43PM -0500, Jason Merrill wrote:
> > > > > > > > > > On 3/6/20 6:54 PM, Marek Polacek wrote:
> > > > > > > > > > > I got a report that building Chromium fails with the "modifying a const
> > > > > > > > > > > object" error.  After some poking I realized it's a bug in GCC, not in
> > > > > > > > > > > their codebase.
> > > > > > > > > > > 
> > > > > > > > > > > Much like with ARRAY_REFs, which can be const even though the array
> > > > > > > > > > > itself isn't, COMPONENT_REFs can be const although neither the object
> > > > > > > > > > > nor the field were declared const.  So let's dial down the checking.
> > > > > > > > > > > Here the COMPONENT_REF was const because of the "const_cast<const U &>(m)"
> > > > > > > > > > > thing -- cxx_eval_component_reference then builds a COMPONENT_REF with
> > > > > > > > > > > TREE_TYPE (t).
> > > > > > > > > > 
> > > > > > > > > > What is folding the const into the COMPONENT_REF?
> > > > > > > > > 
> > > > > > > > > cxx_eval_component_reference when it is called on
> > > > > > > > > ((const struct array *) this)->elems
> > > > > > > > > with /*lval=*/true and lval is true because we are evaluating
> > > > > > > > > <retval> = (const int &) &((const struct array *) this)->elems[VIEW_CONVERT_EXPR<size_t>(n)];
> > > > > > > > 
> > > > > > > > Ah, sure.  We're pretty loose with cv-quals in the constexpr code in
> > > > > > > > general, so it's probably not worth trying to change that here.  Getting
> > > > > > > > back to the patch:
> > > > > > > 
> > > > > > > Yes, here the additional const was caused by a const_cast adding a const.
> > > > > > > 
> > > > > > > But this could also happen with wrapper functions like this one from
> > > > > > > __array_traits in std::array:
> > > > > > > 
> > > > > > >          static constexpr _Tp&
> > > > > > >          _S_ref(const _Type& __t, std::size_t __n) noexcept
> > > > > > >          { return const_cast<_Tp&>(__t[__n]); }
> > > > > > > 
> > > > > > > where the ref-to-const parameter added the const.
> > > > > > > 
> > > > > > > > > +      if (TREE_CODE (obj) == COMPONENT_REF)
> > > > > > > > > +	{
> > > > > > > > > +	  tree op1 = TREE_OPERAND (obj, 1);
> > > > > > > > > +	  if (CP_TYPE_CONST_P (TREE_TYPE (op1)))
> > > > > > > > > +	    return true;
> > > > > > > > > +	  else
> > > > > > > > > +	    {
> > > > > > > > > +	      tree op0 = TREE_OPERAND (obj, 0);
> > > > > > > > > +	      /* The LHS of . or -> might itself be a COMPONENT_REF.  */
> > > > > > > > > +	      if (TREE_CODE (op0) == COMPONENT_REF)
> > > > > > > > > +		op0 = TREE_OPERAND (op0, 1);
> > > > > > > > > +	      return CP_TYPE_CONST_P (TREE_TYPE (op0));
> > > > > > > > > +	    }
> > > > > > > > > +	}
> > > > > > > > 
> > > > > > > > Shouldn't this be a loop?
> > > > > > > 
> > > > > > > I don't think so, though my earlier patch had a call to
> > > > > > > 
> > > > > > > +static bool
> > > > > > > +cref_has_const_field (tree ref)
> > > > > > > +{
> > > > > > > +  while (TREE_CODE (ref) == COMPONENT_REF)
> > > > > > > +    {
> > > > > > > +      if (CP_TYPE_CONST_P (TREE_TYPE (TREE_OPERAND (ref, 1))))
> > > > > > > +       return true;
> > > > > > > +      ref = TREE_OPERAND (ref, 0);
> > > > > > > +    }
> > > > > > > +  return false;
> > > > > > > +}
> > > > > > 
> > > > > > > here.  A problem arised when I checked even the outermost expression (which is not a
> > > > > > > field_decl), then I saw another problematical error.
> > > > > > > 
> > > > > > > The more outer fields are expected to be checked in subsequent calls to
> > > > > > > modifying_const_object_p in next iterations of the
> > > > > > > 
> > > > > > > 4459   for (tree probe = target; object == NULL_TREE; )
> > > > > > > 
> > > > > > > loop in cxx_eval_store_expression.
> > > > > > 
> > > > > > OK, but then why do you want to check two levels here rather than just one?
> > > > > 
> > > > > It's a hack to keep constexpr-tracking-const7.C working.  There we have
> > > > > 
> > > > >     b.a.c.d.n
> > > > > 
> > > > > wherein 'd' is const struct D, but 'n' isn't const.  Without the hack
> > > > > const_object_being_modified would be 'b.a.c.d', but due to the problem I
> > > > > desribed in the original mail[1] the constructor for D wouldn't have
> > > > > TREE_READONLY set.  With the hack const_object_being_modified will be
> > > > > 'b.a.c.d.n', which is of non-class type so we error:
> > > > > 
> > > > > 4710       if (!CLASS_TYPE_P (const_objtype))
> > > > > 4711         fail = true;
> > > > > 
> > > > > I could remove the hack and maybe XFAIL constexpr-tracking-const7.C if you
> > > > > want.  Unfortunately I wasn't aware of [1] when I added that feature and
> > > > > checking if the whole COMPONENT_REF is const seemed to be enough.
> > > 
> > > So if D was a wrapper around another class with the int field, this hack
> > > looking one level out wouldn't help?
> > 
> > Correct ;(.  I went back to my version using cref_has_const_field to keep
> > constexpr-tracking-const7.C and its derivates working.
> > 
> > > > > It's probably not a good idea to make this checking more strict at this
> > > > > stage.
> > > > > 
> > > > > [1] "While looking into this I noticed that we don't detect modifying a const
> > > > > object in certain cases like in
> > > > > <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94074#c2>.  That's because
> > > > > we never evaluate an X::X() CALL_EXPR -- there's none.  So there's no
> > > > > CONSTRUCTOR to set TREE_READONLY on.  No idea how to fix this, but it's
> > > > > likely something for GCC 11 anyway."
> > > How about this?
> > > 
> > 
> > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> > > index 76af0d710c4..b3d3499b9ac 100644
> > > --- a/gcc/cp/constexpr.c
> > > +++ b/gcc/cp/constexpr.c
> > > @@ -4759,6 +4759,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
> > >     else
> > >       *valp = init;
> > > +  /* After initialization, 'const' semantics apply to the value of the
> > > +     object. Make a note of this fact by marking the CONSTRUCTOR
> > > +     TREE_READONLY.  */
> > > +  if (TREE_CODE (t) == INIT_EXPR
> > > +      && TREE_CODE (*valp) == CONSTRUCTOR
> > > +      && TYPE_READONLY (type))
> > > +    TREE_READONLY (*valp) = true;
> > > +
> > >     /* Update TREE_CONSTANT and TREE_SIDE_EFFECTS on enclosing
> > >        CONSTRUCTORs, if any.  */
> > >     tree elt;
> > 
> > That works, thanks!  How about this, then?
> > 
> > Bootstrapped/regtested on x86_64-linux.
> > 
> > -- >8 --
> > I got a report that building Chromium fails with the "modifying a const
> > object" error.  After some poking I realized it's a bug in GCC, not in
> > their codebase.
> > 
> > Much like with ARRAY_REFs, which can be const even though the array
> > itself isn't, COMPONENT_REFs can be const although neither the object
> > nor the field were declared const.  So let's dial down the checking.
> > Here the COMPONENT_REF was const because of the "const_cast<const U &>(m)"
> > thing -- cxx_eval_component_reference then builds a COMPONENT_REF with
> > TREE_TYPE (t).
> > 
> > While looking into this I noticed that we don't detect modifying a const
> > object in certain cases like in
> > <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94074#c2>.  That's because
> > we never evaluate an X::X() CALL_EXPR -- there's none.  Fixed as per
> > Jason's suggestion by setting TREE_READONLY on a CONSTRUCTOR after
> > initialization in cxx_eval_store_expression.
> > 
> > 2020-03-11  Marek Polacek  <polacek@redhat.com>
> > 	    Jason Merrill  <jason@redhat.com>
> > 
> > 	PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
> > 	* constexpr.c (cref_has_const_field): New function.
> > 	(modifying_const_object_p): Consider a COMPONENT_REF
> > 	const only if any of its fields are const.
> > 	(cxx_eval_store_expression): Mark a CONSTRUCTOR of a const type
> > 	as readonly after its initialization has been done.
> > 
> > 	* g++.dg/cpp1y/constexpr-tracking-const17.C: New test.
> > 	* g++.dg/cpp1y/constexpr-tracking-const18.C: New test.
> > 	* g++.dg/cpp1y/constexpr-tracking-const19.C: New test.
> > 	* g++.dg/cpp1y/constexpr-tracking-const20.C: New test.
> > 	* g++.dg/cpp1y/constexpr-tracking-const21.C: New test.
> > 	* g++.dg/cpp1y/constexpr-tracking-const22.C: New test.
> > ---
> >   gcc/cp/constexpr.c                            | 41 ++++++++++++++++++-
> >   .../g++.dg/cpp1y/constexpr-tracking-const17.C | 23 +++++++++++
> >   .../g++.dg/cpp1y/constexpr-tracking-const18.C | 23 +++++++++++
> >   .../g++.dg/cpp1y/constexpr-tracking-const19.C | 23 +++++++++++
> >   .../g++.dg/cpp1y/constexpr-tracking-const20.C | 28 +++++++++++++
> >   .../g++.dg/cpp1y/constexpr-tracking-const21.C | 28 +++++++++++++
> >   .../g++.dg/cpp1y/constexpr-tracking-const22.C | 17 ++++++++
> >   7 files changed, 182 insertions(+), 1 deletion(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const17.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const18.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const19.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const20.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const21.C
> >   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const22.C
> > 
> > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
> > index 76af0d710c4..eeaba011a01 100644
> > --- a/gcc/cp/constexpr.c
> > +++ b/gcc/cp/constexpr.c
> > @@ -4384,6 +4384,21 @@ maybe_simplify_trivial_copy (tree &target, tree &init)
> >       }
> >   }
> > +/* Returns true if REF, which is a COMPONENT_REF, has any fields
> > +   of constant type.  */
> > +
> > +static bool
> > +cref_has_const_field (tree ref)
> > +{
> > +  while (TREE_CODE (ref) == COMPONENT_REF)
> > +    {
> > +      if (CP_TYPE_CONST_P (TREE_TYPE (TREE_OPERAND (ref, 1))))
> > +       return true;
> > +      ref = TREE_OPERAND (ref, 0);
> 
> I guess we don't need to check mutable here because the caller already does?
> That makes this function specific to this caller, though.  Please add a
> comment to that effect.  OK with that change.

Yes, mutable is expected to be checked outside this function.  Pushed with
that comment updated.  Thanks a lot.

Marek