c++: constexpr RANGE_EXPR ctor indexes [PR95241]

Message ID 20200527205816.3647257-1-ppalka@redhat.com
State New
Headers show
Series
  • c++: constexpr RANGE_EXPR ctor indexes [PR95241]
Related show

Commit Message

Kees Cook via Gcc-patches May 27, 2020, 8:58 p.m.
In the testcase below, the CONSTRUCTOR for 'field' contains a
RANGE_EXPR index:

  {aggr_init_expr<...>, [1...2]={.off=1}}

but get_or_insert_ctor_field isn't prepared to handle RANGE_EXPR
indexes.

This patch adds limited support for RANGE_EXPR indexes to
get_or_insert_ctor_field.  The limited scope of this patch should make
it more suitable for backporting, and support for more access patterns
would be needed only to handle self-modifying CONSTRUCTORs containing a
RANGE_EXPR index, but I haven't yet been able to come up with a testcase
that exhibits such a CONSTRUCTOR.

Passes 'make check-c++', does this look OK to commit to master and to
the GCC 10 branch after a full bootstrap and regtest?

gcc/cp/ChangeLog:

	PR c++/95241
	* constexpr.c (get_or_insert_ctor_field): Add limited support
	for RANGE_EXPR indexes.

gcc/testsuite/ChangeLog:

	PR c++/95241
	* g++.dg/cpp0x/constexpr-array25.C: New test.
---
 gcc/cp/constexpr.c                            | 12 +++++++++++
 .../g++.dg/cpp0x/constexpr-array25.C          | 21 +++++++++++++++++++
 2 files changed, 33 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C

-- 
2.27.0.rc1.5.gae92ac8ae3

Comments

Kees Cook via Gcc-patches May 27, 2020, 9:03 p.m. | #1
On Wed, 27 May 2020, Patrick Palka wrote:

> In the testcase below, the CONSTRUCTOR for 'field' contains a

> RANGE_EXPR index:

> 

>   {aggr_init_expr<...>, [1...2]={.off=1}}

> 

> but get_or_insert_ctor_field isn't prepared to handle RANGE_EXPR

> indexes.

> 

> This patch adds limited support for RANGE_EXPR indexes to

> get_or_insert_ctor_field.  The limited scope of this patch should make

> it more suitable for backporting, and support for more access patterns

> would be needed only to handle self-modifying CONSTRUCTORs containing a

> RANGE_EXPR index, but I haven't yet been able to come up with a testcase

> that exhibits such a CONSTRUCTOR.

> 

> Passes 'make check-c++', does this look OK to commit to master and to

> the GCC 10 branch after a full bootstrap and regtest?

> 

> gcc/cp/ChangeLog:

> 

> 	PR c++/95241

> 	* constexpr.c (get_or_insert_ctor_field): Add limited support

> 	for RANGE_EXPR indexes.

> 

> gcc/testsuite/ChangeLog:

> 

> 	PR c++/95241

> 	* g++.dg/cpp0x/constexpr-array25.C: New test.

> ---

>  gcc/cp/constexpr.c                            | 12 +++++++++++

>  .../g++.dg/cpp0x/constexpr-array25.C          | 21 +++++++++++++++++++

>  2 files changed, 33 insertions(+)

>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C

> 

> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c

> index 4e441ac8d2f..6f9bafbe8d8 100644

> --- a/gcc/cp/constexpr.c

> +++ b/gcc/cp/constexpr.c

> @@ -3301,6 +3301,18 @@ get_or_insert_ctor_field (tree ctor, tree index, int pos_hint = -1)

>      }

>    else if (TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == VECTOR_TYPE)

