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

[Patrick Palka <ppalka@redhat.com>]

On Fri, 21 Oct 2022, Nathan Sidwell wrote:

> On 10/19/22 09:55, Patrick Palka wrote:
> > On Wed, 19 Oct 2022, Richard Biener wrote:
> > 
> > > On Tue, Oct 18, 2022 at 8:26 PM Patrick Palka <ppalka@redhat.com> wrote:
> > > > 
> > > > On Fri, 14 Oct 2022, Richard Biener wrote:
> > > > 
> > > > > 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.
> > > > 
> > > > *nod* reading in the enum before reading in the typedef seems like
> > > > the most direct solution, though not sure how to accomplish that :/
> > > 
> > > For LTO streaming we DFS walk tree edges from all entries into the tree
> > > graph we want to stream, collecting and streaming SCCs.  Not sure if
> > > doing similar for module streaming would help this case though.
> > 
> > FWIW I managed to obtain a more interesting reduction for this ICE, one
> > that doesn't use a typedef bound to the same name as the enum:
> > 
> > $ cat 106848_a.H
> > template<typename _T1>
> > struct pair {
> >    using type = void(*)(const _T1&);
> > };
> > struct _ScannerBase {
> >    enum _TokenT { _S_token_anychar };
> >    pair<_TokenT> _M_token_tbl;
> > };
> > 
> > $ cat 106848_b.C
> > import "106848_a.H";
> > 
> > using type = _ScannerBase;
> > 
> > $ g++ -fmodules-ts -g 106848_a.H 106848_b.C
> > 106848_b.C:3:14: error: type variant differs by TYPE_MAX_VALUE
> > <enumeral_type 0x7f252c757f18 _TokenT ...>
> > <enumeral_type 0x7f252c757f18 _TokenT ...>
> > 
> > Like in the less interesting testcase, the problem is ultimately that we
> > create a variant of the enum (as part of reading in pair<_TokenT>::type)
> > before reading the enum's definition, thus the variant inherits stale
> > TYPE_MIN/MAX_VALUE.
> > 
> > Perhaps pair<_TokenT>::type should indirectly depend on the definition
> > of _TokenT -- but IIUC we generally don't require a type to be defined
> > in order to refer to it, so enforcing such a dependency would be a
> > pessimization I think.
> > 
> > So ISTM this isn't a dependency issue (pair<_TokenT>::type already
> > implicitly depends on the ENUMERAL_TYPE, just not also the enum's
> > defining TYPE_DECL), and the true issue is that we're streaming
> > TYPE_MIN/MAX_VALUE only as part of an enum's definition, which the
> > linked patch fixes.
> 
> Thanks for the explanation, it's a situation I didn;t anticipate and your fix
> is good.  Could you add a comment about why you need to propagate the values
> though?

Thanks a lot, will do.  Just to make sure since there are multiple
solutions proposed, do you prefer to go with
https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603487.html
or
https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603831.html ?

Both solutions fix the PR106848 issue (empty TYPE_MIN/MAX_VALUE on an
enum type variant), but the latter also fixes the related PR102600
(empty TYPE_MIN/MAX_VALUE on the main variant of an enum with no
enumerators).  (We could maybe even combine the two solutions: stream
TYPE_MIN/MAX_VALUE as part of ENUMERAL_TYPE, and also update TYPE_VALUES
of each variant during trees_in::read_enum_def)

> 
> nathan
> 
> > 
> > > 
> > > > A somewhat orthogonal issue (that incidentally fixes this testcase) is
> > > > that we stream TYPE_MIN/MAX_VALUE only for enums with a definition, but
> > > > the frontend sets these fields even for opaque enums.  If we make sure
> > > > to stream these fields for all ENUMERAL_TYPEs, then we won't have to
> > > > worry about these fields being stale for variants that may have been
> > > > created before reading in the enum definition (their TYPE_VALUES field
> > > > will still be stale I guess, but verify_type doesn't worry about that
> > > > it seems, so we avoid the ICE).
> > > > 
> > > > patch to that effect is at
> > > > https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603831.html
> > > > 
> > > > > 
> > > > > 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
> > > > > > 
> > > > > 
> > > > > 
> > > > 
> > > 
> > > 
> > 
> 
> -- 
> Nathan Sidwell
> 
>