| Message ID | 20240229205639.4112668-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++/modules: depending local enums [PR104919, PR106009] | expand |
On 2/29/24 15:56, Patrick Palka wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > OK for trunk? > > -- >8 -- > > For local enums defined in a non-template function or a function template > instantiation it seems we neglect to make the function depend on the enum > definition, which ultimately causes streaming to fail due to the enum > definition not being streamed before uses of its enumerators are streamed, > as far as I can tell. I would think that the function doesn't need to depend on the local enum in order for the local enum to be streamed before the use of the enumerator, which comes after the definition of the enum in the function body? Why isn't streaming the body of the function outputting the enum definition before the use of the enumerator? > This was nearly enough to make things work, except we now ran into > issues with the local TYPE/CONST_DECL copies when streaming the > constexpr version of a function body. It occurred to me that we don't > need to make copies of local types when copying a constexpr function > body; only VAR_DECLs etc need to be copied for sake of recursive > constexpr calls. So this patch adjusts copy_fn accordingly. Maybe adjust can_be_nonlocal instead? It seems unnecessary in general to remap types and enumerators for inlining. Jason
On Fri, 1 Mar 2024, Jason Merrill wrote: > On 2/29/24 15:56, Patrick Palka wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > > OK for trunk? > > > > -- >8 -- > > > > For local enums defined in a non-template function or a function template > > instantiation it seems we neglect to make the function depend on the enum > > definition, which ultimately causes streaming to fail due to the enum > > definition not being streamed before uses of its enumerators are streamed, > > as far as I can tell. > > I would think that the function doesn't need to depend on the local enum in > order for the local enum to be streamed before the use of the enumerator, > which comes after the definition of the enum in the function body? > > Why isn't streaming the body of the function outputting the enum definition > before the use of the enumerator? IIUC (based on observing the behavior for local classes) streaming the definition of a local class/enum as part of the function definition is what we want to avoid; we want to treat a local type definition as a logically separate definition and stream it separately (similar to class defns vs member defns I guess). And by not registering a dependency between the function and the local enum, we end up never streaming out the local enum definition separately and instead stream it out as part of the function definition (accidentally) which we then can't stream in properly. Perhaps the motivation for treating local type definitions as logically separate from the function definition is because they can leak out of a function with a deduced return type: auto f() { struct A { }; return A(); } using type = decltype(f()); // refers directly to f()::A It's also consistent with templated local types getting their own TEMPLATE_DECL (which Nathan revisited in r11-4529-g9703b8d98c116e). > > > This was nearly enough to make things work, except we now ran into > > issues with the local TYPE/CONST_DECL copies when streaming the > > constexpr version of a function body. It occurred to me that we don't > > need to make copies of local types when copying a constexpr function > > body; only VAR_DECLs etc need to be copied for sake of recursive > > constexpr calls. So this patch adjusts copy_fn accordingly. > > Maybe adjust can_be_nonlocal instead? It seems unnecessary in general to > remap types and enumerators for inlining. That seems to work nicely too. I'm testing a patch to that effect with all front ends and will report back. > > Jason > >
On 3/1/24 10:00, Patrick Palka wrote: > On Fri, 1 Mar 2024, Jason Merrill wrote: > >> On 2/29/24 15:56, Patrick Palka wrote: >>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look >>> OK for trunk? >>> >>> -- >8 -- >>> >>> For local enums defined in a non-template function or a function template >>> instantiation it seems we neglect to make the function depend on the enum >>> definition, which ultimately causes streaming to fail due to the enum >>> definition not being streamed before uses of its enumerators are streamed, >>> as far as I can tell. >> >> I would think that the function doesn't need to depend on the local enum in >> order for the local enum to be streamed before the use of the enumerator, >> which comes after the definition of the enum in the function body? >> >> Why isn't streaming the body of the function outputting the enum definition >> before the use of the enumerator? > > IIUC (based on observing the behavior for local classes) streaming the > definition of a local class/enum as part of the function definition is > what we want to avoid; we want to treat a local type definition as a > logically separate definition and stream it separately (similar > to class defns vs member defns I guess). And by not registering a dependency > between the function and the local enum, we end up never streaming out > the local enum definition separately and instead stream it out as part > of the function definition (accidentally) which we then can't stream in > properly. > > Perhaps the motivation for treating local type definitions as logically > separate from the function definition is because they can leak out of a > function with a deduced return type: > > auto f() { > struct A { }; > return A(); > } > > using type = decltype(f()); // refers directly to f()::A Yes, I believe that's what modules.cc refers to as a "voldemort". But for non-voldemort local types, the declaration of the function doesn't depend on them, only the definition. Why doesn't streaming them in the definition work properly? Jason
On 3/1/24 10:32, Jason Merrill wrote: > On 3/1/24 10:00, Patrick Palka wrote: >> On Fri, 1 Mar 2024, Jason Merrill wrote: >> >>> On 2/29/24 15:56, Patrick Palka wrote: >>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look >>>> OK for trunk? >>>> >>>> -- >8 -- >>>> >>>> For local enums defined in a non-template function or a function >>>> template >>>> instantiation it seems we neglect to make the function depend on the >>>> enum >>>> definition, which ultimately causes streaming to fail due to the enum >>>> definition not being streamed before uses of its enumerators are >>>> streamed, >>>> as far as I can tell. >>> >>> I would think that the function doesn't need to depend on the local >>> enum in >>> order for the local enum to be streamed before the use of the >>> enumerator, >>> which comes after the definition of the enum in the function body? >>> >>> Why isn't streaming the body of the function outputting the enum >>> definition >>> before the use of the enumerator? >> >> IIUC (based on observing the behavior for local classes) streaming the >> definition of a local class/enum as part of the function definition is >> what we want to avoid; we want to treat a local type definition as a >> logically separate definition and stream it separately (similar >> to class defns vs member defns I guess). And by not registering a >> dependency >> between the function and the local enum, we end up never streaming out >> the local enum definition separately and instead stream it out as part >> of the function definition (accidentally) which we then can't stream in >> properly. >> >> Perhaps the motivation for treating local type definitions as logically >> separate from the function definition is because they can leak out of a >> function with a deduced return type: >> >> auto f() { >> struct A { }; >> return A(); >> } >> >> using type = decltype(f()); // refers directly to f()::A > > Yes, I believe that's what modules.cc refers to as a "voldemort". > > But for non-voldemort local types, the declaration of the function > doesn't depend on them, only the definition. Why doesn't streaming them > in the definition work properly? And does your 99426 patch address that problem? Jason
On Fri, 1 Mar 2024, Jason Merrill wrote: > On 3/1/24 10:00, Patrick Palka wrote: > > On Fri, 1 Mar 2024, Jason Merrill wrote: > > > > > On 2/29/24 15:56, Patrick Palka wrote: > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > > > > OK for trunk? > > > > > > > > -- >8 -- > > > > > > > > For local enums defined in a non-template function or a function > > > > template > > > > instantiation it seems we neglect to make the function depend on the > > > > enum > > > > definition, which ultimately causes streaming to fail due to the enum > > > > definition not being streamed before uses of its enumerators are > > > > streamed, > > > > as far as I can tell. > > > > > > I would think that the function doesn't need to depend on the local enum > > > in > > > order for the local enum to be streamed before the use of the enumerator, > > > which comes after the definition of the enum in the function body? > > > > > > Why isn't streaming the body of the function outputting the enum > > > definition > > > before the use of the enumerator? > > > > IIUC (based on observing the behavior for local classes) streaming the > > definition of a local class/enum as part of the function definition is > > what we want to avoid; we want to treat a local type definition as a > > logically separate definition and stream it separately (similar > > to class defns vs member defns I guess). And by not registering a > > dependency > > between the function and the local enum, we end up never streaming out > > the local enum definition separately and instead stream it out as part > > of the function definition (accidentally) which we then can't stream in > > properly. > > > > Perhaps the motivation for treating local type definitions as logically > > separate from the function definition is because they can leak out of a > > function with a deduced return type: > > > > auto f() { > > struct A { }; > > return A(); > > } > > > > using type = decltype(f()); // refers directly to f()::A > > Yes, I believe that's what modules.cc refers to as a "voldemort". > > But for non-voldemort local types, the declaration of the function doesn't > depend on them, only the definition. Why doesn't streaming them in the > definition work properly? I should note that for a templated local type we already always add a dependency between the function template _pattern_ and the local type _pattern_ and therefore always stream the local type pattern separately (even if its not actually a voldemort), thanks to the TREE_CODE (decl) == TEMPLATE_DECL case guarding the add_dependency call (inside a template pattern we see the TEMPLATE_DECL of the local TYPE_DECL). The dependency is missing only when the function is a non-template or non-template-pattern. My patch makes us consistently add the dependency and in turn consistently stream the definitions separately. (For a local _class_, in the non-template and non-template-pattern case we currently add a dependency between the function and the injected-class-name of the class as opposed to the class itself, which seems quite accidental but suffices. And that's why only local enums are problematic currently. After my patch we instead add a dependency to the local class itself.) Part of the puzzle of why we don't/can't stream them as part of the function definition is because we don't mark the enumerators for by-value walking when marking the function definition. So when streaming out the enumerator definition we stream out _references_ to the enumerators (tt_const_decl tags) instead of the actual definitions which breaks stream-in. The best place to mark local types for by-value walking would be in trees_out::mark_function_def which is suspiciously empty! I experimented with (never mind that it only marks the outermost block's types): @@ -11713,8 +11713,12 @@ trees_out::write_function_def (tree decl) } void -trees_out::mark_function_def (tree) +trees_out::mark_function_def (tree decl) { + tree initial = DECL_INITIAL (decl); + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) + if (DECL_IMPLICIT_TYPEDEF_P (var)) + mark_declaration (var, true); } Which actually fixes the non-template PR104919 testcase, but it breaks streaming of templated local types wherein we run into the sanity check: @@ -7677,16 +7677,6 @@ trees_out::decl_value (tree decl, depset *dep) merge_kind mk = get_merge_kind (decl, dep); ! if (CHECKING_P) ! { ! /* Never start in the middle of a template. */ ! int use_tpl = -1; ! if (tree ti = node_template_info (decl, use_tpl)) ! gcc_checking_assert (TREE_CODE (TI_TEMPLATE (ti)) == OVERLOAD ! || TREE_CODE (TI_TEMPLATE (ti)) == FIELD_DECL ! || (DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) ! != decl)); ! } if (streaming_p ()) { If we try to work around this sanity check by only marking local types when inside a non-template and non-template-pattern (i.e. inside an instantiation): @@ -11713,8 +11713,16 @@ trees_out::write_function_def (tree decl) } void -trees_out::mark_function_def (tree) +trees_out::mark_function_def (tree decl) { + tree initial = DECL_INITIAL (decl); + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) + if (DECL_IMPLICIT_TYPEDEF_P (var)) + { + tree ti = get_template_info (var); + if (!ti || DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) != var) + mark_declaration (var, true); + } } Then we trip over this other sanity check (when streaming a local TYPE_DECL from a function template instantiation): gcc/cp/module.cc @@ -7859,16 +7859,6 @@ trees_out::decl_value (tree decl, depset *dep) tree_node (get_constraints (decl)); } ! if (streaming_p ()) ! { ! /* Do not stray outside this section. */ ! gcc_checking_assert (!dep || dep->section == dep_hash->section); ! ! /* Write the entity index, so we can insert it as soon as we ! know this is new. */ ! install_entity (decl, dep); ! } ! if (DECL_LANG_SPECIFIC (inner) && DECL_MODULE_KEYED_DECLS_P (inner) && !is_key_order ()) At this point I gave up on this approach. It seems simpler to just consistently add the dependencies. > > Jason > >
On Fri, 1 Mar 2024, Jason Merrill wrote: > On 3/1/24 10:32, Jason Merrill wrote: > > On 3/1/24 10:00, Patrick Palka wrote: > > > On Fri, 1 Mar 2024, Jason Merrill wrote: > > > > > > > On 2/29/24 15:56, Patrick Palka wrote: > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > > > > > OK for trunk? > > > > > > > > > > -- >8 -- > > > > > > > > > > For local enums defined in a non-template function or a function > > > > > template > > > > > instantiation it seems we neglect to make the function depend on the > > > > > enum > > > > > definition, which ultimately causes streaming to fail due to the enum > > > > > definition not being streamed before uses of its enumerators are > > > > > streamed, > > > > > as far as I can tell. > > > > > > > > I would think that the function doesn't need to depend on the local enum > > > > in > > > > order for the local enum to be streamed before the use of the > > > > enumerator, > > > > which comes after the definition of the enum in the function body? > > > > > > > > Why isn't streaming the body of the function outputting the enum > > > > definition > > > > before the use of the enumerator? > > > > > > IIUC (based on observing the behavior for local classes) streaming the > > > definition of a local class/enum as part of the function definition is > > > what we want to avoid; we want to treat a local type definition as a > > > logically separate definition and stream it separately (similar > > > to class defns vs member defns I guess). And by not registering a > > > dependency > > > between the function and the local enum, we end up never streaming out > > > the local enum definition separately and instead stream it out as part > > > of the function definition (accidentally) which we then can't stream in > > > properly. > > > > > > Perhaps the motivation for treating local type definitions as logically > > > separate from the function definition is because they can leak out of a > > > function with a deduced return type: > > > > > > auto f() { > > > struct A { }; > > > return A(); > > > } > > > > > > using type = decltype(f()); // refers directly to f()::A > > > > Yes, I believe that's what modules.cc refers to as a "voldemort". > > > > But for non-voldemort local types, the declaration of the function doesn't > > depend on them, only the definition. Why doesn't streaming them in the > > definition work properly? > > And does your 99426 patch address that problem? I don't think so, that patch should only affect declaration merging (of a streamed-in local type with the corresponding in-TU local type after their containing function is merged). > > This was nearly enough to make things work, except we now ran into > > issues with the local TYPE/CONST_DECL copies when streaming the > > constexpr version of a function body. It occurred to me that we don't > > need to make copies of local types when copying a constexpr function > > body; only VAR_DECLs etc need to be copied for sake of recursive > > constexpr calls. So this patch adjusts copy_fn accordingly. > > Maybe adjust can_be_nonlocal instead? It seems unnecessary in general > to remap types and enumerators for inlining. Unfortunately this approached caused a boostrap failure with Ada: raised STORAGE_ERROR : stack overflow or erroneous memory access The patch was --- a/gcc/tree-inline.cc +++ b/gcc/tree-inline.cc @@ -725,6 +725,9 @@ can_be_nonlocal (tree decl, copy_body_data *id) if (TREE_CODE (decl) == FUNCTION_DECL) return true; + if (TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == CONST_DECL) + return true; + /* Local static vars must be non-local or we get multiple declaration problems. */ if (VAR_P (decl) && !auto_var_in_fn_p (decl, id->src_fn)) > > Jason > >
On Fri, 1 Mar 2024, Patrick Palka wrote: > On Fri, 1 Mar 2024, Jason Merrill wrote: > > > On 3/1/24 10:00, Patrick Palka wrote: > > > On Fri, 1 Mar 2024, Jason Merrill wrote: > > > > > > > On 2/29/24 15:56, Patrick Palka wrote: > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > > > > > OK for trunk? > > > > > > > > > > -- >8 -- > > > > > > > > > > For local enums defined in a non-template function or a function > > > > > template > > > > > instantiation it seems we neglect to make the function depend on the > > > > > enum > > > > > definition, which ultimately causes streaming to fail due to the enum > > > > > definition not being streamed before uses of its enumerators are > > > > > streamed, > > > > > as far as I can tell. > > > > > > > > I would think that the function doesn't need to depend on the local enum > > > > in > > > > order for the local enum to be streamed before the use of the enumerator, > > > > which comes after the definition of the enum in the function body? > > > > > > > > Why isn't streaming the body of the function outputting the enum > > > > definition > > > > before the use of the enumerator? > > > > > > IIUC (based on observing the behavior for local classes) streaming the > > > definition of a local class/enum as part of the function definition is > > > what we want to avoid; we want to treat a local type definition as a > > > logically separate definition and stream it separately (similar > > > to class defns vs member defns I guess). And by not registering a > > > dependency > > > between the function and the local enum, we end up never streaming out > > > the local enum definition separately and instead stream it out as part > > > of the function definition (accidentally) which we then can't stream in > > > properly. > > > > > > Perhaps the motivation for treating local type definitions as logically > > > separate from the function definition is because they can leak out of a > > > function with a deduced return type: > > > > > > auto f() { > > > struct A { }; > > > return A(); > > > } > > > > > > using type = decltype(f()); // refers directly to f()::A > > > > Yes, I believe that's what modules.cc refers to as a "voldemort". > > > > But for non-voldemort local types, the declaration of the function doesn't > > depend on them, only the definition. Why doesn't streaming them in the > > definition work properly? > > I should note that for a templated local type we already always add a > dependency between the function template _pattern_ and the local type > _pattern_ and therefore always stream the local type pattern separately > (even if its not actually a voldemort), thanks to the TREE_CODE (decl) == TEMPLATE_DECL > case guarding the add_dependency call (inside a template pattern we > see the TEMPLATE_DECL of the local TYPE_DECL). The dependency is > missing only when the function is a non-template or non-template-pattern. > My patch makes us consistently add the dependency and in turn consistently stream the definitions separately. > > (For a local _class_, in the non-template and non-template-pattern case > we currently add a dependency between the function and the > injected-class-name of the class as opposed to the class itself, which > seems quite accidental but suffices. And that's why only local enums > are problematic currently. After my patch we instead add a dependency > to the local class itself.) > > Part of the puzzle of why we don't/can't stream them as part of the > function definition is because we don't mark the enumerators for > by-value walking when marking the function definition. So when > streaming out the enumerator definition we stream out _references_ > to the enumerators (tt_const_decl tags) instead of the actual > definitions which breaks stream-in. > > The best place to mark local types for by-value walking would be > in trees_out::mark_function_def which is suspiciously empty! I > experimented with (never mind that it only marks the outermost block's > types): > > @@ -11713,8 +11713,12 @@ trees_out::write_function_def (tree decl) > } > > void > -trees_out::mark_function_def (tree) > +trees_out::mark_function_def (tree decl) > { > + tree initial = DECL_INITIAL (decl); > + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) > + if (DECL_IMPLICIT_TYPEDEF_P (var)) > + mark_declaration (var, true); > } > > Which actually fixes the non-template PR104919 testcase, but it > breaks streaming of templated local types wherein we run into > the sanity check: > > @@ -7677,16 +7677,6 @@ trees_out::decl_value (tree decl, depset *dep) > > merge_kind mk = get_merge_kind (decl, dep); > > ! if (CHECKING_P) > ! { > ! /* Never start in the middle of a template. */ > ! int use_tpl = -1; > ! if (tree ti = node_template_info (decl, use_tpl)) > ! gcc_checking_assert (TREE_CODE (TI_TEMPLATE (ti)) == OVERLOAD > ! || TREE_CODE (TI_TEMPLATE (ti)) == FIELD_DECL > ! || (DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) > ! != decl)); > ! } > > if (streaming_p ()) > { > > If we try to work around this sanity check by only marking local types > when inside a non-template and non-template-pattern (i.e. inside an > instantiation): > > @@ -11713,8 +11713,16 @@ trees_out::write_function_def (tree decl) > } > > void > -trees_out::mark_function_def (tree) > +trees_out::mark_function_def (tree decl) > { > + tree initial = DECL_INITIAL (decl); > + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) > + if (DECL_IMPLICIT_TYPEDEF_P (var)) > + { > + tree ti = get_template_info (var); > + if (!ti || DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) != var) > + mark_declaration (var, true); > + } > } > > Then we trip over this other sanity check (when streaming a local > TYPE_DECL from a function template instantiation): > > gcc/cp/module.cc > @@ -7859,16 +7859,6 @@ trees_out::decl_value (tree decl, depset *dep) > tree_node (get_constraints (decl)); > } > > ! if (streaming_p ()) > ! { > ! /* Do not stray outside this section. */ > ! gcc_checking_assert (!dep || dep->section == dep_hash->section); > ! > ! /* Write the entity index, so we can insert it as soon as we > ! know this is new. */ > ! install_entity (decl, dep); > ! } > ! > if (DECL_LANG_SPECIFIC (inner) > && DECL_MODULE_KEYED_DECLS_P (inner) > && !is_key_order ()) > > At this point I gave up on this approach. It seems simpler to just > consistently add the dependencies. Ah, it just occurred to me we might as well remove the sneakoscope flag altogether, since the code it affects is only used for dependency analysis (!streaming_p ()) which happens from find_dependencies. This seems like a nice simplification and passes the testsuite and other smoke tests. -- >8 -- Subject: [PATCH] c++/modules: depending local enums [PR104919, PR106009] PR c++/104919 PR c++/106009 gcc/cp/ChangeLog: * module.cc (depset::hash::sneakoscope): Remove. (trees_out::decl_node): Depend on non-templated non-template-pattern local types as well. (depset::hash::find_dependencies): Remove sneakoscope code. gcc/ChangeLog: * tree-inline.cc (remap_decl): Handle copy_decl returning the original decl. (remap_decls): Handle remap_decl returning the original decl. (copy_fn): Adjust copy_decl callback to skip TYPE_DECL and CONST_DECL. gcc/testsuite/ChangeLog: * g++.dg/modules/tdef-7.h: Remove outdated comment. * g++.dg/modules/tdef-7_b.C: Don't expect two TYPE_DECLs. * g++.dg/modules/enum-13_a.C: New test. * g++.dg/modules/enum-13_b.C: New test. --- gcc/cp/module.cc | 12 ++---------- gcc/testsuite/g++.dg/modules/enum-13_a.C | 23 +++++++++++++++++++++++ gcc/testsuite/g++.dg/modules/enum-13_b.C | 8 ++++++++ gcc/testsuite/g++.dg/modules/tdef-7.h | 2 -- gcc/testsuite/g++.dg/modules/tdef-7_b.C | 2 +- gcc/tree-inline.cc | 14 +++++++++++--- 6 files changed, 45 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/enum-13_a.C create mode 100644 gcc/testsuite/g++.dg/modules/enum-13_b.C diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 66ef0bcaa94..cef547f8e0a 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -2522,13 +2522,12 @@ public: hash *chain; /* Original table. */ depset *current; /* Current depset being depended. */ unsigned section; /* When writing out, the section. */ - bool sneakoscope; /* Detecting dark magic (of a voldemort). */ bool reached_unreached; /* We reached an unreached entity. */ public: hash (size_t size, hash *c = NULL) : parent (size), chain (c), current (NULL), section (0), - sneakoscope (false), reached_unreached (false) + reached_unreached (false) { worklist.create (size); } @@ -8786,7 +8785,7 @@ trees_out::decl_node (tree decl, walk_kind ref) dep = dep_hash->find_dependency (decl); else if (TREE_CODE (ctx) != FUNCTION_DECL || TREE_CODE (decl) == TEMPLATE_DECL - || (dep_hash->sneakoscope && DECL_IMPLICIT_TYPEDEF_P (decl)) + || DECL_IMPLICIT_TYPEDEF_P (decl) || (DECL_LANG_SPECIFIC (decl) && DECL_MODULE_IMPORT_P (decl))) { @@ -13540,14 +13539,7 @@ depset::hash::find_dependencies (module_state *module) add_namespace_context (item, ns); } - // FIXME: Perhaps p1815 makes this redundant? Or at - // least simplifies it. Voldemort types are only - // ever emissable when containing (inline) function - // definition is emitted? - /* Turn the Sneakoscope on when depending the decl. */ - sneakoscope = true; walker.decl_value (decl, current); - sneakoscope = false; if (current->has_defn ()) walker.write_definition (decl); } diff --git a/gcc/testsuite/g++.dg/modules/enum-13_a.C b/gcc/testsuite/g++.dg/modules/enum-13_a.C new file mode 100644 index 00000000000..1d0c86d939a --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/enum-13_a.C @@ -0,0 +1,23 @@ +// PR c++/104919 +// PR c++/106009 +// { dg-additional-options -fmodules-ts } +// { dg-module-cmi Enum13 } + +export module Enum13; + +export +constexpr int f() { + enum E { e = 42 }; + return e; +} + +template<class T> +constexpr int ft(T) { + enum E { e = 43 }; + return e; +} + +export +constexpr int g() { + return ft(0); +} diff --git a/gcc/testsuite/g++.dg/modules/enum-13_b.C b/gcc/testsuite/g++.dg/modules/enum-13_b.C new file mode 100644 index 00000000000..16d4a7c8ac5 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/enum-13_b.C @@ -0,0 +1,8 @@ +// PR c++/104919 +// PR c++/106009 +// { dg-additional-options -fmodules-ts } + +import Enum13; + +static_assert(f() == 42); +static_assert(g() == 43); diff --git a/gcc/testsuite/g++.dg/modules/tdef-7.h b/gcc/testsuite/g++.dg/modules/tdef-7.h index 5bc21e176cb..6125f0460e2 100644 --- a/gcc/testsuite/g++.dg/modules/tdef-7.h +++ b/gcc/testsuite/g++.dg/modules/tdef-7.h @@ -1,7 +1,5 @@ constexpr void duration_cast () { - // the constexpr's body's clone merely duplicates the TYPE_DECL, it - // doesn't create a kosher typedef typedef int __to_rep; } diff --git a/gcc/testsuite/g++.dg/modules/tdef-7_b.C b/gcc/testsuite/g++.dg/modules/tdef-7_b.C index c526ca8dd4f..ea76458715b 100644 --- a/gcc/testsuite/g++.dg/modules/tdef-7_b.C +++ b/gcc/testsuite/g++.dg/modules/tdef-7_b.C @@ -5,5 +5,5 @@ import "tdef-7_a.H"; // { dg-final { scan-lang-dump-times {merge key \(matched\) function_decl:'::duration_cast} 1 module } } // { dg-final { scan-lang-dump-not {merge key \(new\)} module } } -// { dg-final { scan-lang-dump-times {merge key \(unique\) type_decl:'#null#'} 2 module } } +// { dg-final { scan-lang-dump-times {merge key \(unique\) type_decl:'#null#'} 1 module } } // { dg-final { scan-lang-dump-times {Cloned:-[0-9]* typedef integer_type:'::duration_cast::__to_rep'} 1 module } } diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc index 75c10eb7dfc..b35760ae9f0 100644 --- a/gcc/tree-inline.cc +++ b/gcc/tree-inline.cc @@ -370,7 +370,7 @@ remap_decl (tree decl, copy_body_data *id) need this decl for TYPE_STUB_DECL. */ insert_decl_map (id, decl, t); - if (!DECL_P (t)) + if (!DECL_P (t) || t == decl) return t; /* Remap types, if necessary. */ @@ -765,7 +765,7 @@ remap_decls (tree decls, vec<tree, va_gc> **nonlocalized_list, TREE_CHAIN. If we remapped this variable to the return slot, it's already declared somewhere else, so don't declare it here. */ - if (new_var == id->retvar) + if (new_var == old_var || new_var == id->retvar) ; else if (!new_var) { @@ -6610,7 +6610,15 @@ copy_fn (tree fn, tree& parms, tree& result) id.src_cfun = DECL_STRUCT_FUNCTION (fn); id.decl_map = &decl_map; - id.copy_decl = copy_decl_no_change; + id.copy_decl = [] (tree decl, copy_body_data *id) + { + if (TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == CONST_DECL) + /* Don't make copies of local types or enumerators, the C++ + constexpr evaluator doesn't need them and they cause problems + with modules. */ + return decl; + return copy_decl_no_change (decl, id); + }; id.transform_call_graph_edges = CB_CGE_DUPLICATE; id.transform_new_cfg = false; id.transform_return_to_modify = false;
On 3/1/24 11:45, Patrick Palka wrote: > On Fri, 1 Mar 2024, Jason Merrill wrote: > >>> This was nearly enough to make things work, except we now ran into >>> issues with the local TYPE/CONST_DECL copies when streaming the >>> constexpr version of a function body. It occurred to me that we don't >>> need to make copies of local types when copying a constexpr function >>> body; only VAR_DECLs etc need to be copied for sake of recursive >>> constexpr calls. So this patch adjusts copy_fn accordingly. >> >> Maybe adjust can_be_nonlocal instead? It seems unnecessary in general >> to remap types and enumerators for inlining. > > Unfortunately this approached caused a boostrap failure with Ada: > > raised STORAGE_ERROR : stack overflow or erroneous memory access > > The patch was > > --- a/gcc/tree-inline.cc > +++ b/gcc/tree-inline.cc > @@ -725,6 +725,9 @@ can_be_nonlocal (tree decl, copy_body_data *id) > if (TREE_CODE (decl) == FUNCTION_DECL) > return true; > > + if (TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == CONST_DECL) > + return true; Hmm, maybe a problem with a local type whose size depends on a local variable, so this would need to exclude that case. I suppose an enumerator could also have a value of sizeof(local-var), even in C++. Jason
On 3/1/24 12:08, Patrick Palka wrote: > On Fri, 1 Mar 2024, Patrick Palka wrote: > >> On Fri, 1 Mar 2024, Jason Merrill wrote: >> >>> On 3/1/24 10:00, Patrick Palka wrote: >>>> On Fri, 1 Mar 2024, Jason Merrill wrote: >>>> >>>>> On 2/29/24 15:56, Patrick Palka wrote: >>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look >>>>>> OK for trunk? >>>>>> >>>>>> -- >8 -- >>>>>> >>>>>> For local enums defined in a non-template function or a function >>>>>> template >>>>>> instantiation it seems we neglect to make the function depend on the >>>>>> enum >>>>>> definition, which ultimately causes streaming to fail due to the enum >>>>>> definition not being streamed before uses of its enumerators are >>>>>> streamed, >>>>>> as far as I can tell. >>>>> >>>>> I would think that the function doesn't need to depend on the local enum >>>>> in >>>>> order for the local enum to be streamed before the use of the enumerator, >>>>> which comes after the definition of the enum in the function body? >>>>> >>>>> Why isn't streaming the body of the function outputting the enum >>>>> definition >>>>> before the use of the enumerator? >>>> >>>> IIUC (based on observing the behavior for local classes) streaming the >>>> definition of a local class/enum as part of the function definition is >>>> what we want to avoid; we want to treat a local type definition as a >>>> logically separate definition and stream it separately (similar >>>> to class defns vs member defns I guess). And by not registering a >>>> dependency >>>> between the function and the local enum, we end up never streaming out >>>> the local enum definition separately and instead stream it out as part >>>> of the function definition (accidentally) which we then can't stream in >>>> properly. >>>> >>>> Perhaps the motivation for treating local type definitions as logically >>>> separate from the function definition is because they can leak out of a >>>> function with a deduced return type: >>>> >>>> auto f() { >>>> struct A { }; >>>> return A(); >>>> } >>>> >>>> using type = decltype(f()); // refers directly to f()::A >>> >>> Yes, I believe that's what modules.cc refers to as a "voldemort". >>> >>> But for non-voldemort local types, the declaration of the function doesn't >>> depend on them, only the definition. Why doesn't streaming them in the >>> definition work properly? >> >> I should note that for a templated local type we already always add a >> dependency between the function template _pattern_ and the local type >> _pattern_ and therefore always stream the local type pattern separately >> (even if its not actually a voldemort), thanks to the TREE_CODE (decl) == TEMPLATE_DECL >> case guarding the add_dependency call (inside a template pattern we >> see the TEMPLATE_DECL of the local TYPE_DECL). The dependency is >> missing only when the function is a non-template or non-template-pattern. >> My patch makes us consistently add the dependency and in turn consistently stream the definitions separately. >> >> (For a local _class_, in the non-template and non-template-pattern case >> we currently add a dependency between the function and the >> injected-class-name of the class as opposed to the class itself, which >> seems quite accidental but suffices. And that's why only local enums >> are problematic currently. After my patch we instead add a dependency >> to the local class itself.) >> >> Part of the puzzle of why we don't/can't stream them as part of the >> function definition is because we don't mark the enumerators for >> by-value walking when marking the function definition. So when >> streaming out the enumerator definition we stream out _references_ >> to the enumerators (tt_const_decl tags) instead of the actual >> definitions which breaks stream-in. >> >> The best place to mark local types for by-value walking would be >> in trees_out::mark_function_def which is suspiciously empty! I >> experimented with (never mind that it only marks the outermost block's >> types): >> >> @@ -11713,8 +11713,12 @@ trees_out::write_function_def (tree decl) >> } >> >> void >> -trees_out::mark_function_def (tree) >> +trees_out::mark_function_def (tree decl) >> { >> + tree initial = DECL_INITIAL (decl); >> + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) >> + if (DECL_IMPLICIT_TYPEDEF_P (var)) >> + mark_declaration (var, true); >> } >> >> Which actually fixes the non-template PR104919 testcase, but it >> breaks streaming of templated local types wherein we run into >> the sanity check: >> >> @@ -7677,16 +7677,6 @@ trees_out::decl_value (tree decl, depset *dep) >> >> merge_kind mk = get_merge_kind (decl, dep); >> >> ! if (CHECKING_P) >> ! { >> ! /* Never start in the middle of a template. */ >> ! int use_tpl = -1; >> ! if (tree ti = node_template_info (decl, use_tpl)) >> ! gcc_checking_assert (TREE_CODE (TI_TEMPLATE (ti)) == OVERLOAD >> ! || TREE_CODE (TI_TEMPLATE (ti)) == FIELD_DECL >> ! || (DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) >> ! != decl)); >> ! } >> >> if (streaming_p ()) >> { >> >> If we try to work around this sanity check by only marking local types >> when inside a non-template and non-template-pattern (i.e. inside an >> instantiation): >> >> @@ -11713,8 +11713,16 @@ trees_out::write_function_def (tree decl) >> } >> >> void >> -trees_out::mark_function_def (tree) >> +trees_out::mark_function_def (tree decl) >> { >> + tree initial = DECL_INITIAL (decl); >> + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) >> + if (DECL_IMPLICIT_TYPEDEF_P (var)) >> + { >> + tree ti = get_template_info (var); >> + if (!ti || DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) != var) >> + mark_declaration (var, true); >> + } >> } >> >> Then we trip over this other sanity check (when streaming a local >> TYPE_DECL from a function template instantiation): >> >> gcc/cp/module.cc >> @@ -7859,16 +7859,6 @@ trees_out::decl_value (tree decl, depset *dep) >> tree_node (get_constraints (decl)); >> } >> >> ! if (streaming_p ()) >> ! { >> ! /* Do not stray outside this section. */ >> ! gcc_checking_assert (!dep || dep->section == dep_hash->section); >> ! >> ! /* Write the entity index, so we can insert it as soon as we >> ! know this is new. */ >> ! install_entity (decl, dep); >> ! } >> ! >> if (DECL_LANG_SPECIFIC (inner) >> && DECL_MODULE_KEYED_DECLS_P (inner) >> && !is_key_order ()) >> >> At this point I gave up on this approach. It seems simpler to just >> consistently add the dependencies. Fair enough. > Ah, it just occurred to me we might as well remove the sneakoscope > flag altogether, since the code it affects is only used for dependency > analysis (!streaming_p ()) which happens from find_dependencies. This > seems like a nice simplification and passes the testsuite and other > smoke tests. Makes sense. > @@ -6610,7 +6610,15 @@ copy_fn (tree fn, tree& parms, tree& result) > id.src_cfun = DECL_STRUCT_FUNCTION (fn); > id.decl_map = &decl_map; > > - id.copy_decl = copy_decl_no_change; > + id.copy_decl = [] (tree decl, copy_body_data *id) > + { > + if (TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == CONST_DECL) > + /* Don't make copies of local types or enumerators, the C++ > + constexpr evaluator doesn't need them and they cause problems > + with modules. */ As mentioned in my other reply, I'm not sure this is safe for e.g. an enumerator with the value sizeof(local-var). Why are we streaming constexpr function copies, anyway? Can we not? Jason
On Fri, 1 Mar 2024, Jason Merrill wrote: > On 3/1/24 12:08, Patrick Palka wrote: > > On Fri, 1 Mar 2024, Patrick Palka wrote: > > > > > On Fri, 1 Mar 2024, Jason Merrill wrote: > > > > > > > On 3/1/24 10:00, Patrick Palka wrote: > > > > > On Fri, 1 Mar 2024, Jason Merrill wrote: > > > > > > > > > > > On 2/29/24 15:56, Patrick Palka wrote: > > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > > > > > > > OK for trunk? > > > > > > > > > > > > > > -- >8 -- > > > > > > > > > > > > > > For local enums defined in a non-template function or a function > > > > > > > template > > > > > > > instantiation it seems we neglect to make the function depend on > > > > > > > the > > > > > > > enum > > > > > > > definition, which ultimately causes streaming to fail due to the > > > > > > > enum > > > > > > > definition not being streamed before uses of its enumerators are > > > > > > > streamed, > > > > > > > as far as I can tell. > > > > > > > > > > > > I would think that the function doesn't need to depend on the local > > > > > > enum > > > > > > in > > > > > > order for the local enum to be streamed before the use of the > > > > > > enumerator, > > > > > > which comes after the definition of the enum in the function body? > > > > > > > > > > > > Why isn't streaming the body of the function outputting the enum > > > > > > definition > > > > > > before the use of the enumerator? > > > > > > > > > > IIUC (based on observing the behavior for local classes) streaming the > > > > > definition of a local class/enum as part of the function definition is > > > > > what we want to avoid; we want to treat a local type definition as a > > > > > logically separate definition and stream it separately (similar > > > > > to class defns vs member defns I guess). And by not registering a > > > > > dependency > > > > > between the function and the local enum, we end up never streaming out > > > > > the local enum definition separately and instead stream it out as part > > > > > of the function definition (accidentally) which we then can't stream > > > > > in > > > > > properly. > > > > > > > > > > Perhaps the motivation for treating local type definitions as > > > > > logically > > > > > separate from the function definition is because they can leak out of > > > > > a > > > > > function with a deduced return type: > > > > > > > > > > auto f() { > > > > > struct A { }; > > > > > return A(); > > > > > } > > > > > > > > > > using type = decltype(f()); // refers directly to f()::A > > > > > > > > Yes, I believe that's what modules.cc refers to as a "voldemort". > > > > > > > > But for non-voldemort local types, the declaration of the function > > > > doesn't > > > > depend on them, only the definition. Why doesn't streaming them in the > > > > definition work properly? > > > > > > I should note that for a templated local type we already always add a > > > dependency between the function template _pattern_ and the local type > > > _pattern_ and therefore always stream the local type pattern separately > > > (even if its not actually a voldemort), thanks to the TREE_CODE (decl) == > > > TEMPLATE_DECL > > > case guarding the add_dependency call (inside a template pattern we > > > see the TEMPLATE_DECL of the local TYPE_DECL). The dependency is > > > missing only when the function is a non-template or non-template-pattern. > > > My patch makes us consistently add the dependency and in turn consistently > > > stream the definitions separately. > > > > > > (For a local _class_, in the non-template and non-template-pattern case > > > we currently add a dependency between the function and the > > > injected-class-name of the class as opposed to the class itself, which > > > seems quite accidental but suffices. And that's why only local enums > > > are problematic currently. After my patch we instead add a dependency > > > to the local class itself.) > > > > > > Part of the puzzle of why we don't/can't stream them as part of the > > > function definition is because we don't mark the enumerators for > > > by-value walking when marking the function definition. So when > > > streaming out the enumerator definition we stream out _references_ > > > to the enumerators (tt_const_decl tags) instead of the actual > > > definitions which breaks stream-in. > > > > > > The best place to mark local types for by-value walking would be > > > in trees_out::mark_function_def which is suspiciously empty! I > > > experimented with (never mind that it only marks the outermost block's > > > types): > > > > > > @@ -11713,8 +11713,12 @@ trees_out::write_function_def (tree decl) > > > } > > > void > > > -trees_out::mark_function_def (tree) > > > +trees_out::mark_function_def (tree decl) > > > { > > > + tree initial = DECL_INITIAL (decl); > > > + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) > > > + if (DECL_IMPLICIT_TYPEDEF_P (var)) > > > + mark_declaration (var, true); > > > } > > > > > > Which actually fixes the non-template PR104919 testcase, but it > > > breaks streaming of templated local types wherein we run into > > > the sanity check: > > > > > > @@ -7677,16 +7677,6 @@ trees_out::decl_value (tree decl, depset *dep) > > > merge_kind mk = get_merge_kind (decl, dep); > > > ! if (CHECKING_P) > > > ! { > > > ! /* Never start in the middle of a template. */ > > > ! int use_tpl = -1; > > > ! if (tree ti = node_template_info (decl, use_tpl)) > > > ! gcc_checking_assert (TREE_CODE (TI_TEMPLATE (ti)) == OVERLOAD > > > ! || TREE_CODE (TI_TEMPLATE (ti)) == FIELD_DECL > > > ! || (DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) > > > ! != decl)); > > > ! } > > > if (streaming_p ()) > > > { > > > > > > If we try to work around this sanity check by only marking local types > > > when inside a non-template and non-template-pattern (i.e. inside an > > > instantiation): > > > > > > @@ -11713,8 +11713,16 @@ trees_out::write_function_def (tree decl) > > > } > > > void > > > -trees_out::mark_function_def (tree) > > > +trees_out::mark_function_def (tree decl) > > > { > > > + tree initial = DECL_INITIAL (decl); > > > + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) > > > + if (DECL_IMPLICIT_TYPEDEF_P (var)) > > > + { > > > + tree ti = get_template_info (var); > > > + if (!ti || DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) != var) > > > + mark_declaration (var, true); > > > + } > > > } > > > > > > Then we trip over this other sanity check (when streaming a local > > > TYPE_DECL from a function template instantiation): > > > > > > gcc/cp/module.cc > > > @@ -7859,16 +7859,6 @@ trees_out::decl_value (tree decl, depset *dep) > > > tree_node (get_constraints (decl)); > > > } > > > ! if (streaming_p ()) > > > ! { > > > ! /* Do not stray outside this section. */ > > > ! gcc_checking_assert (!dep || dep->section == dep_hash->section); > > > ! > > > ! /* Write the entity index, so we can insert it as soon as we > > > ! know this is new. */ > > > ! install_entity (decl, dep); > > > ! } > > > ! > > > if (DECL_LANG_SPECIFIC (inner) > > > && DECL_MODULE_KEYED_DECLS_P (inner) > > > && !is_key_order ()) > > > > > > At this point I gave up on this approach. It seems simpler to just > > > consistently add the dependencies. > > Fair enough. > > > Ah, it just occurred to me we might as well remove the sneakoscope > > flag altogether, since the code it affects is only used for dependency > > analysis (!streaming_p ()) which happens from find_dependencies. This > > seems like a nice simplification and passes the testsuite and other > > smoke tests. > > Makes sense. > > > @@ -6610,7 +6610,15 @@ copy_fn (tree fn, tree& parms, tree& result) > > id.src_cfun = DECL_STRUCT_FUNCTION (fn); > > id.decl_map = &decl_map; > > - id.copy_decl = copy_decl_no_change; > > + id.copy_decl = [] (tree decl, copy_body_data *id) > > + { > > + if (TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == CONST_DECL) > > + /* Don't make copies of local types or enumerators, the C++ > > + constexpr evaluator doesn't need them and they cause problems > > + with modules. */ > > As mentioned in my other reply, I'm not sure this is safe for e.g. an > enumerator with the value sizeof(local-var). Wouldn't such a value get reduced to a constant? If the local variable has a variably modified type then we reject the enumerator as non-constant: void f(int n) { int arr[n]; enum E { e = sizeof(arr); }; // error: 'n' is not a constant expression } > > Why are we streaming constexpr function copies, anyway? Can we not? Ah sorry, what I meant was that we stream the pre-gimplified version of a constexpr function created by maybe_save_constexpr_fundef (alongside the canonical gimplified version), not the various copies thereof created by get_fundef_copy. I don't think we can avoid not streaming it or else we would not need to do maybe_save_constexpr_fundef in the first place? > > Jason > >
On 3/1/24 13:28, Patrick Palka wrote: > On Fri, 1 Mar 2024, Jason Merrill wrote: > >> On 3/1/24 12:08, Patrick Palka wrote: >>> On Fri, 1 Mar 2024, Patrick Palka wrote: >>> >>>> On Fri, 1 Mar 2024, Jason Merrill wrote: >>>> >>>>> On 3/1/24 10:00, Patrick Palka wrote: >>>>>> On Fri, 1 Mar 2024, Jason Merrill wrote: >>>>>> >>>>>>> On 2/29/24 15:56, Patrick Palka wrote: >>>>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look >>>>>>>> OK for trunk? >>>>>>>> >>>>>>>> -- >8 -- >>>>>>>> >>>>>>>> For local enums defined in a non-template function or a function >>>>>>>> template >>>>>>>> instantiation it seems we neglect to make the function depend on >>>>>>>> the >>>>>>>> enum >>>>>>>> definition, which ultimately causes streaming to fail due to the >>>>>>>> enum >>>>>>>> definition not being streamed before uses of its enumerators are >>>>>>>> streamed, >>>>>>>> as far as I can tell. >>>>>>> >>>>>>> I would think that the function doesn't need to depend on the local >>>>>>> enum >>>>>>> in >>>>>>> order for the local enum to be streamed before the use of the >>>>>>> enumerator, >>>>>>> which comes after the definition of the enum in the function body? >>>>>>> >>>>>>> Why isn't streaming the body of the function outputting the enum >>>>>>> definition >>>>>>> before the use of the enumerator? >>>>>> >>>>>> IIUC (based on observing the behavior for local classes) streaming the >>>>>> definition of a local class/enum as part of the function definition is >>>>>> what we want to avoid; we want to treat a local type definition as a >>>>>> logically separate definition and stream it separately (similar >>>>>> to class defns vs member defns I guess). And by not registering a >>>>>> dependency >>>>>> between the function and the local enum, we end up never streaming out >>>>>> the local enum definition separately and instead stream it out as part >>>>>> of the function definition (accidentally) which we then can't stream >>>>>> in >>>>>> properly. >>>>>> >>>>>> Perhaps the motivation for treating local type definitions as >>>>>> logically >>>>>> separate from the function definition is because they can leak out of >>>>>> a >>>>>> function with a deduced return type: >>>>>> >>>>>> auto f() { >>>>>> struct A { }; >>>>>> return A(); >>>>>> } >>>>>> >>>>>> using type = decltype(f()); // refers directly to f()::A >>>>> >>>>> Yes, I believe that's what modules.cc refers to as a "voldemort". >>>>> >>>>> But for non-voldemort local types, the declaration of the function >>>>> doesn't >>>>> depend on them, only the definition. Why doesn't streaming them in the >>>>> definition work properly? >>>> >>>> I should note that for a templated local type we already always add a >>>> dependency between the function template _pattern_ and the local type >>>> _pattern_ and therefore always stream the local type pattern separately >>>> (even if its not actually a voldemort), thanks to the TREE_CODE (decl) == >>>> TEMPLATE_DECL >>>> case guarding the add_dependency call (inside a template pattern we >>>> see the TEMPLATE_DECL of the local TYPE_DECL). The dependency is >>>> missing only when the function is a non-template or non-template-pattern. >>>> My patch makes us consistently add the dependency and in turn consistently >>>> stream the definitions separately. >>>> >>>> (For a local _class_, in the non-template and non-template-pattern case >>>> we currently add a dependency between the function and the >>>> injected-class-name of the class as opposed to the class itself, which >>>> seems quite accidental but suffices. And that's why only local enums >>>> are problematic currently. After my patch we instead add a dependency >>>> to the local class itself.) >>>> >>>> Part of the puzzle of why we don't/can't stream them as part of the >>>> function definition is because we don't mark the enumerators for >>>> by-value walking when marking the function definition. So when >>>> streaming out the enumerator definition we stream out _references_ >>>> to the enumerators (tt_const_decl tags) instead of the actual >>>> definitions which breaks stream-in. >>>> >>>> The best place to mark local types for by-value walking would be >>>> in trees_out::mark_function_def which is suspiciously empty! I >>>> experimented with (never mind that it only marks the outermost block's >>>> types): >>>> >>>> @@ -11713,8 +11713,12 @@ trees_out::write_function_def (tree decl) >>>> } >>>> void >>>> -trees_out::mark_function_def (tree) >>>> +trees_out::mark_function_def (tree decl) >>>> { >>>> + tree initial = DECL_INITIAL (decl); >>>> + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) >>>> + if (DECL_IMPLICIT_TYPEDEF_P (var)) >>>> + mark_declaration (var, true); >>>> } >>>> >>>> Which actually fixes the non-template PR104919 testcase, but it >>>> breaks streaming of templated local types wherein we run into >>>> the sanity check: >>>> >>>> @@ -7677,16 +7677,6 @@ trees_out::decl_value (tree decl, depset *dep) >>>> merge_kind mk = get_merge_kind (decl, dep); >>>> ! if (CHECKING_P) >>>> ! { >>>> ! /* Never start in the middle of a template. */ >>>> ! int use_tpl = -1; >>>> ! if (tree ti = node_template_info (decl, use_tpl)) >>>> ! gcc_checking_assert (TREE_CODE (TI_TEMPLATE (ti)) == OVERLOAD >>>> ! || TREE_CODE (TI_TEMPLATE (ti)) == FIELD_DECL >>>> ! || (DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) >>>> ! != decl)); >>>> ! } >>>> if (streaming_p ()) >>>> { >>>> >>>> If we try to work around this sanity check by only marking local types >>>> when inside a non-template and non-template-pattern (i.e. inside an >>>> instantiation): >>>> >>>> @@ -11713,8 +11713,16 @@ trees_out::write_function_def (tree decl) >>>> } >>>> void >>>> -trees_out::mark_function_def (tree) >>>> +trees_out::mark_function_def (tree decl) >>>> { >>>> + tree initial = DECL_INITIAL (decl); >>>> + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) >>>> + if (DECL_IMPLICIT_TYPEDEF_P (var)) >>>> + { >>>> + tree ti = get_template_info (var); >>>> + if (!ti || DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) != var) >>>> + mark_declaration (var, true); >>>> + } >>>> } >>>> >>>> Then we trip over this other sanity check (when streaming a local >>>> TYPE_DECL from a function template instantiation): >>>> >>>> gcc/cp/module.cc >>>> @@ -7859,16 +7859,6 @@ trees_out::decl_value (tree decl, depset *dep) >>>> tree_node (get_constraints (decl)); >>>> } >>>> ! if (streaming_p ()) >>>> ! { >>>> ! /* Do not stray outside this section. */ >>>> ! gcc_checking_assert (!dep || dep->section == dep_hash->section); >>>> ! >>>> ! /* Write the entity index, so we can insert it as soon as we >>>> ! know this is new. */ >>>> ! install_entity (decl, dep); >>>> ! } >>>> ! >>>> if (DECL_LANG_SPECIFIC (inner) >>>> && DECL_MODULE_KEYED_DECLS_P (inner) >>>> && !is_key_order ()) >>>> >>>> At this point I gave up on this approach. It seems simpler to just >>>> consistently add the dependencies. >> >> Fair enough. >> >>> Ah, it just occurred to me we might as well remove the sneakoscope >>> flag altogether, since the code it affects is only used for dependency >>> analysis (!streaming_p ()) which happens from find_dependencies. This >>> seems like a nice simplification and passes the testsuite and other >>> smoke tests. >> >> Makes sense. >> >>> @@ -6610,7 +6610,15 @@ copy_fn (tree fn, tree& parms, tree& result) >>> id.src_cfun = DECL_STRUCT_FUNCTION (fn); >>> id.decl_map = &decl_map; >>> - id.copy_decl = copy_decl_no_change; >>> + id.copy_decl = [] (tree decl, copy_body_data *id) >>> + { >>> + if (TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == CONST_DECL) >>> + /* Don't make copies of local types or enumerators, the C++ >>> + constexpr evaluator doesn't need them and they cause problems >>> + with modules. */ >> >> As mentioned in my other reply, I'm not sure this is safe for e.g. an >> enumerator with the value sizeof(local-var). > > Wouldn't such a value get reduced to a constant? If the local variable > has a variably modified type then we reject the enumerator as non-constant: > > void f(int n) { > int arr[n]; > enum E { e = sizeof(arr); }; // error: 'n' is not a constant expression > } Good point. So in the can_be_nonlocal patch, can we return false for variably_modified_type_p types to avoid Ada problems? >> Why are we streaming constexpr function copies, anyway? Can we not? > > Ah sorry, what I meant was that we stream the pre-gimplified version of > a constexpr function created by maybe_save_constexpr_fundef (alongside > the canonical gimplified version), not the various copies thereof > created by get_fundef_copy. I don't think we can avoid not streaming > it or else we would not need to do maybe_save_constexpr_fundef in > the first place? Ah, right. Jason
On Fri, 1 Mar 2024, Jason Merrill wrote: > On 3/1/24 13:28, Patrick Palka wrote: > > On Fri, 1 Mar 2024, Jason Merrill wrote: > > > > > On 3/1/24 12:08, Patrick Palka wrote: > > > > On Fri, 1 Mar 2024, Patrick Palka wrote: > > > > > > > > > On Fri, 1 Mar 2024, Jason Merrill wrote: > > > > > > > > > > > On 3/1/24 10:00, Patrick Palka wrote: > > > > > > > On Fri, 1 Mar 2024, Jason Merrill wrote: > > > > > > > > > > > > > > > On 2/29/24 15:56, Patrick Palka wrote: > > > > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this > > > > > > > > > look > > > > > > > > > OK for trunk? > > > > > > > > > > > > > > > > > > -- >8 -- > > > > > > > > > > > > > > > > > > For local enums defined in a non-template function or a > > > > > > > > > function > > > > > > > > > template > > > > > > > > > instantiation it seems we neglect to make the function depend > > > > > > > > > on > > > > > > > > > the > > > > > > > > > enum > > > > > > > > > definition, which ultimately causes streaming to fail due to > > > > > > > > > the > > > > > > > > > enum > > > > > > > > > definition not being streamed before uses of its enumerators > > > > > > > > > are > > > > > > > > > streamed, > > > > > > > > > as far as I can tell. > > > > > > > > > > > > > > > > I would think that the function doesn't need to depend on the > > > > > > > > local > > > > > > > > enum > > > > > > > > in > > > > > > > > order for the local enum to be streamed before the use of the > > > > > > > > enumerator, > > > > > > > > which comes after the definition of the enum in the function > > > > > > > > body? > > > > > > > > > > > > > > > > Why isn't streaming the body of the function outputting the enum > > > > > > > > definition > > > > > > > > before the use of the enumerator? > > > > > > > > > > > > > > IIUC (based on observing the behavior for local classes) streaming > > > > > > > the > > > > > > > definition of a local class/enum as part of the function > > > > > > > definition is > > > > > > > what we want to avoid; we want to treat a local type definition as > > > > > > > a > > > > > > > logically separate definition and stream it separately (similar > > > > > > > to class defns vs member defns I guess). And by not registering a > > > > > > > dependency > > > > > > > between the function and the local enum, we end up never streaming > > > > > > > out > > > > > > > the local enum definition separately and instead stream it out as > > > > > > > part > > > > > > > of the function definition (accidentally) which we then can't > > > > > > > stream > > > > > > > in > > > > > > > properly. > > > > > > > > > > > > > > Perhaps the motivation for treating local type definitions as > > > > > > > logically > > > > > > > separate from the function definition is because they can leak out > > > > > > > of > > > > > > > a > > > > > > > function with a deduced return type: > > > > > > > > > > > > > > auto f() { > > > > > > > struct A { }; > > > > > > > return A(); > > > > > > > } > > > > > > > > > > > > > > using type = decltype(f()); // refers directly to f()::A > > > > > > > > > > > > Yes, I believe that's what modules.cc refers to as a "voldemort". > > > > > > > > > > > > But for non-voldemort local types, the declaration of the function > > > > > > doesn't > > > > > > depend on them, only the definition. Why doesn't streaming them in > > > > > > the > > > > > > definition work properly? > > > > > > > > > > I should note that for a templated local type we already always add a > > > > > dependency between the function template _pattern_ and the local type > > > > > _pattern_ and therefore always stream the local type pattern > > > > > separately > > > > > (even if its not actually a voldemort), thanks to the TREE_CODE (decl) > > > > > == > > > > > TEMPLATE_DECL > > > > > case guarding the add_dependency call (inside a template pattern we > > > > > see the TEMPLATE_DECL of the local TYPE_DECL). The dependency is > > > > > missing only when the function is a non-template or > > > > > non-template-pattern. > > > > > My patch makes us consistently add the dependency and in turn > > > > > consistently > > > > > stream the definitions separately. > > > > > > > > > > (For a local _class_, in the non-template and non-template-pattern > > > > > case > > > > > we currently add a dependency between the function and the > > > > > injected-class-name of the class as opposed to the class itself, which > > > > > seems quite accidental but suffices. And that's why only local enums > > > > > are problematic currently. After my patch we instead add a dependency > > > > > to the local class itself.) > > > > > > > > > > Part of the puzzle of why we don't/can't stream them as part of the > > > > > function definition is because we don't mark the enumerators for > > > > > by-value walking when marking the function definition. So when > > > > > streaming out the enumerator definition we stream out _references_ > > > > > to the enumerators (tt_const_decl tags) instead of the actual > > > > > definitions which breaks stream-in. > > > > > > > > > > The best place to mark local types for by-value walking would be > > > > > in trees_out::mark_function_def which is suspiciously empty! I > > > > > experimented with (never mind that it only marks the outermost block's > > > > > types): > > > > > > > > > > @@ -11713,8 +11713,12 @@ trees_out::write_function_def (tree decl) > > > > > } > > > > > void > > > > > -trees_out::mark_function_def (tree) > > > > > +trees_out::mark_function_def (tree decl) > > > > > { > > > > > + tree initial = DECL_INITIAL (decl); > > > > > + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) > > > > > + if (DECL_IMPLICIT_TYPEDEF_P (var)) > > > > > + mark_declaration (var, true); > > > > > } > > > > > > > > > > Which actually fixes the non-template PR104919 testcase, but it > > > > > breaks streaming of templated local types wherein we run into > > > > > the sanity check: > > > > > > > > > > @@ -7677,16 +7677,6 @@ trees_out::decl_value (tree decl, depset *dep) > > > > > merge_kind mk = get_merge_kind (decl, dep); > > > > > ! if (CHECKING_P) > > > > > ! { > > > > > ! /* Never start in the middle of a template. */ > > > > > ! int use_tpl = -1; > > > > > ! if (tree ti = node_template_info (decl, use_tpl)) > > > > > ! gcc_checking_assert (TREE_CODE (TI_TEMPLATE (ti)) == OVERLOAD > > > > > ! || TREE_CODE (TI_TEMPLATE (ti)) == > > > > > FIELD_DECL > > > > > ! || (DECL_TEMPLATE_RESULT (TI_TEMPLATE > > > > > (ti)) > > > > > ! != decl)); > > > > > ! } > > > > > if (streaming_p ()) > > > > > { > > > > > > > > > > If we try to work around this sanity check by only marking local types > > > > > when inside a non-template and non-template-pattern (i.e. inside an > > > > > instantiation): > > > > > > > > > > @@ -11713,8 +11713,16 @@ trees_out::write_function_def (tree decl) > > > > > } > > > > > void > > > > > -trees_out::mark_function_def (tree) > > > > > +trees_out::mark_function_def (tree decl) > > > > > { > > > > > + tree initial = DECL_INITIAL (decl); > > > > > + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) > > > > > + if (DECL_IMPLICIT_TYPEDEF_P (var)) > > > > > + { > > > > > + tree ti = get_template_info (var); > > > > > + if (!ti || DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) != var) > > > > > + mark_declaration (var, true); > > > > > + } > > > > > } > > > > > > > > > > Then we trip over this other sanity check (when streaming a local > > > > > TYPE_DECL from a function template instantiation): > > > > > > > > > > gcc/cp/module.cc > > > > > @@ -7859,16 +7859,6 @@ trees_out::decl_value (tree decl, depset *dep) > > > > > tree_node (get_constraints (decl)); > > > > > } > > > > > ! if (streaming_p ()) > > > > > ! { > > > > > ! /* Do not stray outside this section. */ > > > > > ! gcc_checking_assert (!dep || dep->section == > > > > > dep_hash->section); > > > > > ! > > > > > ! /* Write the entity index, so we can insert it as soon as we > > > > > ! know this is new. */ > > > > > ! install_entity (decl, dep); > > > > > ! } > > > > > ! > > > > > if (DECL_LANG_SPECIFIC (inner) > > > > > && DECL_MODULE_KEYED_DECLS_P (inner) > > > > > && !is_key_order ()) > > > > > > > > > > At this point I gave up on this approach. It seems simpler to just > > > > > consistently add the dependencies. > > > > > > Fair enough. > > > > > > > Ah, it just occurred to me we might as well remove the sneakoscope > > > > flag altogether, since the code it affects is only used for dependency > > > > analysis (!streaming_p ()) which happens from find_dependencies. This > > > > seems like a nice simplification and passes the testsuite and other > > > > smoke tests. > > > > > > Makes sense. > > > > > > > @@ -6610,7 +6610,15 @@ copy_fn (tree fn, tree& parms, tree& result) > > > > id.src_cfun = DECL_STRUCT_FUNCTION (fn); > > > > id.decl_map = &decl_map; > > > > - id.copy_decl = copy_decl_no_change; > > > > + id.copy_decl = [] (tree decl, copy_body_data *id) > > > > + { > > > > + if (TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == > > > > CONST_DECL) > > > > + /* Don't make copies of local types or enumerators, the C++ > > > > + constexpr evaluator doesn't need them and they cause problems > > > > + with modules. */ > > > > > > As mentioned in my other reply, I'm not sure this is safe for e.g. an > > > enumerator with the value sizeof(local-var). > > > > Wouldn't such a value get reduced to a constant? If the local variable > > has a variably modified type then we reject the enumerator as non-constant: > > > > void f(int n) { > > int arr[n]; > > enum E { e = sizeof(arr); }; // error: 'n' is not a constant > > expression > > } > > Good point. So in the can_be_nonlocal patch, can we return false for > variably_modified_type_p types to avoid Ada problems? Unfortunately that doesn't help, bootstrap still fails the same way for Ada with --- a/gcc/tree-inline.cc +++ b/gcc/tree-inline.cc @@ -725,6 +725,10 @@ can_be_nonlocal (tree decl, copy_body_data *id) if (TREE_CODE (decl) == FUNCTION_DECL) return true; + if ((TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == CONST_DECL) + && !variably_modified_type_p (TREE_TYPE (decl), id->src_fn)) + return true; + /* Local static vars must be non-local or we get multiple declaration problems. */ if (VAR_P (decl) && !auto_var_in_fn_p (decl, id->src_fn)) > > > > Why are we streaming constexpr function copies, anyway? Can we not? > > > > Ah sorry, what I meant was that we stream the pre-gimplified version of > > a constexpr function created by maybe_save_constexpr_fundef (alongside > > the canonical gimplified version), not the various copies thereof > > created by get_fundef_copy. I don't think we can avoid not streaming > > it or else we would not need to do maybe_save_constexpr_fundef in > > the first place? > > Ah, right. > > Jason > >
On 3/1/24 14:34, Patrick Palka wrote: > On Fri, 1 Mar 2024, Jason Merrill wrote: > >> On 3/1/24 13:28, Patrick Palka wrote: >>> On Fri, 1 Mar 2024, Jason Merrill wrote: >>> >>>> On 3/1/24 12:08, Patrick Palka wrote: >>>>> On Fri, 1 Mar 2024, Patrick Palka wrote: >>>>> >>>>>> On Fri, 1 Mar 2024, Jason Merrill wrote: >>>>>> >>>>>>> On 3/1/24 10:00, Patrick Palka wrote: >>>>>>>> On Fri, 1 Mar 2024, Jason Merrill wrote: >>>>>>>> >>>>>>>>> On 2/29/24 15:56, Patrick Palka wrote: >>>>>>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this >>>>>>>>>> look >>>>>>>>>> OK for trunk? >>>>>>>>>> >>>>>>>>>> -- >8 -- >>>>>>>>>> >>>>>>>>>> For local enums defined in a non-template function or a >>>>>>>>>> function >>>>>>>>>> template >>>>>>>>>> instantiation it seems we neglect to make the function depend >>>>>>>>>> on >>>>>>>>>> the >>>>>>>>>> enum >>>>>>>>>> definition, which ultimately causes streaming to fail due to >>>>>>>>>> the >>>>>>>>>> enum >>>>>>>>>> definition not being streamed before uses of its enumerators >>>>>>>>>> are >>>>>>>>>> streamed, >>>>>>>>>> as far as I can tell. >>>>>>>>> >>>>>>>>> I would think that the function doesn't need to depend on the >>>>>>>>> local >>>>>>>>> enum >>>>>>>>> in >>>>>>>>> order for the local enum to be streamed before the use of the >>>>>>>>> enumerator, >>>>>>>>> which comes after the definition of the enum in the function >>>>>>>>> body? >>>>>>>>> >>>>>>>>> Why isn't streaming the body of the function outputting the enum >>>>>>>>> definition >>>>>>>>> before the use of the enumerator? >>>>>>>> >>>>>>>> IIUC (based on observing the behavior for local classes) streaming >>>>>>>> the >>>>>>>> definition of a local class/enum as part of the function >>>>>>>> definition is >>>>>>>> what we want to avoid; we want to treat a local type definition as >>>>>>>> a >>>>>>>> logically separate definition and stream it separately (similar >>>>>>>> to class defns vs member defns I guess). And by not registering a >>>>>>>> dependency >>>>>>>> between the function and the local enum, we end up never streaming >>>>>>>> out >>>>>>>> the local enum definition separately and instead stream it out as >>>>>>>> part >>>>>>>> of the function definition (accidentally) which we then can't >>>>>>>> stream >>>>>>>> in >>>>>>>> properly. >>>>>>>> >>>>>>>> Perhaps the motivation for treating local type definitions as >>>>>>>> logically >>>>>>>> separate from the function definition is because they can leak out >>>>>>>> of >>>>>>>> a >>>>>>>> function with a deduced return type: >>>>>>>> >>>>>>>> auto f() { >>>>>>>> struct A { }; >>>>>>>> return A(); >>>>>>>> } >>>>>>>> >>>>>>>> using type = decltype(f()); // refers directly to f()::A >>>>>>> >>>>>>> Yes, I believe that's what modules.cc refers to as a "voldemort". >>>>>>> >>>>>>> But for non-voldemort local types, the declaration of the function >>>>>>> doesn't >>>>>>> depend on them, only the definition. Why doesn't streaming them in >>>>>>> the >>>>>>> definition work properly? >>>>>> >>>>>> I should note that for a templated local type we already always add a >>>>>> dependency between the function template _pattern_ and the local type >>>>>> _pattern_ and therefore always stream the local type pattern >>>>>> separately >>>>>> (even if its not actually a voldemort), thanks to the TREE_CODE (decl) >>>>>> == >>>>>> TEMPLATE_DECL >>>>>> case guarding the add_dependency call (inside a template pattern we >>>>>> see the TEMPLATE_DECL of the local TYPE_DECL). The dependency is >>>>>> missing only when the function is a non-template or >>>>>> non-template-pattern. >>>>>> My patch makes us consistently add the dependency and in turn >>>>>> consistently >>>>>> stream the definitions separately. >>>>>> >>>>>> (For a local _class_, in the non-template and non-template-pattern >>>>>> case >>>>>> we currently add a dependency between the function and the >>>>>> injected-class-name of the class as opposed to the class itself, which >>>>>> seems quite accidental but suffices. And that's why only local enums >>>>>> are problematic currently. After my patch we instead add a dependency >>>>>> to the local class itself.) >>>>>> >>>>>> Part of the puzzle of why we don't/can't stream them as part of the >>>>>> function definition is because we don't mark the enumerators for >>>>>> by-value walking when marking the function definition. So when >>>>>> streaming out the enumerator definition we stream out _references_ >>>>>> to the enumerators (tt_const_decl tags) instead of the actual >>>>>> definitions which breaks stream-in. >>>>>> >>>>>> The best place to mark local types for by-value walking would be >>>>>> in trees_out::mark_function_def which is suspiciously empty! I >>>>>> experimented with (never mind that it only marks the outermost block's >>>>>> types): >>>>>> >>>>>> @@ -11713,8 +11713,12 @@ trees_out::write_function_def (tree decl) >>>>>> } >>>>>> void >>>>>> -trees_out::mark_function_def (tree) >>>>>> +trees_out::mark_function_def (tree decl) >>>>>> { >>>>>> + tree initial = DECL_INITIAL (decl); >>>>>> + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) >>>>>> + if (DECL_IMPLICIT_TYPEDEF_P (var)) >>>>>> + mark_declaration (var, true); >>>>>> } >>>>>> >>>>>> Which actually fixes the non-template PR104919 testcase, but it >>>>>> breaks streaming of templated local types wherein we run into >>>>>> the sanity check: >>>>>> >>>>>> @@ -7677,16 +7677,6 @@ trees_out::decl_value (tree decl, depset *dep) >>>>>> merge_kind mk = get_merge_kind (decl, dep); >>>>>> ! if (CHECKING_P) >>>>>> ! { >>>>>> ! /* Never start in the middle of a template. */ >>>>>> ! int use_tpl = -1; >>>>>> ! if (tree ti = node_template_info (decl, use_tpl)) >>>>>> ! gcc_checking_assert (TREE_CODE (TI_TEMPLATE (ti)) == OVERLOAD >>>>>> ! || TREE_CODE (TI_TEMPLATE (ti)) == >>>>>> FIELD_DECL >>>>>> ! || (DECL_TEMPLATE_RESULT (TI_TEMPLATE >>>>>> (ti)) >>>>>> ! != decl)); >>>>>> ! } >>>>>> if (streaming_p ()) >>>>>> { >>>>>> >>>>>> If we try to work around this sanity check by only marking local types >>>>>> when inside a non-template and non-template-pattern (i.e. inside an >>>>>> instantiation): >>>>>> >>>>>> @@ -11713,8 +11713,16 @@ trees_out::write_function_def (tree decl) >>>>>> } >>>>>> void >>>>>> -trees_out::mark_function_def (tree) >>>>>> +trees_out::mark_function_def (tree decl) >>>>>> { >>>>>> + tree initial = DECL_INITIAL (decl); >>>>>> + for (tree var = BLOCK_VARS (initial); var; var = DECL_CHAIN (var)) >>>>>> + if (DECL_IMPLICIT_TYPEDEF_P (var)) >>>>>> + { >>>>>> + tree ti = get_template_info (var); >>>>>> + if (!ti || DECL_TEMPLATE_RESULT (TI_TEMPLATE (ti)) != var) >>>>>> + mark_declaration (var, true); >>>>>> + } >>>>>> } >>>>>> >>>>>> Then we trip over this other sanity check (when streaming a local >>>>>> TYPE_DECL from a function template instantiation): >>>>>> >>>>>> gcc/cp/module.cc >>>>>> @@ -7859,16 +7859,6 @@ trees_out::decl_value (tree decl, depset *dep) >>>>>> tree_node (get_constraints (decl)); >>>>>> } >>>>>> ! if (streaming_p ()) >>>>>> ! { >>>>>> ! /* Do not stray outside this section. */ >>>>>> ! gcc_checking_assert (!dep || dep->section == >>>>>> dep_hash->section); >>>>>> ! >>>>>> ! /* Write the entity index, so we can insert it as soon as we >>>>>> ! know this is new. */ >>>>>> ! install_entity (decl, dep); >>>>>> ! } >>>>>> ! >>>>>> if (DECL_LANG_SPECIFIC (inner) >>>>>> && DECL_MODULE_KEYED_DECLS_P (inner) >>>>>> && !is_key_order ()) >>>>>> >>>>>> At this point I gave up on this approach. It seems simpler to just >>>>>> consistently add the dependencies. >>>> >>>> Fair enough. >>>> >>>>> Ah, it just occurred to me we might as well remove the sneakoscope >>>>> flag altogether, since the code it affects is only used for dependency >>>>> analysis (!streaming_p ()) which happens from find_dependencies. This >>>>> seems like a nice simplification and passes the testsuite and other >>>>> smoke tests. >>>> >>>> Makes sense. >>>> >>>>> @@ -6610,7 +6610,15 @@ copy_fn (tree fn, tree& parms, tree& result) >>>>> id.src_cfun = DECL_STRUCT_FUNCTION (fn); >>>>> id.decl_map = &decl_map; >>>>> - id.copy_decl = copy_decl_no_change; >>>>> + id.copy_decl = [] (tree decl, copy_body_data *id) >>>>> + { >>>>> + if (TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == >>>>> CONST_DECL) >>>>> + /* Don't make copies of local types or enumerators, the C++ >>>>> + constexpr evaluator doesn't need them and they cause problems >>>>> + with modules. */ >>>> >>>> As mentioned in my other reply, I'm not sure this is safe for e.g. an >>>> enumerator with the value sizeof(local-var). >>> >>> Wouldn't such a value get reduced to a constant? If the local variable >>> has a variably modified type then we reject the enumerator as non-constant: >>> >>> void f(int n) { >>> int arr[n]; >>> enum E { e = sizeof(arr); }; // error: 'n' is not a constant >>> expression >>> } >> >> Good point. So in the can_be_nonlocal patch, can we return false for >> variably_modified_type_p types to avoid Ada problems? > > Unfortunately that doesn't help, bootstrap still fails the same way for > Ada with > > --- a/gcc/tree-inline.cc > +++ b/gcc/tree-inline.cc > @@ -725,6 +725,10 @@ can_be_nonlocal (tree decl, copy_body_data *id) > if (TREE_CODE (decl) == FUNCTION_DECL) > return true; > > + if ((TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == CONST_DECL) > + && !variably_modified_type_p (TREE_TYPE (decl), id->src_fn)) > + return true; Alas. Then your last patch is OK. Jason
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 66ef0bcaa94..29e57716297 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -13547,9 +13547,9 @@ depset::hash::find_dependencies (module_state *module) /* Turn the Sneakoscope on when depending the decl. */ sneakoscope = true; walker.decl_value (decl, current); - sneakoscope = false; if (current->has_defn ()) walker.write_definition (decl); + sneakoscope = false; } walker.end (); diff --git a/gcc/testsuite/g++.dg/modules/enum-13_a.C b/gcc/testsuite/g++.dg/modules/enum-13_a.C new file mode 100644 index 00000000000..2e570c6c4fb --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/enum-13_a.C @@ -0,0 +1,23 @@ +// PR c++/104919 +// PR c++/106009 +// { dg-additional-options -fmodules-ts } +// { dg-module-cmi Enum13 } + +export module Enum13; + +export +constexpr int f() { + enum E { e = 42 }; + return e; +} + +template<class T> +constexpr int ft(T) { + enum blah { e = 43 }; + return e; +} + +export +constexpr int g() { + return ft(0); +} diff --git a/gcc/testsuite/g++.dg/modules/enum-13_b.C b/gcc/testsuite/g++.dg/modules/enum-13_b.C new file mode 100644 index 00000000000..16d4a7c8ac5 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/enum-13_b.C @@ -0,0 +1,8 @@ +// PR c++/104919 +// PR c++/106009 +// { dg-additional-options -fmodules-ts } + +import Enum13; + +static_assert(f() == 42); +static_assert(g() == 43); diff --git a/gcc/testsuite/g++.dg/modules/tdef-7.h b/gcc/testsuite/g++.dg/modules/tdef-7.h index 5bc21e176cb..6125f0460e2 100644 --- a/gcc/testsuite/g++.dg/modules/tdef-7.h +++ b/gcc/testsuite/g++.dg/modules/tdef-7.h @@ -1,7 +1,5 @@ constexpr void duration_cast () { - // the constexpr's body's clone merely duplicates the TYPE_DECL, it - // doesn't create a kosher typedef typedef int __to_rep; } diff --git a/gcc/testsuite/g++.dg/modules/tdef-7_b.C b/gcc/testsuite/g++.dg/modules/tdef-7_b.C index c526ca8dd4f..ea76458715b 100644 --- a/gcc/testsuite/g++.dg/modules/tdef-7_b.C +++ b/gcc/testsuite/g++.dg/modules/tdef-7_b.C @@ -5,5 +5,5 @@ import "tdef-7_a.H"; // { dg-final { scan-lang-dump-times {merge key \(matched\) function_decl:'::duration_cast} 1 module } } // { dg-final { scan-lang-dump-not {merge key \(new\)} module } } -// { dg-final { scan-lang-dump-times {merge key \(unique\) type_decl:'#null#'} 2 module } } +// { dg-final { scan-lang-dump-times {merge key \(unique\) type_decl:'#null#'} 1 module } } // { dg-final { scan-lang-dump-times {Cloned:-[0-9]* typedef integer_type:'::duration_cast::__to_rep'} 1 module } } diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc index 75c10eb7dfc..b35760ae9f0 100644 --- a/gcc/tree-inline.cc +++ b/gcc/tree-inline.cc @@ -370,7 +370,7 @@ remap_decl (tree decl, copy_body_data *id) need this decl for TYPE_STUB_DECL. */ insert_decl_map (id, decl, t); - if (!DECL_P (t)) + if (!DECL_P (t) || t == decl) return t; /* Remap types, if necessary. */ @@ -765,7 +765,7 @@ remap_decls (tree decls, vec<tree, va_gc> **nonlocalized_list, TREE_CHAIN. If we remapped this variable to the return slot, it's already declared somewhere else, so don't declare it here. */ - if (new_var == id->retvar) + if (new_var == old_var || new_var == id->retvar) ; else if (!new_var) { @@ -6610,7 +6610,15 @@ copy_fn (tree fn, tree& parms, tree& result) id.src_cfun = DECL_STRUCT_FUNCTION (fn); id.decl_map = &decl_map; - id.copy_decl = copy_decl_no_change; + id.copy_decl = [] (tree decl, copy_body_data *id) + { + if (TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == CONST_DECL) + /* Don't make copies of local types or enumerators, the C++ + constexpr evaluator doesn't need them and they cause problems + with modules. */ + return decl; + return copy_decl_no_change (decl, id); + }; id.transform_call_graph_edges = CB_CGE_DUPLICATE; id.transform_new_cfg = false; id.transform_return_to_modify = false;
Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for trunk? -- >8 -- For local enums defined in a non-template function or a function template instantiation it seems we neglect to make the function depend on the enum definition, which ultimately causes streaming to fail due to the enum definition not being streamed before uses of its enumerators are streamed, as far as I can tell. The code responsible for adding such dependencies is gcc/cp/module.cc @@ -8784,17 +8784,6 @@ trees_out::decl_node (tree decl, walk_kind ref) depset *dep = NULL; if (streaming_p ()) dep = dep_hash->find_dependency (decl); ! else if (TREE_CODE (ctx) != FUNCTION_DECL ! || TREE_CODE (decl) == TEMPLATE_DECL ! || (dep_hash->sneakoscope && DECL_IMPLICIT_TYPEDEF_P (decl)) ! || (DECL_LANG_SPECIFIC (decl) ! && DECL_MODULE_IMPORT_P (decl))) ! { ! auto kind = (TREE_CODE (decl) == NAMESPACE_DECL ! && !DECL_NAMESPACE_ALIAS (decl) ! ? depset::EK_NAMESPACE : depset::EK_DECL); ! dep = dep_hash->add_dependency (decl, kind); ! } if (!dep) { and the condition there notably excludes local TYPE_DECLs from a non-template function or a function template instantiation. (For a TYPE_DECL from a function template definition, we'll be dealing with the corresponding TEMPLATE_DECL instead, so we'll add the dependency.) Local classes seem fine as-is but perhaps by accident: with a local class we end up depending on the injected-class-name of the local class since it satisfies the above conditions. A local enum doesn't have such a TYPE_DECL member than we can depend on (its CONST_DECLs are handled earlier as tt_enum_decl tags). This patch attempts to fix this by keeping the 'sneakoscope' flag set while walking the definition of a function, so that we add this needed dependency between a containing function (non-template or specialization) and its local types. Currently it's set only when walking the declaration (presumably to catch local types that escape via a deduced return type), but it seems to make sense to add a dependency regardless of the type escapes. This was nearly enough to make things work, except we now ran into issues with the local TYPE/CONST_DECL copies when streaming the constexpr version of a function body. It occurred to me that we don't need to make copies of local types when copying a constexpr function body; only VAR_DECLs etc need to be copied for sake of recursive constexpr calls. So this patch adjusts copy_fn accordingly. PR c++/104919 PR c++/106009 gcc/cp/ChangeLog: * module.cc (depset::hash::find_dependencies): Keep sneakoscope set when walking the definition. gcc/ChangeLog: * tree-inline.cc (remap_decl): Handle copy_decl returning the original decl. (remap_decls): Handle remap_decl returning the original decl. (copy_fn): Adjust copy_decl callback to skip TYPE_DECL and CONST_DECL. gcc/testsuite/ChangeLog: * g++.dg/modules/tdef-7.h: * g++.dg/modules/tdef-7_b.C: * g++.dg/modules/enum-13_a.C: New test. * g++.dg/modules/enum-13_b.C: New test. --- gcc/cp/module.cc | 2 +- gcc/testsuite/g++.dg/modules/enum-13_a.C | 23 +++++++++++++++++++++++ gcc/testsuite/g++.dg/modules/enum-13_b.C | 8 ++++++++ gcc/testsuite/g++.dg/modules/tdef-7.h | 2 -- gcc/testsuite/g++.dg/modules/tdef-7_b.C | 2 +- gcc/tree-inline.cc | 14 +++++++++++--- 6 files changed, 44 insertions(+), 7 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/enum-13_a.C create mode 100644 gcc/testsuite/g++.dg/modules/enum-13_b.C