>      {

> +      if (TREE_CODE (index) == RANGE_EXPR)

> +	{

> +	  /* Our support for RANGE_EXPR indexes is limited to accessing an

> +	     existing one via POS_HINT, and appending a new one to the end of

> +	     CTOR.  ??? Support for other access patterns might be needed.  */

> +	  tree lo = TREE_OPERAND (index, 0);

> +	  auto *elts = CONSTRUCTOR_ELTS (ctor);

> +	  gcc_assert (vec_safe_is_empty (elts)

> +		      || array_index_cmp (lo, elts->last().index) > 0);

> +	  return vec_safe_push (elts, {index, NULL_TREE});

> +	}

> +


Oops, it just occurred to me that the use of C++11 features here would
make this patch unsuitable for backporting.  C++98-compatible patch
incoming...

>        HOST_WIDE_INT i = find_array_ctor_elt (ctor, index, /*insert*/true);

>        gcc_assert (i >= 0);

>        constructor_elt *cep = CONSTRUCTOR_ELT (ctor, i);

> diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C

> new file mode 100644

> index 00000000000..9162943249f

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C

> @@ -0,0 +1,21 @@

> +// PR c++/95241

> +// { dg-do compile { target c++11 } }

> +

> +struct Fragment

> +{

> +  int off;

> +  constexpr Fragment(int _off) : off(_off) { }

> +  constexpr Fragment() : Fragment(1) { }

> +};

> +

> +struct Field

> +{

> +  Fragment fragments[3];

> +  constexpr Field(int off) : fragments{{off}} { }

> +};

> +

> +constexpr Field field{0};

> +

> +static_assert(field.fragments[0].off == 0

> +	      && field.fragments[1].off == 1

> +	      && field.fragments[2].off == 1, "");

> -- 

> 2.27.0.rc1.5.gae92ac8ae3

> 

>
Kees Cook via Gcc-patches May 27, 2020, 9:15 p.m. | #2
On Wed, 27 May 2020, Patrick Palka wrote:

> On Wed, 27 May 2020, Patrick Palka wrote:

> 

> > In the testcase below, the CONSTRUCTOR for 'field' contains a

> > RANGE_EXPR index:

> > 

> >   {aggr_init_expr<...>, [1...2]={.off=1}}

> > 

> > but get_or_insert_ctor_field isn't prepared to handle RANGE_EXPR

> > indexes.

> > 

> > This patch adds limited support for RANGE_EXPR indexes to

> > get_or_insert_ctor_field.  The limited scope of this patch should make

> > it more suitable for backporting, and support for more access patterns

> > would be needed only to handle self-modifying CONSTRUCTORs containing a

> > RANGE_EXPR index, but I haven't yet been able to come up with a testcase

> > that exhibits such a CONSTRUCTOR.

> > 

> > Passes 'make check-c++', does this look OK to commit to master and to

> > the GCC 10 branch after a full bootstrap and regtest?

> > 

> > gcc/cp/ChangeLog:

> > 

> > 	PR c++/95241

> > 	* constexpr.c (get_or_insert_ctor_field): Add limited support

> > 	for RANGE_EXPR indexes.

> > 

> > gcc/testsuite/ChangeLog:

> > 

> > 	PR c++/95241

> > 	* g++.dg/cpp0x/constexpr-array25.C: New test.

> > ---

> >  gcc/cp/constexpr.c                            | 12 +++++++++++

> >  .../g++.dg/cpp0x/constexpr-array25.C          | 21 +++++++++++++++++++

> >  2 files changed, 33 insertions(+)

> >  create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C

> > 

> > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c

> > index 4e441ac8d2f..6f9bafbe8d8 100644

> > --- a/gcc/cp/constexpr.c

> > +++ b/gcc/cp/constexpr.c

> > @@ -3301,6 +3301,18 @@ get_or_insert_ctor_field (tree ctor, tree index, int pos_hint = -1)

> >      }

> >    else if (TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == VECTOR_TYPE)

