[2/2] c++ modules: always stream TYPE_MIN/MAX_VALUE for enums [PR106848]
Checks
Commit Message
We currently stream TYPE_MIN/MAX_VALUE of an enum only if the enum is
defined rather than just declared. But that seems inconsistent with
what start_enum does, which always sets TYPE_MIN/MAX_VALUE on the
ENUMERAL_TYPE (via copy_type_enum) even for an opaque enum that lacks a
definition.
This patch makes us stream TYPE_MIN/MAX_VALUE as part of the
ENUMERAL_TYPE rather than as part of the defining TYPE_DECL so that
TYPE_MIN/MAX_VALUE gets set even for opaque enums.
Incidentally, this turns out to fix the below testcase in which during
stream in we end up having created a type variant for the enum before we
read the enum's definition, and thus the variant inherited stale
TYPE_MIN/MAX_VALUE, which leads to an ICE from verify_type (with -g).
(The stale variant got created from set_underlying_type during earlier
stream in of the redundant typedef for the enum.) By streaming these
fields as part of the ENUMERAL_TYPE, we guarantee they won't be stale.
Tested on x86_64-pc-linux-gnu, does this look OK for trunk?
PR c++/106848
gcc/cp/ChangeLog:
* module.cc (trees_out::core_vals): Stream TYPE_MAX_VALUE and
TYPE_MIN_VALUE of ENUMERAL_TYPE.
(trees_in::core_vals): Likewise.
(trees_out::write_enum_def): Don't stream them here.
(trees_in::read_enum_def): Likewise.
gcc/testsuite/ChangeLog:
* g++.dg/modules/enum-10_a.H: New test.
* g++.dg/modules/enum-10_b.C: New test.
---
gcc/cp/module.cc | 29 ++++++++++++------------
gcc/testsuite/g++.dg/modules/enum-10_a.H | 5 ++++
gcc/testsuite/g++.dg/modules/enum-10_b.C | 6 +++++
3 files changed, 25 insertions(+), 15 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/enum-10_a.H
create mode 100644 gcc/testsuite/g++.dg/modules/enum-10_b.C
Comments
On Tue, Oct 18, 2022 at 8:11 PM Patrick Palka via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> We currently stream TYPE_MIN/MAX_VALUE of an enum only if the enum is
> defined rather than just declared. But that seems inconsistent with
> what start_enum does, which always sets TYPE_MIN/MAX_VALUE on the
> ENUMERAL_TYPE (via copy_type_enum) even for an opaque enum that lacks a
> definition.
>
> This patch makes us stream TYPE_MIN/MAX_VALUE as part of the
> ENUMERAL_TYPE rather than as part of the defining TYPE_DECL so that
> TYPE_MIN/MAX_VALUE gets set even for opaque enums.
>
> Incidentally, this turns out to fix the below testcase in which during
> stream in we end up having created a type variant for the enum before we
> read the enum's definition, and thus the variant inherited stale
> TYPE_MIN/MAX_VALUE, which leads to an ICE from verify_type (with -g).
> (The stale variant got created from set_underlying_type during earlier
> stream in of the redundant typedef for the enum.) By streaming these
> fields as part of the ENUMERAL_TYPE, we guarantee they won't be stale.
>
> Tested on x86_64-pc-linux-gnu, does this look OK for trunk?
Looks reasonable to me.
Thanks,
Richard.
> PR c++/106848
>
> gcc/cp/ChangeLog:
>
> * module.cc (trees_out::core_vals): Stream TYPE_MAX_VALUE and
> TYPE_MIN_VALUE of ENUMERAL_TYPE.
> (trees_in::core_vals): Likewise.
> (trees_out::write_enum_def): Don't stream them here.
> (trees_in::read_enum_def): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/enum-10_a.H: New test.
> * g++.dg/modules/enum-10_b.C: New test.
> ---
> gcc/cp/module.cc | 29 ++++++++++++------------
> gcc/testsuite/g++.dg/modules/enum-10_a.H | 5 ++++
> gcc/testsuite/g++.dg/modules/enum-10_b.C | 6 +++++
> 3 files changed, 25 insertions(+), 15 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/enum-10_a.H
> create mode 100644 gcc/testsuite/g++.dg/modules/enum-10_b.C
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index cc704817718..bb406a5cf01 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -6016,9 +6016,14 @@ trees_out::core_vals (tree t)
>
> if (CODE_CONTAINS_STRUCT (code, TS_TYPE_NON_COMMON))
> {
> + if (code == ENUMERAL_TYPE)
> + {
> + WT (t->type_non_common.maxval);
> + WT (t->type_non_common.minval);
> + }
> /* Records and unions hold FIELDS, VFIELD & BINFO on these
> things. */
> - if (!RECORD_OR_UNION_CODE_P (code) && code != ENUMERAL_TYPE)
> + else if (!RECORD_OR_UNION_CODE_P (code))
> {
> // FIXME: These are from tpl_parm_value's 'type' writing.
> // Perhaps it should just be doing them directly?
> @@ -6529,9 +6534,14 @@ trees_in::core_vals (tree t)
>
> if (CODE_CONTAINS_STRUCT (code, TS_TYPE_NON_COMMON))
> {
> + if (code == ENUMERAL_TYPE)
> + {
> + RT (t->type_non_common.maxval);
> + RT (t->type_non_common.minval);
> + }
> /* Records and unions hold FIELDS, VFIELD & BINFO on these
> things. */
> - if (!RECORD_OR_UNION_CODE_P (code) && code != ENUMERAL_TYPE)
> + else if (!RECORD_OR_UNION_CODE_P (code))
> {
> /* This is not clobbering TYPE_CACHED_VALUES, because this
> is a type that doesn't have any. */
> @@ -12217,8 +12227,6 @@ trees_out::write_enum_def (tree decl)
> tree type = TREE_TYPE (decl);
>
> tree_node (TYPE_VALUES (type));
> - tree_node (TYPE_MIN_VALUE (type));
> - tree_node (TYPE_MAX_VALUE (type));
> }
>
> void
> @@ -12242,8 +12250,6 @@ trees_in::read_enum_def (tree defn, tree maybe_template)
> {
> tree type = TREE_TYPE (defn);
> tree values = tree_node ();
> - tree min = tree_node ();
> - tree max = tree_node ();
>
> if (get_overrun ())
> return false;
> @@ -12254,8 +12260,6 @@ trees_in::read_enum_def (tree defn, tree maybe_template)
> if (installing)
> {
> TYPE_VALUES (type) = values;
> - TYPE_MIN_VALUE (type) = min;
> - TYPE_MAX_VALUE (type) = max;
>
> rest_of_type_compilation (type, DECL_NAMESPACE_SCOPE_P (defn));
> }
> @@ -12269,22 +12273,17 @@ trees_in::read_enum_def (tree defn, tree maybe_template)
> tree new_decl = TREE_VALUE (values);
>
> if (DECL_NAME (known_decl) != DECL_NAME (new_decl))
> - goto bad;
> + break;
>
> new_decl = maybe_duplicate (new_decl);
>
> if (!cp_tree_equal (DECL_INITIAL (known_decl),
> DECL_INITIAL (new_decl)))
> - goto bad;
> + break;
> }
>
> if (known || values)
> - goto bad;
> -
> - if (!cp_tree_equal (TYPE_MIN_VALUE (type), min)
> - || !cp_tree_equal (TYPE_MAX_VALUE (type), max))
> {
> - bad:;
> error_at (DECL_SOURCE_LOCATION (maybe_dup),
> "definition of %qD does not match", maybe_dup);
> inform (DECL_SOURCE_LOCATION (defn),
> diff --git a/gcc/testsuite/g++.dg/modules/enum-10_a.H b/gcc/testsuite/g++.dg/modules/enum-10_a.H
> new file mode 100644
> index 00000000000..fb7d10ad3b6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/enum-10_a.H
> @@ -0,0 +1,5 @@
> +// PR c++/106848
> +// { dg-additional-options -fmodule-header }
> +// { dg-module-cmi {} }
> +
> +typedef enum memory_order { memory_order_seq_cst } memory_order;
> diff --git a/gcc/testsuite/g++.dg/modules/enum-10_b.C b/gcc/testsuite/g++.dg/modules/enum-10_b.C
> new file mode 100644
> index 00000000000..76dc3152963
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/enum-10_b.C
> @@ -0,0 +1,6 @@
> +// PR c++/106848
> +// { dg-additional-options "-fmodules-ts -g" }
> +
> +import "enum-10_a.H";
> +
> +memory_order x = memory_order_seq_cst;
> --
> 2.38.0.118.g4732897cf0
>
@@ -6016,9 +6016,14 @@ trees_out::core_vals (tree t)
if (CODE_CONTAINS_STRUCT (code, TS_TYPE_NON_COMMON))
{
+ if (code == ENUMERAL_TYPE)
+ {
+ WT (t->type_non_common.maxval);
+ WT (t->type_non_common.minval);
+ }
/* Records and unions hold FIELDS, VFIELD & BINFO on these
things. */
- if (!RECORD_OR_UNION_CODE_P (code) && code != ENUMERAL_TYPE)
+ else if (!RECORD_OR_UNION_CODE_P (code))
{
// FIXME: These are from tpl_parm_value's 'type' writing.
// Perhaps it should just be doing them directly?
@@ -6529,9 +6534,14 @@ trees_in::core_vals (tree t)
if (CODE_CONTAINS_STRUCT (code, TS_TYPE_NON_COMMON))
{
+ if (code == ENUMERAL_TYPE)
+ {
+ RT (t->type_non_common.maxval);
+ RT (t->type_non_common.minval);
+ }
/* Records and unions hold FIELDS, VFIELD & BINFO on these
things. */
- if (!RECORD_OR_UNION_CODE_P (code) && code != ENUMERAL_TYPE)
+ else if (!RECORD_OR_UNION_CODE_P (code))
{
/* This is not clobbering TYPE_CACHED_VALUES, because this
is a type that doesn't have any. */
@@ -12217,8 +12227,6 @@ trees_out::write_enum_def (tree decl)
tree type = TREE_TYPE (decl);
tree_node (TYPE_VALUES (type));
- tree_node (TYPE_MIN_VALUE (type));
- tree_node (TYPE_MAX_VALUE (type));
}
void
@@ -12242,8 +12250,6 @@ trees_in::read_enum_def (tree defn, tree maybe_template)
{
tree type = TREE_TYPE (defn);
tree values = tree_node ();
- tree min = tree_node ();
- tree max = tree_node ();
if (get_overrun ())
return false;
@@ -12254,8 +12260,6 @@ trees_in::read_enum_def (tree defn, tree maybe_template)
if (installing)
{
TYPE_VALUES (type) = values;
- TYPE_MIN_VALUE (type) = min;
- TYPE_MAX_VALUE (type) = max;
rest_of_type_compilation (type, DECL_NAMESPACE_SCOPE_P (defn));
}
@@ -12269,22 +12273,17 @@ trees_in::read_enum_def (tree defn, tree maybe_template)
tree new_decl = TREE_VALUE (values);
if (DECL_NAME (known_decl) != DECL_NAME (new_decl))
- goto bad;
+ break;
new_decl = maybe_duplicate (new_decl);
if (!cp_tree_equal (DECL_INITIAL (known_decl),
DECL_INITIAL (new_decl)))
- goto bad;
+ break;
}
if (known || values)
- goto bad;
-
- if (!cp_tree_equal (TYPE_MIN_VALUE (type), min)
- || !cp_tree_equal (TYPE_MAX_VALUE (type), max))
{
- bad:;
error_at (DECL_SOURCE_LOCATION (maybe_dup),
"definition of %qD does not match", maybe_dup);
inform (DECL_SOURCE_LOCATION (defn),
new file mode 100644
@@ -0,0 +1,5 @@
+// PR c++/106848
+// { dg-additional-options -fmodule-header }
+// { dg-module-cmi {} }
+
+typedef enum memory_order { memory_order_seq_cst } memory_order;
new file mode 100644
@@ -0,0 +1,6 @@
+// PR c++/106848
+// { dg-additional-options "-fmodules-ts -g" }
+
+import "enum-10_a.H";
+
+memory_order x = memory_order_seq_cst;