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
>
>
