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