* Re: [PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
[not found] ` <bd281bd4-d05a-1b28-b94a-a32c512582eb@redhat.com>
@ 2020-03-09 12:58 ` Jakub Jelinek
2020-03-09 13:19 ` Jason Merrill
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Jelinek @ 2020-03-09 12:58 UTC (permalink / raw)
To: Jason Merrill; +Cc: Marek Polacek, GCC Patches
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)];
Jakub
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
2020-03-09 12:58 ` [PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074] Jakub Jelinek
@ 2020-03-09 13:19 ` Jason Merrill
2020-03-09 13:40 ` Marek Polacek
0 siblings, 1 reply; 10+ messages in thread
From: Jason Merrill @ 2020-03-09 13:19 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Marek Polacek, GCC Patches
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:
> + 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?
Jason
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
2020-03-09 13:19 ` Jason Merrill
@ 2020-03-09 13:40 ` Marek Polacek
2020-03-09 19:37 ` Jason Merrill
0 siblings, 1 reply; 10+ messages in thread
From: Marek Polacek @ 2020-03-09 13:40 UTC (permalink / raw)
To: Jason Merrill; +Cc: Jakub Jelinek, GCC Patches
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.
Marek
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
2020-03-09 13:40 ` Marek Polacek
@ 2020-03-09 19:37 ` Jason Merrill
2020-03-09 20:25 ` Marek Polacek
0 siblings, 1 reply; 10+ messages in thread
From: Jason Merrill @ 2020-03-09 19:37 UTC (permalink / raw)
To: Marek Polacek; +Cc: Jakub Jelinek, GCC Patches
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?
Jason
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
2020-03-09 19:37 ` Jason Merrill
@ 2020-03-09 20:25 ` Marek Polacek
2020-03-09 20:34 ` Marek Polacek
0 siblings, 1 reply; 10+ messages in thread
From: Marek Polacek @ 2020-03-09 20:25 UTC (permalink / raw)
To: Jason Merrill; +Cc: Jakub Jelinek, GCC Patches
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."
Marek
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
2020-03-09 20:25 ` Marek Polacek
@ 2020-03-09 20:34 ` Marek Polacek
2020-03-10 19:46 ` Jason Merrill
0 siblings, 1 reply; 10+ messages in thread
From: Marek Polacek @ 2020-03-09 20:34 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
2020-03-09 20:34 ` Marek Polacek
@ 2020-03-10 19:46 ` Jason Merrill
2020-03-11 17:59 ` [PATCH v2] " Marek Polacek
0 siblings, 1 reply; 10+ messages in thread
From: Jason Merrill @ 2020-03-10 19:46 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 4777 bytes --]
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?
>> 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?
[-- Attachment #2: const.diff --]
[-- Type: text/x-patch, Size: 670 bytes --]
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;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
2020-03-10 19:46 ` Jason Merrill
@ 2020-03-11 17:59 ` Marek Polacek
2020-03-11 20:17 ` Jason Merrill
0 siblings, 1 reply; 10+ messages in thread
From: Marek Polacek @ 2020-03-11 17:59 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
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);
+ }
+ return false;
+}
+
/* Return true if we are modifying something that is const during constant
expression evaluation. CODE is the code of the statement, OBJ is the
object in question, MUTABLE_P is true if one of the subobjects were
@@ -4401,7 +4416,23 @@ modifying_const_object_p (tree_code code, tree obj, bool mutable_p)
if (mutable_p)
return false;
- return (TREE_READONLY (obj) || CP_TYPE_CONST_P (TREE_TYPE (obj)));
+ if (TREE_READONLY (obj))
+ return true;
+
+ if (CP_TYPE_CONST_P (TREE_TYPE (obj)))
+ {
+ /* Although a COMPONENT_REF may have a const type, we should
+ only consider it modifying a const object when any of the
+ field components is const. This can happen when using
+ constructs such as const_cast<const T &>(m), making something
+ const even though it wasn't declared const. */
+ if (TREE_CODE (obj) == COMPONENT_REF)
+ return cref_has_const_field (obj);
+ else
+ return true;
+ }
+
+ return false;
}
/* Evaluate an INIT_EXPR or MODIFY_EXPR. */
@@ -4759,6 +4790,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;
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const17.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const17.C
new file mode 100644
index 00000000000..3f215d28175
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const17.C
@@ -0,0 +1,23 @@
+// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
+// { dg-do compile { target c++14 } }
+
+typedef decltype (sizeof (0)) size_t;
+
+template <typename E, size_t N>
+struct array
+{
+ constexpr const E &operator[](size_t n) const noexcept { return elems[n]; }
+ E elems[N];
+};
+
+template <typename T>
+struct S {
+ using U = array<T, 4>;
+ U m;
+ constexpr S(int) : m{}
+ {
+ const_cast<int &>(const_cast<const U &>(m)[0]) = 42;
+ }
+};
+
+constexpr S<int> p = { 10 };
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const18.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const18.C
new file mode 100644
index 00000000000..11a680468c2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const18.C
@@ -0,0 +1,23 @@
+// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
+// { dg-do compile { target c++14 } }
+
+typedef decltype (sizeof (0)) size_t;
+
+template <typename E, size_t N>
+struct array
+{
+ constexpr const E &operator[](size_t n) const noexcept { return elems[n]; }
+ E elems[N];
+};
+
+template <typename T>
+struct S {
+ using U = array<T, 4>;
+ const U m;
+ constexpr S(int) : m{}
+ {
+ const_cast<int &>(const_cast<const U &>(m)[0]) = 42; // { dg-error "modifying a const object" }
+ }
+};
+
+constexpr S<int> p = { 10 }; // { dg-message "originally declared" }
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const19.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const19.C
new file mode 100644
index 00000000000..c31222ffcdd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const19.C
@@ -0,0 +1,23 @@
+// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
+// { dg-do compile { target c++14 } }
+
+typedef decltype (sizeof (0)) size_t;
+
+template <typename E, size_t N>
+struct array
+{
+ constexpr const E &operator[](size_t n) const noexcept { return elems[n]; }
+ const E elems[N];
+};
+
+template <typename T>
+struct S {
+ using U = array<T, 4>;
+ U m;
+ constexpr S(int) : m{}
+ {
+ const_cast<int &>(const_cast<const U &>(m)[0]) = 42; // { dg-error "modifying a const object" }
+ }
+};
+
+constexpr S<int> p = { 10 }; // { dg-message "originally declared" }
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const20.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const20.C
new file mode 100644
index 00000000000..2d5034945bd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const20.C
@@ -0,0 +1,28 @@
+// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
+// { dg-do compile { target c++14 } }
+
+typedef decltype (sizeof (0)) size_t;
+
+template <typename E, size_t N>
+struct array
+{
+ constexpr const E &operator[](size_t n) const noexcept { return elems[n]; }
+ E elems[N];
+};
+
+template <typename E, size_t N>
+struct array2 {
+ array<E, N> a;
+};
+
+template <typename T>
+struct S {
+ using U = array2<T, 4>;
+ U m;
+ constexpr S(int) : m{}
+ {
+ const_cast<int &>(const_cast<const U &>(m).a[0]) = 42;
+ }
+};
+
+constexpr S<int> p = { 10 };
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const21.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const21.C
new file mode 100644
index 00000000000..0b16193398e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const21.C
@@ -0,0 +1,28 @@
+// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
+// { dg-do compile { target c++14 } }
+
+typedef decltype (sizeof (0)) size_t;
+
+template <typename E, size_t N>
+struct array
+{
+ constexpr const E &operator[](size_t n) const noexcept { return elems[n]; }
+ E elems[N];
+};
+
+template <typename E, size_t N>
+struct array2 {
+ array<E, N> a;
+};
+
+template <typename T>
+struct S {
+ using U = array2<T, 4>;
+ const U m;
+ constexpr S(int) : m{}
+ {
+ const_cast<int &>(m.a[0]) = 42; // { dg-error "modifying a const object" }
+ }
+};
+
+constexpr S<int> p = { 10 }; // { dg-message "originally declared" }
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const22.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const22.C
new file mode 100644
index 00000000000..216cf1607a4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const22.C
@@ -0,0 +1,17 @@
+// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
+// { dg-do compile { target c++14 } }
+
+struct X {
+ int i;
+};
+
+template <typename T>
+struct S {
+ const X x;
+ constexpr S(int) : x{}
+ {
+ const_cast<X&>(x).i = 19; // { dg-error "modifying a const object" }
+ }
+};
+
+constexpr S<int> p = { 10 }; // { dg-message "originally declared" }
base-commit: cb99630f254aaec6591e0a200b79905b31d24eb3
--
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
2020-03-11 17:59 ` [PATCH v2] " Marek Polacek
@ 2020-03-11 20:17 ` Jason Merrill
2020-03-11 20:26 ` Marek Polacek
0 siblings, 1 reply; 10+ messages in thread
From: Jason Merrill @ 2020-03-11 20:17 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
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.
> + }
> + return false;
> +}
> +
> /* Return true if we are modifying something that is const during constant
> expression evaluation. CODE is the code of the statement, OBJ is the
> object in question, MUTABLE_P is true if one of the subobjects were
> @@ -4401,7 +4416,23 @@ modifying_const_object_p (tree_code code, tree obj, bool mutable_p)
> if (mutable_p)
> return false;
>
> - return (TREE_READONLY (obj) || CP_TYPE_CONST_P (TREE_TYPE (obj)));
> + if (TREE_READONLY (obj))
> + return true;
> +
> + if (CP_TYPE_CONST_P (TREE_TYPE (obj)))
> + {
> + /* Although a COMPONENT_REF may have a const type, we should
> + only consider it modifying a const object when any of the
> + field components is const. This can happen when using
> + constructs such as const_cast<const T &>(m), making something
> + const even though it wasn't declared const. */
> + if (TREE_CODE (obj) == COMPONENT_REF)
> + return cref_has_const_field (obj);
> + else
> + return true;
> + }
> +
> + return false;
> }
>
> /* Evaluate an INIT_EXPR or MODIFY_EXPR. */
> @@ -4759,6 +4790,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;
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const17.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const17.C
> new file mode 100644
> index 00000000000..3f215d28175
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const17.C
> @@ -0,0 +1,23 @@
> +// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
> +// { dg-do compile { target c++14 } }
> +
> +typedef decltype (sizeof (0)) size_t;
> +
> +template <typename E, size_t N>
> +struct array
> +{
> + constexpr const E &operator[](size_t n) const noexcept { return elems[n]; }
> + E elems[N];
> +};
> +
> +template <typename T>
> +struct S {
> + using U = array<T, 4>;
> + U m;
> + constexpr S(int) : m{}
> + {
> + const_cast<int &>(const_cast<const U &>(m)[0]) = 42;
> + }
> +};
> +
> +constexpr S<int> p = { 10 };
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const18.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const18.C
> new file mode 100644
> index 00000000000..11a680468c2
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const18.C
> @@ -0,0 +1,23 @@
> +// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
> +// { dg-do compile { target c++14 } }
> +
> +typedef decltype (sizeof (0)) size_t;
> +
> +template <typename E, size_t N>
> +struct array
> +{
> + constexpr const E &operator[](size_t n) const noexcept { return elems[n]; }
> + E elems[N];
> +};
> +
> +template <typename T>
> +struct S {
> + using U = array<T, 4>;
> + const U m;
> + constexpr S(int) : m{}
> + {
> + const_cast<int &>(const_cast<const U &>(m)[0]) = 42; // { dg-error "modifying a const object" }
> + }
> +};
> +
> +constexpr S<int> p = { 10 }; // { dg-message "originally declared" }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const19.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const19.C
> new file mode 100644
> index 00000000000..c31222ffcdd
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const19.C
> @@ -0,0 +1,23 @@
> +// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
> +// { dg-do compile { target c++14 } }
> +
> +typedef decltype (sizeof (0)) size_t;
> +
> +template <typename E, size_t N>
> +struct array
> +{
> + constexpr const E &operator[](size_t n) const noexcept { return elems[n]; }
> + const E elems[N];
> +};
> +
> +template <typename T>
> +struct S {
> + using U = array<T, 4>;
> + U m;
> + constexpr S(int) : m{}
> + {
> + const_cast<int &>(const_cast<const U &>(m)[0]) = 42; // { dg-error "modifying a const object" }
> + }
> +};
> +
> +constexpr S<int> p = { 10 }; // { dg-message "originally declared" }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const20.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const20.C
> new file mode 100644
> index 00000000000..2d5034945bd
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const20.C
> @@ -0,0 +1,28 @@
> +// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
> +// { dg-do compile { target c++14 } }
> +
> +typedef decltype (sizeof (0)) size_t;
> +
> +template <typename E, size_t N>
> +struct array
> +{
> + constexpr const E &operator[](size_t n) const noexcept { return elems[n]; }
> + E elems[N];
> +};
> +
> +template <typename E, size_t N>
> +struct array2 {
> + array<E, N> a;
> +};
> +
> +template <typename T>
> +struct S {
> + using U = array2<T, 4>;
> + U m;
> + constexpr S(int) : m{}
> + {
> + const_cast<int &>(const_cast<const U &>(m).a[0]) = 42;
> + }
> +};
> +
> +constexpr S<int> p = { 10 };
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const21.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const21.C
> new file mode 100644
> index 00000000000..0b16193398e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const21.C
> @@ -0,0 +1,28 @@
> +// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
> +// { dg-do compile { target c++14 } }
> +
> +typedef decltype (sizeof (0)) size_t;
> +
> +template <typename E, size_t N>
> +struct array
> +{
> + constexpr const E &operator[](size_t n) const noexcept { return elems[n]; }
> + E elems[N];
> +};
> +
> +template <typename E, size_t N>
> +struct array2 {
> + array<E, N> a;
> +};
> +
> +template <typename T>
> +struct S {
> + using U = array2<T, 4>;
> + const U m;
> + constexpr S(int) : m{}
> + {
> + const_cast<int &>(m.a[0]) = 42; // { dg-error "modifying a const object" }
> + }
> +};
> +
> +constexpr S<int> p = { 10 }; // { dg-message "originally declared" }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const22.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const22.C
> new file mode 100644
> index 00000000000..216cf1607a4
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-tracking-const22.C
> @@ -0,0 +1,17 @@
> +// PR c++/94074 - wrong modifying const object error for COMPONENT_REF.
> +// { dg-do compile { target c++14 } }
> +
> +struct X {
> + int i;
> +};
> +
> +template <typename T>
> +struct S {
> + const X x;
> + constexpr S(int) : x{}
> + {
> + const_cast<X&>(x).i = 19; // { dg-error "modifying a const object" }
> + }
> +};
> +
> +constexpr S<int> p = { 10 }; // { dg-message "originally declared" }
>
> base-commit: cb99630f254aaec6591e0a200b79905b31d24eb3
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
2020-03-11 20:17 ` Jason Merrill
@ 2020-03-11 20:26 ` Marek Polacek
0 siblings, 0 replies; 10+ messages in thread
From: Marek Polacek @ 2020-03-11 20:26 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-03-11 20:27 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200306235406.1943931-1-polacek@redhat.com>
[not found] ` <bd281bd4-d05a-1b28-b94a-a32c512582eb@redhat.com>
2020-03-09 12:58 ` [PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074] 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
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
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).