diff mbox series

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

Message ID 65ea7e1e.630a0220.d1c27.476d@mx.google.com
State New
Headers show
Series [v2] c++: Check module attachment instead of just purview when necessary [PR112631] | expand

Commit Message

Nathaniel Shead March 8, 2024, 2:55 a.m. UTC
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.
	(start_preparsed_function): Ensure block-scope functions are
	emitted in module interfaces.
	* decl2.cc (determine_visibility): Likewise.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/block-decl-1_a.C: New test.
	* g++.dg/modules/block-decl-1_b.C: New test.
	* g++.dg/modules/block-decl-2_a.C: New test.
	* g++.dg/modules/block-decl-2_b.C: New test.
	* g++.dg/modules/block-decl-2_c.C: New test.
	* g++.dg/modules/block-decl-3.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/cp-tree.h                              |   2 +
 gcc/cp/decl.cc                                |  22 ++-
 gcc/cp/decl2.cc                               |  23 +--
 gcc/testsuite/g++.dg/modules/block-decl-1_a.C |   9 ++
 gcc/testsuite/g++.dg/modules/block-decl-1_b.C |  10 ++
 gcc/testsuite/g++.dg/modules/block-decl-2_a.C | 143 ++++++++++++++++++
 gcc/testsuite/g++.dg/modules/block-decl-2_b.C |   8 +
 gcc/testsuite/g++.dg/modules/block-decl-2_c.C |  25 +++
 gcc/testsuite/g++.dg/modules/block-decl-3.C   |  21 +++
 9 files changed, 249 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-1_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-1_b.C
 create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-2_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-2_b.C
 create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-2_c.C
 create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-3.C

Comments

Jason Merrill March 8, 2024, 3:19 p.m. UTC | #1
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
Nathaniel Shead March 8, 2024, 11:18 p.m. UTC | #2
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
Jason Merrill March 11, 2024, 6:13 p.m. UTC | #3
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
diff mbox series

Patch

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 14895bc6585..05913861e06 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7381,6 +7381,8 @@  inline bool module_attach_p ()
 
 inline bool named_module_purview_p ()
 { return named_module_p () && module_purview_p (); }
+inline bool named_module_attach_p ()
+{ return named_module_p () && module_attach_p (); }
 
 /* We're currently exporting declarations.  */
 inline bool module_exporting_p ()
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index dbc3df24e77..92475ecc28f 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -6092,10 +6092,10 @@  start_decl (const cp_declarator *declarator,
     {
       /* A function-scope decl of some namespace-scope decl.  */
       DECL_LOCAL_DECL_P (decl) = true;
-      if (named_module_purview_p ())
+      if (named_module_attach_p ())
 	error_at (declarator->id_loc,
-		  "block-scope extern declaration %q#D not permitted"
-		  " in module purview", decl);
+		  "block-scope extern declaration %q#D must not be"
+		  " attached to a named module", decl);
     }
 
   /* Enter this declaration into the symbol table.  Don't push the plain
@@ -18054,6 +18054,18 @@  start_preparsed_function (tree decl1, tree attrs, int flags)
 	/* This is a function in a local class in an extern inline
 	   or template function.  */
 	comdat_linkage (decl1);
+
+      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);
+	}
     }
   /* If this function belongs to an interface, it is public.
      If it belongs to someone else's interface, it is also external.
@@ -18907,10 +18919,10 @@  grokmethod (cp_decl_specifier_seq *declspecs,
   check_template_shadow (fndecl);
 
   /* p1779 ABI-Isolation makes inline not a default for in-class
-     definitions in named module purview.  If the user explicitly
+     definitions attached to a named module.  If the user explicitly
      made it inline, grokdeclarator will already have done the right
      things.  */
