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 <pola...@redhat.com> Jason Merrill <ja...@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