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

[Jason Merrill <jason@redhat.com>]

On 3/8/24 18:18, Nathaniel Shead wrote:
> 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?

Please.

>>> +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?
> 
> 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).

Hmm, I think you're right.  And this rule:

> -    /* DR 757: A type without linkage shall not be used as the type of a
> -       variable or function with linkage, unless
> -       o the variable or function has extern "C" linkage (7.5 [dcl.link]), or
> -       o the variable or function is not used (3.2 [basic.def.odr]) or is
> -       defined in the same translation unit.

is no longer part of the standard since C++20; the remnant of this rule 
is the example in

https://eel.is/c++draft/basic#def.odr-11

> auto f() {
>   struct A {};
>   return A{};
> }
> decltype(f()) g();
> auto x = g();

> A program containing this translation unit is ill-formed because g is odr-used but not defined, and cannot be defined in any other translation unit because the local class A cannot be named outside this translation unit.

But g could be defined in another translation unit if f is inline or in 
a module interface unit.

So, I think no_linkage_check needs to consider module_has_cmi_p as well 
as vague_linkage_p for relaxed mode.  And in no_linkage_error if 
no_linkage_check returns null in relaxed mode, reduce the permerror to a 
pedwarn before C++20, and no diagnostic at all in C++20 and above.

> +      if (ctx != NULL_TREE && TREE_PUBLIC (ctx) && module_has_cmi_p ())
> +	{
> +	  /* Ensure that functions in local classes within named modules
> +	     have their definitions exported, in case they are (directly
> +	     or indirectly) used by an importer.  */
> +	  TREE_PUBLIC (decl1) = true;
> +	  if (DECL_DECLARED_INLINE_P (decl1))
> +	    comdat_linkage (decl1);
> +	  else
> +	    mark_needed (decl1);
> +	}

Isn't the inline case handled by the comdat_linkage just above?

Jason