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

[Jason Merrill <jason@redhat.com>]

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.

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