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

[Nathan Sidwell <nathan@acm.org>]

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?

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