[PATCH] c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
Jason Merrill
jason@redhat.com
Mon Mar 9 19:37:56 GMT 2020
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
More information about the Gcc-patches
mailing list