Re: [PATCH] c++ modules: verify_type failure with typedef enum [PR106848]

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Oct 13, 2022 at 5:40 PM Patrick Palka via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Here 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_VALUES and TYPE_MIN/MAX_VALUES, which leads to an ICE (with -g).  The
> stale variant got created from set_underlying_type during earlier stream in
> of the (redundant) typedef for the enum.
>
> This patch works around this by setting TYPE_VALUES and TYPE_MIN/MAX_VALUES
> for all variants when reading in an enum definition.  Does this look like
> the right approach?  Or perhaps we need to arrange that we read the enum
> definition before reading in the typedef decl?  Note that seems to be an
> issue only when the typedef name and enum names are the same (thus the
> typedef is redundant), otherwise we seem to read the enum definition first
> as desired.
>
>         PR c++/106848
>
> gcc/cp/ChangeLog:
>
>         * module.cc (trees_in::read_enum_def): Set the TYPE_VALUES,
>         TYPE_MIN_VALUE and TYPE_MAX_VALUE of all type variants.
>
> gcc/testsuite/ChangeLog:
>
>         * g++.dg/modules/enum-9_a.H: New test.
>         * g++.dg/modules/enum-9_b.C: New test.
> ---
>  gcc/cp/module.cc                        | 9 ++++++---
>  gcc/testsuite/g++.dg/modules/enum-9_a.H | 5 +++++
>  gcc/testsuite/g++.dg/modules/enum-9_b.C | 6 ++++++
>  3 files changed, 17 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/enum-9_a.H
>  create mode 100644 gcc/testsuite/g++.dg/modules/enum-9_b.C
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 7ffeefa7c1f..97fb80bcd44 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -12303,9 +12303,12 @@ 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;
> +      for (tree t = type; t; t = TYPE_NEXT_VARIANT (t))
> +       {
> +         TYPE_VALUES (t) = values;
> +         TYPE_MIN_VALUE (t) = min;
> +         TYPE_MAX_VALUE (t) = max;
> +       }

it's definitely somewhat ugly but at least type_hash_canon doesn't hash
these for ENUMERAL_TYPE (but it does compare them!  which in principle
means it could as well hash them ...)

I think that if you read both from the same module that you should arrange
to read what you refer to first?  But maybe that's not the actual issue here.

Richard.

>
>        rest_of_type_compilation (type, DECL_NAMESPACE_SCOPE_P (defn));
>      }
> diff --git a/gcc/testsuite/g++.dg/modules/enum-9_a.H b/gcc/testsuite/g++.dg/modules/enum-9_a.H
> new file mode 100644
> index 00000000000..fb7d10ad3b6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/enum-9_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-9_b.C b/gcc/testsuite/g++.dg/modules/enum-9_b.C
> new file mode 100644
> index 00000000000..63e81675d0a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/enum-9_b.C
> @@ -0,0 +1,6 @@
> +// PR c++/106848
> +// { dg-additional-options "-fmodules-ts -g" }
> +
> +import "enum-9_a.H";
> +
> +memory_order x = memory_order_seq_cst;
> --
> 2.38.0.68.ge85701b4af
>