| Message ID | 20200306235406.1943931-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074] | expand |
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? Should that build a VIEW_CONVERT_EXPR instead? > 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. > > PR c++/94074 - wrong modifying const object error for COMPONENT_REF. > * constexpr.c (modifying_const_object_p): Consider a COMPONENT_REF > const only if its object or field are const. > > * 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. > --- > gcc/cp/constexpr.c | 30 ++++++++++++++++++- > .../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 +++++++++++++++++ > 5 files changed, 126 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 > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > index 521c87f6210..936d171b9e4 100644 > --- a/gcc/cp/constexpr.c > +++ b/gcc/cp/constexpr.c > @@ -4401,7 +4401,35 @@ 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 the field > + or object 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) > + { > + 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)); > + } > + } > + else > + return true; > + } > + > + return false; > } > > /* Evaluate an INIT_EXPR or MODIFY_EXPR. */ > 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 }; > > base-commit: 191bcd0f30dd37dec773efb0125afdcae9bd90ef >
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
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
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
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
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
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
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?
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
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 >
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
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 521c87f6210..936d171b9e4 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -4401,7 +4401,35 @@ 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 the field + or object 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) + { + 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)); + } + } + else + return true; + } + + return false; } /* Evaluate an INIT_EXPR or MODIFY_EXPR. */ 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 };