> >      {

> > +      if (TREE_CODE (index) == RANGE_EXPR)

> > +	{

> > +	  /* Our support for RANGE_EXPR indexes is limited to accessing an

> > +	     existing one via POS_HINT, and appending a new one to the end of

> > +	     CTOR.  ??? Support for other access patterns might be needed.  */

> > +	  tree lo = TREE_OPERAND (index, 0);

> > +	  auto *elts = CONSTRUCTOR_ELTS (ctor);

> > +	  gcc_assert (vec_safe_is_empty (elts)

> > +		      || array_index_cmp (lo, elts->last().index) > 0);

> > +	  return vec_safe_push (elts, {index, NULL_TREE});

> > +	}

> > +

> 

> Oops, it just occurred to me that the use of C++11 features here would

> make this patch unsuitable for backporting.  C++98-compatible patch

> incoming...


Here it is.  Does the following look OK to commit to master and to the
GCC 10 branch after a full bootstrap and regtest?

-- >8 --

Subject: [PATCH] c++: constexpr RANGE_EXPR ctor indexes [PR95241]

In the testcase below, the CONSTRUCTOR for 'field' contains a
RANGE_EXPR index:

  {aggr_init_expr<...>, [1...2]={.off=1}}

but get_or_insert_ctor_field isn't prepared to handle RANGE_EXPR
indexes.

This patch adds limited support for RANGE_EXPR indexes to
get_or_insert_ctor_field.  The limited scope of this patch should make
it more suitable for backporting, and support for more access patterns
would be needed only to handle self-modifying CONSTRUCTORs containing a
RANGE_EXPR index, but I haven't yet been able to come up with a testcase
that exhibits such a CONSTRUCTOR.

gcc/cp/ChangeLog:

	PR c++/95241
	* constexpr.c (get_or_insert_ctor_field): Add limited support
	for RANGE_EXPR indexes.

gcc/testsuite/ChangeLog:

	PR c++/95241
	* g++.dg/cpp0x/constexpr-array25.C: New test.
---
 gcc/cp/constexpr.c                            | 15 +++++++++++++
 .../g++.dg/cpp0x/constexpr-array25.C          | 21 +++++++++++++++++++
 2 files changed, 36 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 4e441ac8d2f..32f2ef96fc7 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -3301,6 +3301,21 @@ get_or_insert_ctor_field (tree ctor, tree index, int pos_hint = -1)
     }
   else if (TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == VECTOR_TYPE)
     {
+      if (TREE_CODE (index) == RANGE_EXPR)
+	{
+	  /* ??? Support for RANGE_EXPR indexes is currently limited to
+	     accessing one via POS_HINT, or appending a new one to the end
+	     of CTOR.  Support for other access patterns may be needed.  */
+	  vec<constructor_elt, va_gc> *elts = CONSTRUCTOR_ELTS (ctor);
+	  if (vec_safe_length (elts))
+	    {
+	      tree lo = TREE_OPERAND (index, 0);
+	      gcc_assert (array_index_cmp (lo, elts->last().index) > 0);
+	    }
+	  CONSTRUCTOR_APPEND_ELT (elts, index, NULL_TREE);
+	  return &elts->last();
+	}
+
       HOST_WIDE_INT i = find_array_ctor_elt (ctor, index, /*insert*/true);
       gcc_assert (i >= 0);
       constructor_elt *cep = CONSTRUCTOR_ELT (ctor, i);
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C
new file mode 100644
index 00000000000..9162943249f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C
@@ -0,0 +1,21 @@
+// PR c++/95241
+// { dg-do compile { target c++11 } }
+
+struct Fragment
+{
+  int off;
+  constexpr Fragment(int _off) : off(_off) { }
+  constexpr Fragment() : Fragment(1) { }
+};
+
+struct Field
+{
+  Fragment fragments[3];
+  constexpr Field(int off) : fragments{{off}} { }
+};
+
+constexpr Field field{0};
+
+static_assert(field.fragments[0].off == 0
+	      && field.fragments[1].off == 1
+	      && field.fragments[2].off == 1, "");
-- 
2.27.0.rc1.5.gae92ac8ae3
Kees Cook via Gcc-patches May 28, 2020, 8:44 p.m. | #3
On 5/27/20 5:15 PM, Patrick Palka wrote:
> On Wed, 27 May 2020, Patrick Palka wrote:

> 

>> On Wed, 27 May 2020, Patrick Palka wrote:

>>

>>> In the testcase below, the CONSTRUCTOR for 'field' contains a

>>> RANGE_EXPR index:

>>>

>>>    {aggr_init_expr<...>, [1...2]={.off=1}}

>>>

>>> but get_or_insert_ctor_field isn't prepared to handle RANGE_EXPR

>>> indexes.

>>>

>>> This patch adds limited support for RANGE_EXPR indexes to

>>> get_or_insert_ctor_field.  The limited scope of this patch should make

>>> it more suitable for backporting, and support for more access patterns

>>> would be needed only to handle self-modifying CONSTRUCTORs containing a

>>> RANGE_EXPR index, but I haven't yet been able to come up with a testcase

>>> that exhibits such a CONSTRUCTOR.

>>>

>>> Passes 'make check-c++', does this look OK to commit to master and to

>>> the GCC 10 branch after a full bootstrap and regtest?


OK.

>>> gcc/cp/ChangeLog:

>>>

>>> 	PR c++/95241

>>> 	* constexpr.c (get_or_insert_ctor_field): Add limited support

>>> 	for RANGE_EXPR indexes.

>>>

>>> gcc/testsuite/ChangeLog:

>>>

>>> 	PR c++/95241

>>> 	* g++.dg/cpp0x/constexpr-array25.C: New test.

>>> ---

>>>   gcc/cp/constexpr.c                            | 12 +++++++++++

>>>   .../g++.dg/cpp0x/constexpr-array25.C          | 21 +++++++++++++++++++

>>>   2 files changed, 33 insertions(+)

>>>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C

>>>

>>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c

>>> index 4e441ac8d2f..6f9bafbe8d8 100644

>>> --- a/gcc/cp/constexpr.c

>>> +++ b/gcc/cp/constexpr.c

>>> @@ -3301,6 +3301,18 @@ get_or_insert_ctor_field (tree ctor, tree index, int pos_hint = -1)

>>>       }

