From: Marek Polacek <polacek@redhat.com>
To: Jason Merrill <jason@redhat.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
Date: Mon, 9 Mar 2020 16:34:25 -0400 [thread overview]
Message-ID: <20200309203425.GC3554@redhat.com> (raw)
In-Reply-To: <20200309202500.GB3554@redhat.com>
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.
>
> 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."
The testcase disappeared from Bugzilla, but it was
<https://paste.centos.org/view/fc9527f6>.
Marek
next prev parent reply other threads:[~2020-03-09 20:34 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20200306235406.1943931-1-polacek@redhat.com>
[not found] ` <bd281bd4-d05a-1b28-b94a-a32c512582eb@redhat.com>
2020-03-09 12:58 ` Jakub Jelinek
2020-03-09 13:19 ` Jason Merrill
2020-03-09 13:40 ` Marek Polacek
2020-03-09 19:37 ` Jason Merrill
2020-03-09 20:25 ` Marek Polacek
2020-03-09 20:34 ` Marek Polacek [this message]
2020-03-10 19:46 ` Jason Merrill
2020-03-11 17:59 ` [PATCH v2] " Marek Polacek
2020-03-11 20:17 ` Jason Merrill
2020-03-11 20:26 ` 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=20200309203425.GC3554@redhat.com \
--to=polacek@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@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).