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