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 <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);
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
