Re: [PATCH v2] c++: Check module attachment instead of just purview when necessary [PR112631]

[Nathaniel Shead <nathanieloshead@gmail.com>]

On Fri, Mar 08, 2024 at 10:19:52AM -0500, Jason Merrill wrote:
> On 3/7/24 21:55, Nathaniel Shead wrote:
> > On Mon, Nov 27, 2023 at 03:59:39PM +1100, Nathaniel Shead wrote:
> > > On Thu, Nov 23, 2023 at 03:03:37PM -0500, Nathan Sidwell wrote:
> > > > On 11/20/23 04:47, Nathaniel Shead wrote:
> > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write
> > > > > access.
> > > > > 
> > > > > -- >8 --
> > > > > 
> > > > > Block-scope declarations of functions or extern values are not allowed
> > > > > when attached to a named module. Similarly, class member functions are
> > > > > not inline if attached to a named module. However, in both these cases
> > > > > we currently only check if the declaration is within the module purview;
> > > > > it is possible for such a declaration to occur within the module purview
> > > > > but not be attached to a named module (e.g. in an 'extern "C++"' block).
> > > > > This patch makes the required adjustments.
> > > > 
> > > > 
> > > > Ah I'd been puzzling over the default inlinedness of  member-fns of
> > > > block-scope structs.  Could you augment the testcase to make sure that's
> > > > right too?
> > > > 
> > > > Something like:
> > > > 
> > > > // dg-module-do link
> > > > export module Mod;
> > > > 
> > > > export auto Get () {
> > > >    struct X { void Fn () {} };
> > > >    return X();
> > > > }
> > > > 
> > > > 
> > > > ///
> > > > import Mod
> > > > void Frob () { Get().Fn(); }
> > > > 
> > > 
> > > I gave this a try and it indeed doesn't work correctly; 'Fn' needs to be
> > > marked 'inline' for this to link (whether or not 'Get' itself is
> > > inline). I've tried tracing the code to work out what's going on but
> > > I've been struggling to work out how all the different flags (e.g.
> > > TREE_PUBLIC, TREE_EXTERNAL, TREE_COMDAT, DECL_NOT_REALLY_EXTERN)
> > > interact, which flags we want to be set where, and where the decision of
> > > what function definitions to emit is actually made.
> > > 
> > > I did find that doing 'mark_used(decl)' on all member functions in
> > > block-scope structs seems to work however, but I wonder if that's maybe
> > > too aggressive or if there's something else we should be doing?
> > 
> > I got around to looking at this again, here's an updated version of this
> > patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > 
> > (I'm not sure if 'start_preparsed_function' is the right place to be
> > putting this kind of logic or if it should instead be going in
> > 'grokfndecl', e.g. decl.cc:10761 where the rules for making local
> > functions have no linkage are initially determined, but I found this
> > easier to implement: happy to move around though if preferred.)
> > 
> > -- >8 --
> > 
> > Block-scope declarations of functions or extern values are not allowed
> > when attached to a named module. Similarly, class member functions are
> > not inline if attached to a named module. However, in both these cases
> > we currently only check if the declaration is within the module purview;
> > it is possible for such a declaration to occur within the module purview
> > but not be attached to a named module (e.g. in an 'extern "C++"' block).
> > This patch makes the required adjustments.
> > 
> > While implementing this we discovered that block-scope local functions
> > are not correctly emitted, causing link failures; this patch also
> > corrects some assumptions here and ensures that they are emitted when
> > needed.
> > 
> > 	PR c++/112631
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* cp-tree.h (named_module_attach_p): New function.
> > 	* decl.cc (start_decl): Check for attachment not purview.
> > 	(grokmethod): Likewise.
> 
> These changes are OK; the others I want to consider more.
> 

Thanks, I can commit this as a separate commit if you prefer?

> > +export auto n_n() {
> > +  internal();
> > +  struct X { void f() { internal(); } };
> > +  return X{};
> 
> Hmm, is this not a prohibited exposure?  Seems like X has no linkage because
> it's at block scope, and the deduced return type names it.
> 
> I know we try to support this "voldemort" pattern, but is that actually
> correct?
> 
> Jason
> 

I had similar doubts, but this is not an especially uncommon pattern in
the wild either. I also asked some other people for their thoughts and
got told:

  "no linkage" doesn't mean it's ill-formed to name it in other scopes.
  It means a declaration in another scope cannot correspond to it

And that the wording in [basic.link] p2.4 is imprecise. (Apparently they
were going to raise a core issue about this too, I think?)

As for whether it's an exposure, looking at [basic.link] p15, the entity
'X' doesn't actually appear to be TU-local: it doesn't have a name with
internal linkage (no linkage is different) and is not declared or
introduced within the definition of a TU-local entity (n_n is not
TU-local). So I think this example is OK, but this example is not:

  namespace {
    auto x() {
      struct X { void f() {} };
      return X{};
    }
  }

  export auto illegal() {
    return x();
  }

Which we correctly complain about already:

error: 'struct {anonymous}::x()::X' references internal linkage entity 'auto {anonymous}::x()'
    6 |       struct X { void f() {} };
      |              ^

Nathaniel