From: Patrick Palka <ppalka@redhat.com>
To: Richard Biener <richard.guenther@gmail.com>
Cc: Patrick Palka <ppalka@redhat.com>,
gcc-patches@gcc.gnu.org, nathan@acm.org
Subject: Re: [PATCH] c++ modules: verify_type failure with typedef enum [PR106848]
Date: Wed, 19 Oct 2022 09:55:17 -0400 (EDT) [thread overview]
Message-ID: <5601279b-939f-4492-c14b-c495d7a2a3b2@idea> (raw)
In-Reply-To: <CAFiYyc2rC+XrKWKDi_5yw7FRhTs9JiNwYJKdOfEuRs4BBVydHQ@mail.gmail.com>
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.
>
> > 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
> > > >
> > >
> > >
> >
>
>
next prev parent reply other threads:[~2022-10-19 13:55 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-13 15:39 Patrick Palka
2022-10-14 6:04 ` Richard Biener
2022-10-18 18:26 ` Patrick Palka
2022-10-19 7:33 ` Richard Biener
2022-10-19 13:55 ` Patrick Palka [this message]
2022-10-21 12:36 ` Nathan Sidwell
2022-10-21 13:11 ` Patrick Palka
2022-10-24 12:39 ` Nathan Sidwell
2022-10-25 17:46 ` Patrick Palka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5601279b-939f-4492-c14b-c495d7a2a3b2@idea \
--to=ppalka@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=nathan@acm.org \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).