c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]

Message ID 20200306235406.1943931-1-polacek@redhat.com
State New
Headers show
Series
  • c++: Fix wrong modifying const object error for COMPONENT_REF [PR94074]
Related show

Commit Message

Marek Polacek March 6, 2020, 11:54 p.m.
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.  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


base-commit: 191bcd0f30dd37dec773efb0125afdcae9bd90ef
-- 
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA

Comments

Jason Merrill March 7, 2020, 12:43 a.m. | #1
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

>
Jakub Jelinek March 9, 2020, 12:58 p.m. | #2
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
Jason Merrill March 9, 2020, 1:19 p.m. | #3
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
Marek Polacek March 9, 2020, 1:40 p.m. | #4
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
Jason Merrill March 9, 2020, 7:37 p.m. | #5
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
Marek Polacek March 9, 2020, 8:25 p.m. | #6
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
Marek Polacek March 9, 2020, 8:34 p.m. | #7
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
Jakub Jelinek via Gcc-patches March 10, 2020, 7:46 p.m. | #8
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?
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;
Jakub Jelinek via Gcc-patches March 11, 2020, 5:59 p.m. | #9
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
-- 
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
Jakub Jelinek via Gcc-patches March 11, 2020, 8:17 p.m. | #10
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

>
Jakub Jelinek via Gcc-patches March 11, 2020, 8:26 p.m. | #11
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

Patch

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