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 <ppa...@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?
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