>>>     else if (TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == VECTOR_TYPE)

>>>       {

>>> +      if (TREE_CODE (index) == RANGE_EXPR)

>>> +	{

>>> +	  /* Our support for RANGE_EXPR indexes is limited to accessing an

>>> +	     existing one via POS_HINT, and appending a new one to the end of

>>> +	     CTOR.  ??? Support for other access patterns might be needed.  */

>>> +	  tree lo = TREE_OPERAND (index, 0);

>>> +	  auto *elts = CONSTRUCTOR_ELTS (ctor);

>>> +	  gcc_assert (vec_safe_is_empty (elts)

>>> +		      || array_index_cmp (lo, elts->last().index) > 0);

>>> +	  return vec_safe_push (elts, {index, NULL_TREE});

>>> +	}

>>> +

>>

>> Oops, it just occurred to me that the use of C++11 features here would

>> make this patch unsuitable for backporting.  C++98-compatible patch

>> incoming...

> 

> Here it is.  Does the following look OK to commit to master and to the

> GCC 10 branch after a full bootstrap and regtest?

> 

> -- >8 --

> 

> Subject: [PATCH] c++: constexpr RANGE_EXPR ctor indexes [PR95241]

> 

> In the testcase below, the CONSTRUCTOR for 'field' contains a

> RANGE_EXPR index:

> 

>    {aggr_init_expr<...>, [1...2]={.off=1}}

> 

> but get_or_insert_ctor_field isn't prepared to handle RANGE_EXPR

> indexes.

> 

> This patch adds limited support for RANGE_EXPR indexes to

> get_or_insert_ctor_field.  The limited scope of this patch should make

> it more suitable for backporting, and support for more access patterns

> would be needed only to handle self-modifying CONSTRUCTORs containing a

> RANGE_EXPR index, but I haven't yet been able to come up with a testcase

> that exhibits such a CONSTRUCTOR.

> 

> gcc/cp/ChangeLog:

> 

> 	PR c++/95241

> 	* constexpr.c (get_or_insert_ctor_field): Add limited support

> 	for RANGE_EXPR indexes.

> 

> gcc/testsuite/ChangeLog:

> 

> 	PR c++/95241

> 	* g++.dg/cpp0x/constexpr-array25.C: New test.

> ---

>   gcc/cp/constexpr.c                            | 15 +++++++++++++

>   .../g++.dg/cpp0x/constexpr-array25.C          | 21 +++++++++++++++++++

>   2 files changed, 36 insertions(+)

>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C

> 

> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c

> index 4e441ac8d2f..32f2ef96fc7 100644

> --- a/gcc/cp/constexpr.c

> +++ b/gcc/cp/constexpr.c

> @@ -3301,6 +3301,21 @@ get_or_insert_ctor_field (tree ctor, tree index, int pos_hint = -1)

>       }