-  if ((!named_module_purview_p ()
+  if ((!named_module_attach_p ()
        || flag_module_implicit_inline
       /* Lambda's operator function remains inline.  */
        || LAMBDA_TYPE_P (DECL_CONTEXT (fndecl)))
diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
index 6c9fd415d40..94eaf98c609 100644
--- a/gcc/cp/decl2.cc
+++ b/gcc/cp/decl2.cc
@@ -3050,15 +3050,20 @@  determine_visibility (tree decl)
 	constrain_visibility (decl, tvis, false);
     }
   else if (no_linkage_check (TREE_TYPE (decl), /*relaxed_p=*/true))
-    /* 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.
-
-       Since non-extern "C" decls need to be defined in the same
-       translation unit, we can make the type internal.  */
-    constrain_visibility (decl, VISIBILITY_ANON, false);
+    {
+      /* 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.
+
+	 Since non-extern "C" decls need to be defined in the same
+	 translation unit, we can make the type internal, unless this
+	 type is part of a module CMI, in which case importers may need
+	 to access it.  */
+      if (!module_has_cmi_p ())
+	constrain_visibility (decl, VISIBILITY_ANON, false);
+    }
 
   /* If visibility changed and DECL already has DECL_RTL, ensure
      symbol flags are updated.  */
diff --git a/gcc/testsuite/g++.dg/modules/block-decl-1_a.C b/gcc/testsuite/g++.dg/modules/block-decl-1_a.C
new file mode 100644
index 00000000000..e7ffc629192
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/block-decl-1_a.C
@@ -0,0 +1,9 @@ 
+// { dg-additional-options "-fmodules-ts" }
+// { dg-module-cmi bla }
+
+export module bla;
+
+export extern "C++" inline void fun() {
+  void oops();  // { dg-bogus "block-scope extern declaration" }
+  oops();
+}
diff --git a/gcc/testsuite/g++.dg/modules/block-decl-1_b.C b/gcc/testsuite/g++.dg/modules/block-decl-1_b.C
new file mode 100644
index 00000000000..c0d724f25ac
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/block-decl-1_b.C
@@ -0,0 +1,10 @@ 
+// { dg-module-do link }
+// { dg-additional-options "-fmodules-ts" }
+
+import bla;
+
+void oops() {}
+
+int main() {
+  fun();
+}
diff --git a/gcc/testsuite/g++.dg/modules/block-decl-2_a.C b/gcc/testsuite/g++.dg/modules/block-decl-2_a.C
new file mode 100644
index 00000000000..00a4f229ae8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/block-decl-2_a.C
@@ -0,0 +1,143 @@ 
+// { dg-additional-options "-fmodules-ts" }
+// { dg-module-cmi mod }
+
+export module mod;
+
+namespace {
+  void internal() {}
+}
+
+// singly-nested (i=inline, n=non-inline)
+
+export auto n_n() {
+  internal();
+  struct X { void f() { internal(); } };
+  return X{};
+}
+
+export auto n_i() {
+  internal();
+  struct X { inline void f() {} };
+  return X{};
+}
+
+export inline auto i_n() {
+  // `f` is not inline here, so this is OK
+  struct X { void f() { internal(); } };
+  return X{};
+}
+
+export inline auto i_i() {
+  struct X { inline void f() {} };
+  return X{};
+}
+
+
+// doubly nested
+
+export auto n_n_n() {
+  struct X {
+    auto f() {
+      struct Y {
+	void g() { internal(); }
+      };
+      return Y{};
+    }
+  };
+  return X{};
+}
+
+export auto n_i_n() {
+  struct X {
+    inline auto f() {
+      struct Y {
+	void g() { internal(); }
+      };
+      return Y{};
+    }
+  };
+  return X{};
+}
+
+export inline auto i_n_i() {
+  struct X {
+    auto f() {
+      struct Y {
+	inline void g() {}
+      };
+      return Y {};
+    }
+  };
+  return X{};
+}
+
+export inline auto i_i_i() {
+  struct X {
+    inline auto f() {
+      struct Y {
+	inline void g() {}
+      };
+      return Y{};
+    }
+  };
+  return X{};
+}
+
+
+// multiple types
+
+export auto multi_n_n() {
+  struct X {
+    void f() { internal(); }
+  };
+  struct Y {
+    X x;
+  };
+  return Y {};
+}
+
+export auto multi_n_i() {
+  struct X {
+    inline void f() {}
+  };
+  struct Y {
+    X x;
+  };
+  return Y {};
+}
+
+export inline auto multi_i_i() {
+  struct X {
+    inline void f() {}
+  };
+  struct Y {
+    X x;
+  };
+  return Y {};
+};
+
+
+// extern "C++"
+
+export extern "C++" auto extern_n_i() {
+  struct X {
+    void f() {}  // implicitly inline
+  };
+  return X{};
+};
+
+export extern "C++" inline auto extern_i_i() {
+  struct X {
+    void f() {}
+  };
+  return X{};
+};
+
+
+// can access from implementation unit
+
+auto only_used_in_impl() {
+  struct X { void f() {} };
+  return X{};
+}
+export void test_from_impl_unit();
diff --git a/gcc/testsuite/g++.dg/modules/block-decl-2_b.C b/gcc/testsuite/g++.dg/modules/block-decl-2_b.C
new file mode 100644
index 00000000000..bc9b2a213f0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/block-decl-2_b.C
@@ -0,0 +1,8 @@ 
+// { dg-additional-options "-fmodules-ts" }
+
+module mod;
+
+// Test that we can access (and link) to declarations from the interface
+void test_from_impl_unit() {
+  only_used_in_impl().f();
+}
diff --git a/gcc/testsuite/g++.dg/modules/block-decl-2_c.C b/gcc/testsuite/g++.dg/modules/block-decl-2_c.C
new file mode 100644
index 00000000000..ef275d10f0e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/block-decl-2_c.C
@@ -0,0 +1,25 @@ 
+// { dg-module-do link }
+// { dg-additional-options "-fmodules-ts" }
+
+import mod;
+
+int main() {
+  n_n().f();
+  n_i().f();
+  i_n().f();
+  i_i().f();
+
+  n_n_n().f().g();
+  n_i_n().f().g();
+  i_n_i().f().g();
+  i_i_i().f().g();
+
+  multi_n_n().x.f();
+  multi_n_i().x.f();
+  multi_i_i().x.f();
+
+  extern_n_i().f();
+  extern_i_i().f();
+
+  test_from_impl_unit();
+}
diff --git a/gcc/testsuite/g++.dg/modules/block-decl-3.C b/gcc/testsuite/g++.dg/modules/block-decl-3.C
new file mode 100644
index 00000000000..8bbebd06bab
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/block-decl-3.C
@@ -0,0 +1,21 @@ 
+// { dg-additional-options "-fmodules-ts" }
+// { dg-module-cmi !mod }
+
+export module mod;
+
+namespace {
+  void internal();
+}
+
+export extern "C++" auto foo() {
+  struct X {
+    // `foo` is not attached to a named module, and as such
+    // `X::f` should be implicitly `inline` here
+    void f() {  // { dg-error "references internal linkage entity" }
+      internal();
+    }
+  };
+  return X{};
+}
+
+// { dg-prune-output "failed to write compiled module" }