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.