>     else if (TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == VECTOR_TYPE)

>       {

> +      if (TREE_CODE (index) == RANGE_EXPR)

> +	{

> +	  /* ??? Support for RANGE_EXPR indexes is currently limited to

> +	     accessing one via POS_HINT, or appending a new one to the end

> +	     of CTOR.  Support for other access patterns may be needed.  */

> +	  vec<constructor_elt, va_gc> *elts = CONSTRUCTOR_ELTS (ctor);

> +	  if (vec_safe_length (elts))

> +	    {

> +	      tree lo = TREE_OPERAND (index, 0);

> +	      gcc_assert (array_index_cmp (lo, elts->last().index) > 0);

> +	    }

> +	  CONSTRUCTOR_APPEND_ELT (elts, index, NULL_TREE);

> +	  return &elts->last();

> +	}

> +

>         HOST_WIDE_INT i = find_array_ctor_elt (ctor, index, /*insert*/true);

>         gcc_assert (i >= 0);

>         constructor_elt *cep = CONSTRUCTOR_ELT (ctor, i);

> diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C

> new file mode 100644

> index 00000000000..9162943249f

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C

> @@ -0,0 +1,21 @@

> +// PR c++/95241

> +// { dg-do compile { target c++11 } }

> +

> +struct Fragment

> +{

> +  int off;

> +  constexpr Fragment(int _off) : off(_off) { }

> +  constexpr Fragment() : Fragment(1) { }

> +};

> +

> +struct Field

> +{

> +  Fragment fragments[3];

> +  constexpr Field(int off) : fragments{{off}} { }

> +};

> +

> +constexpr Field field{0};

> +

> +static_assert(field.fragments[0].off == 0

> +	      && field.fragments[1].off == 1

> +	      && field.fragments[2].off == 1, "");

>

Patch

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 4e441ac8d2f..6f9bafbe8d8 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -3301,6 +3301,18 @@  get_or_insert_ctor_field (tree ctor, tree index, int pos_hint = -1)
     }
   else if (TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == VECTOR_TYPE)
     {
+      if (TREE_CODE (index) == RANGE_EXPR)
+	{
+	  /* Our support for RANGE_EXPR indexes is limited to accessing an
+	     existing one via POS_HINT, and appending a new one to the end of
+	     CTOR.  ??? Support for other access patterns might be needed.  */
+	  tree lo = TREE_OPERAND (index, 0);
+	  auto *elts = CONSTRUCTOR_ELTS (ctor);
+	  gcc_assert (vec_safe_is_empty (elts)
+		      || array_index_cmp (lo, elts->last().index) > 0);
+	  return vec_safe_push (elts, {index, NULL_TREE});
+	}
+
       HOST_WIDE_INT i = find_array_ctor_elt (ctor, index, /*insert*/true);
       gcc_assert (i >= 0);
       constructor_elt *cep = CONSTRUCTOR_ELT (ctor, i);
diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C b/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C
new file mode 100644
index 00000000000..9162943249f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-array25.C
@@ -0,0 +1,21 @@ 
+// PR c++/95241
+// { dg-do compile { target c++11 } }
+
+struct Fragment
+{
+  int off;
+  constexpr Fragment(int _off) : off(_off) { }
+  constexpr Fragment() : Fragment(1) { }
+};
+
+struct Field
+{
+  Fragment fragments[3];
+  constexpr Field(int off) : fragments{{off}} { }
+};
+
+constexpr Field field{0};
+
+static_assert(field.fragments[0].off == 0
+	      && field.fragments[1].off == 1
+	      && field.fragments[2].off == 1, "");