c++/modules: Handle gnu_inline attribute, cleanup linkage determination [PR119154]

  Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

Alternatively, a more minimal fix for the PR would be to just special
case the note_vague_linkage_fn calls to not be performed for gnu_inline
functions, if that's a preferable fix this late into stage 4.

-- >8 --

Currently, note_vague_linkage_fn is called on all definitions imported
from modules.  This is not correct, however; among other things, a
gnu_inline function does not have vague linkage, and causes an ICE if
its treated as such.

There are other things that we seem to potentially miss (e.g. dllexport
handling), so it seems sensible to stop trying to manage linkage in such
an ad-hoc manner but to use the normal interfaces more often.

The change to use expand_or_defer_fn exposes a checking-only ICE in
trees_in::assert_definition, where we forget that we already installed a
definition for a function.  We work around this by instead of clearing
DECL_SAVED_TREE entirely in expand_or_defer_fn_1, we instead set it to a
dummy value.  This way we can also avoid the check for !TREE_ASM_WRITTEN
beforehand.

	PR c++/119154

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Clear DECL_INTERFACE_KNOWN
	for external-linkage functions, except gnu_inline.
	(module_state::read_cluster): Use expand_or_defer_fn instead of
	ad-hoc linkage management.
	(post_load_processing): Likewise.
	* semantics.cc (expand_or_defer_fn_1): Don't forget that we had
	a definition at all.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/pr119154_a.C: New test.
	* g++.dg/modules/pr119154_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/module.cc                          | 33 ++++++++---------------
 gcc/cp/semantics.cc                       |  2 +-
 gcc/testsuite/g++.dg/modules/pr119154_a.C |  6 +++++
 gcc/testsuite/g++.dg/modules/pr119154_b.C | 10 +++++++
 4 files changed, 28 insertions(+), 23 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/pr119154_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/pr119154_b.C
  

On 3/10/25 9:50 AM, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> 
> Alternatively, a more minimal fix for the PR would be to just special
> case the note_vague_linkage_fn calls to not be performed for gnu_inline
> functions, if that's a preferable fix this late into stage 4.
> 
> -- >8 --
> 
> Currently, note_vague_linkage_fn is called on all definitions imported
> from modules.  This is not correct, however; among other things, a
> gnu_inline function does not have vague linkage, and causes an ICE if
> its treated as such.
> 
> There are other things that we seem to potentially miss (e.g. dllexport
> handling), so it seems sensible to stop trying to manage linkage in such
> an ad-hoc manner but to use the normal interfaces more often.

Agreed.

> The change to use expand_or_defer_fn exposes a checking-only ICE in
> trees_in::assert_definition, where we forget that we already installed a
> definition for a function.  We work around this by instead of clearing
> DECL_SAVED_TREE entirely in expand_or_defer_fn_1, we instead set it to a
> dummy value.  This way we can also avoid the check for !TREE_ASM_WRITTEN
> beforehand.

Makes sense.

> 	PR c++/119154
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (trees_out::core_bools): Clear DECL_INTERFACE_KNOWN
> 	for external-linkage functions, except gnu_inline.

...but why is this change needed?  It seems like adding an ad-hoc subset 
of vague_linkage_p while we're trying to remove ad-hocery elsewhere. 
And I'm skeptical about doing this here for vague linkage in general; 
the typeinfo/vtable bits are for a specific ABI requirement, not because 
they're vague linkage.

It does seem like we need to teach vague_linkage_p about gnu_inline, 
would that remove the motivation for this change?

> 	(module_state::read_cluster): Use expand_or_defer_fn instead of
> 	ad-hoc linkage management.
> 	(post_load_processing): Likewise.
> 	* semantics.cc (expand_or_defer_fn_1): Don't forget that we had
> 	a definition at all.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/pr119154_a.C: New test.
> 	* g++.dg/modules/pr119154_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/module.cc                          | 33 ++++++++---------------
>   gcc/cp/semantics.cc                       |  2 +-
>   gcc/testsuite/g++.dg/modules/pr119154_a.C |  6 +++++
>   gcc/testsuite/g++.dg/modules/pr119154_b.C | 10 +++++++
>   4 files changed, 28 insertions(+), 23 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/pr119154_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/pr119154_b.C
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 8b0f42951c2..8e0fa3c5bfc 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -212,6 +212,7 @@ Classes used:
>   #define INCLUDE_VECTOR
>   #include "system.h"
>   #include "coretypes.h"
> +#include "target.h"
>   #include "cp-tree.h"
>   #include "timevar.h"
>   #include "stringpool.h"
> @@ -5620,6 +5621,14 @@ trees_out::core_bools (tree t, bits_out& bits)
>   	bool interface_known = t->decl_common.lang_flag_5;
>   	if (VAR_P (t) && (DECL_VTABLE_OR_VTT_P (t) || DECL_TINFO_P (t)))
>   	  interface_known = false;
> +	/* External linkage functions should also be redetermined on
> +	   stream-in, except gnu_inline which is always external.  */
> +	if (interface_known
> +	    && TREE_CODE (t) == FUNCTION_DECL
> +	    && TREE_PUBLIC (t)
> +	    && !(DECL_DECLARED_INLINE_P (t)
> +		 && lookup_attribute("gnu_inline", DECL_ATTRIBUTES (t))))
> +	  interface_known = false;
>   	WB (interface_known);
>         }
>   
> @@ -16417,12 +16426,7 @@ module_state::read_cluster (unsigned snum)
>   	  cfun->returns_pcc_struct = aggr;
>   #endif
>   	  cfun->returns_struct = aggr;
> -
> -	  if (DECL_COMDAT (decl))
> -	    // FIXME: Comdat grouping?
> -	    comdat_linkage (decl);
> -	  note_vague_linkage_fn (decl);
> -	  cgraph_node::finalize_function (decl, true);
> +	  expand_or_defer_fn (decl);
>   	}
>   
>       }
> @@ -18901,22 +18905,7 @@ post_load_processing ()
>         dump () && dump ("Post-load processing of %N", decl);
>   
>         gcc_checking_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl));
> -
> -      if (DECL_COMDAT (decl))
> -	comdat_linkage (decl);
> -      if (!TREE_ASM_WRITTEN (decl))
> -	{
> -	  /* Cloning can cause loading -- specifically operator delete for
> -	     the deleting dtor.  */
> -	  if (maybe_clone_body (decl))
> -	    TREE_ASM_WRITTEN (decl) = 1;
> -	  else
> -	    {
> -	      /* We didn't clone the cdtor, make sure we emit it.  */
> -	      note_vague_linkage_fn (decl);
> -	      cgraph_node::finalize_function (decl, true);
> -	    }
> -	}
> +      expand_or_defer_fn (decl);
>       }
>   
>     cfun = old_cfun;
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 7c7d3e3c432..914502abe7a 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -5504,7 +5504,7 @@ expand_or_defer_fn_1 (tree fn)
>   	 need it anymore.  */
>         if (!DECL_DECLARED_CONSTEXPR_P (fn)
>   	  && !(module_maybe_has_cmi_p () && vague_linkage_p (fn)))
> -	DECL_SAVED_TREE (fn) = NULL_TREE;
> +	DECL_SAVED_TREE (fn) = void_node;
>         return false;
>       }
>   
> diff --git a/gcc/testsuite/g++.dg/modules/pr119154_a.C b/gcc/testsuite/g++.dg/modules/pr119154_a.C
> new file mode 100644
> index 00000000000..23bd186fe19
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr119154_a.C
> @@ -0,0 +1,6 @@
> +// PR c++/119154
> +// { dg-additional-options "-fmodules" }
> +// { dg-module-cmi foo }
> +
> +export module foo;
> +extern "C++" inline __attribute__((__gnu_inline__)) void bar() {}
> diff --git a/gcc/testsuite/g++.dg/modules/pr119154_b.C b/gcc/testsuite/g++.dg/modules/pr119154_b.C
> new file mode 100644
> index 00000000000..1558e717761
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr119154_b.C
> @@ -0,0 +1,10 @@
> +// PR c++/119154
> +// { dg-module-do link }
> +// { dg-additional-options "-fmodules" }
> +
> +void bar();
> +import foo;
> +
> +int main() {
> +  bar();
> +}
  

On Wed, Mar 12, 2025 at 08:52:19AM -0400, Jason Merrill wrote:
> On 3/10/25 9:50 AM, Nathaniel Shead wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > 
> > Alternatively, a more minimal fix for the PR would be to just special
> > case the note_vague_linkage_fn calls to not be performed for gnu_inline
> > functions, if that's a preferable fix this late into stage 4.
> > 
> > -- >8 --
> > 
> > Currently, note_vague_linkage_fn is called on all definitions imported
> > from modules.  This is not correct, however; among other things, a
> > gnu_inline function does not have vague linkage, and causes an ICE if
> > its treated as such.
> > 
> > There are other things that we seem to potentially miss (e.g. dllexport
> > handling), so it seems sensible to stop trying to manage linkage in such
> > an ad-hoc manner but to use the normal interfaces more often.
> 
> Agreed.
> 
> > The change to use expand_or_defer_fn exposes a checking-only ICE in
> > trees_in::assert_definition, where we forget that we already installed a
> > definition for a function.  We work around this by instead of clearing
> > DECL_SAVED_TREE entirely in expand_or_defer_fn_1, we instead set it to a
> > dummy value.  This way we can also avoid the check for !TREE_ASM_WRITTEN
> > beforehand.
> 
> Makes sense.
> 
> > 	PR c++/119154
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* module.cc (trees_out::core_bools): Clear DECL_INTERFACE_KNOWN
> > 	for external-linkage functions, except gnu_inline.
> 
> ...but why is this change needed?  It seems like adding an ad-hoc subset of
> vague_linkage_p while we're trying to remove ad-hocery elsewhere. And I'm
> skeptical about doing this here for vague linkage in general; the
> typeinfo/vtable bits are for a specific ABI requirement, not because they're
> vague linkage.
> 
> It does seem like we need to teach vague_linkage_p about gnu_inline, would
> that remove the motivation for this change?

Thanks.  And right, that's a good point.  We still need a bit of this
change (as otherwise import_export_decl would never attempt to
redetermine whether we need to actually emit vague linkage entities,
since DECL_INTERFACE_KNOWN would still be set from when the exporter
processed these declarations) but it seems reasonable to move this
special-casing of gnu_inline into vague_linkage_p.

My initial concern with this was whether this would stop us from
emitting the definition of the function, but has_definition just uses
DECL_DECLARED_INLINE_P for FUNCTION_DECLs so it should continue working
correctly.

Here's an updated patch which does this, and also as a drive-by fix I
noticed while working on this, ensures that imported inline variables
are correctly marked as COMDAT.  Maybe there's a better way to do this
though: I'm not sure how concerned to be about !flag_weak handling?

Tested modules.exp so far on x86_64-pc-linux-gnu, OK for trunk if full
bootstrap and regtest succeeds?

-- >8 --

Currently, note_vague_linkage_fn is called on all definitions imported
from modules.  This is not correct, however; among other things, a
gnu_inline function does not have vague linkage, and causes an ICE if
its treated as such.

There are other things that we seem to potentially miss (e.g. dllexport
handling), so it seems sensible to stop trying to manage linkage in such
an ad-hoc manner but to use the normal interfaces more often.  While
looking at this I also found that we seem to miss marking vague linkage
variables as COMDAT, so this patch fixes that as well.

The change to use expand_or_defer_fn exposes a checking-only ICE in
trees_in::assert_definition, where we forget that we already installed a
definition for a function.  We work around this by instead of clearing
DECL_SAVED_TREE entirely in expand_or_defer_fn_1, we instead set it to a
dummy value.  This way we can also avoid the check for !TREE_ASM_WRITTEN
beforehand.

	PR c++/119154

gcc/cp/ChangeLog:

	* decl2.cc (vague_linkage_p): Don't treat gnu_inline functions
	as having vague linkage.
	* module.cc (trees_out::core_bools): Clear DECL_INTERFACE_KNOWN
	for vague-linkage entities.
	(read_var_def): Maybe set comdat linkage for imported var
	definitions.
	(module_state::read_cluster): Use expand_or_defer_fn instead of
	ad-hoc linkage management.
	(post_load_processing): Likewise.
	* semantics.cc (expand_or_defer_fn_1): Don't forget that we had
	a definition at all.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/linkage-3_a.C: New test.
	* g++.dg/modules/linkage-3_b.C: New test.
	* g++.dg/modules/pr119154_a.C: New test.
	* g++.dg/modules/pr119154_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/decl2.cc                            |  4 ++-
 gcc/cp/module.cc                           | 33 ++++++----------------
 gcc/cp/semantics.cc                        |  2 +-
 gcc/testsuite/g++.dg/modules/linkage-3_a.C |  5 ++++
 gcc/testsuite/g++.dg/modules/linkage-3_b.C |  9 ++++++
 gcc/testsuite/g++.dg/modules/pr119154_a.C  |  6 ++++
 gcc/testsuite/g++.dg/modules/pr119154_b.C  | 10 +++++++
 7 files changed, 42 insertions(+), 27 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/linkage-3_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/linkage-3_b.C
 create mode 100644 gcc/testsuite/g++.dg/modules/pr119154_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/pr119154_b.C

diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
index fe9c56b6637..4a9fb1c3c00 100644
--- a/gcc/cp/decl2.cc
+++ b/gcc/cp/decl2.cc
@@ -2482,7 +2482,9 @@ vague_linkage_p (tree decl)
      DECL_COMDAT.  */
   if (DECL_COMDAT (decl)
       || (TREE_CODE (decl) == FUNCTION_DECL
-	  && DECL_DECLARED_INLINE_P (decl))
+	  && DECL_DECLARED_INLINE_P (decl)
+	  /* But gnu_inline functions are always external.  */
+	  && !lookup_attribute ("gnu_inline", DECL_ATTRIBUTES (decl)))
       || (DECL_LANG_SPECIFIC (decl)
 	  && DECL_TEMPLATE_INSTANTIATION (decl))
       || (VAR_P (decl) && DECL_INLINE_VAR_P (decl)))
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 97c3549093c..5374e412b9f 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -5615,11 +5615,13 @@ trees_out::core_bools (tree t, bits_out& bits)
 
       {
 	/* This is DECL_INTERFACE_KNOWN: We should redetermine whether
-	   we need to import or export any vtables or typeinfo objects
-	   on stream-in.  */
+	   we need to import or export any vtables, typeinfo objects,
+	   or other vague-linkage entities on stream-in.  */
 	bool interface_known = t->decl_common.lang_flag_5;
 	if (VAR_P (t) && (DECL_VTABLE_OR_VTT_P (t) || DECL_TINFO_P (t)))
 	  interface_known = false;
+	if (interface_known && vague_linkage_p (t))
+	  interface_known = false;
 	WB (interface_known);
       }
 
@@ -12551,6 +12553,7 @@ trees_in::read_var_def (tree decl, tree maybe_template)
   bool installing = maybe_dup && !initialized;
   if (installing)
     {
+      DECL_INITIAL (decl) = init;
       if (DECL_EXTERNAL (decl))
 	DECL_NOT_REALLY_EXTERN (decl) = true;
       if (VAR_P (decl))
@@ -12558,13 +12561,13 @@ trees_in::read_var_def (tree decl, tree maybe_template)
 	  DECL_INITIALIZED_P (decl) = true;
 	  if (maybe_dup && DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (maybe_dup))
 	    DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl) = true;
+	  tentative_decl_linkage (decl);
 	  if (DECL_IMPLICIT_INSTANTIATION (decl)
 	      || (DECL_CLASS_SCOPE_P (decl)
 		  && !DECL_VTABLE_OR_VTT_P (decl)
 		  && !DECL_TEMPLATE_INFO (decl)))
 	    note_vague_linkage_variable (decl);
 	}
-      DECL_INITIAL (decl) = init;
       if (!dyn_init)
 	;
       else if (CP_DECL_THREAD_LOCAL_P (decl))
@@ -16469,12 +16472,7 @@ module_state::read_cluster (unsigned snum)
 	  cfun->returns_pcc_struct = aggr;
 #endif
 	  cfun->returns_struct = aggr;
-
-	  if (DECL_COMDAT (decl))
-	    // FIXME: Comdat grouping?
-	    comdat_linkage (decl);
-	  note_vague_linkage_fn (decl);
-	  cgraph_node::finalize_function (decl, true);
+	  expand_or_defer_fn (decl);
 	}
 
     }
@@ -18953,22 +18951,7 @@ post_load_processing ()
       dump () && dump ("Post-load processing of %N", decl);
 
       gcc_checking_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl));
-
-      if (DECL_COMDAT (decl))
-	comdat_linkage (decl);
-      if (!TREE_ASM_WRITTEN (decl))
-	{
-	  /* Cloning can cause loading -- specifically operator delete for
-	     the deleting dtor.  */
-	  if (maybe_clone_body (decl))
-	    TREE_ASM_WRITTEN (decl) = 1;
-	  else
-	    {
-	      /* We didn't clone the cdtor, make sure we emit it.  */
-	      note_vague_linkage_fn (decl);
-	      cgraph_node::finalize_function (decl, true);
-	    }
-	}
+      expand_or_defer_fn (decl);
     }
 
   cfun = old_cfun;
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 7c7d3e3c432..914502abe7a 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -5504,7 +5504,7 @@ expand_or_defer_fn_1 (tree fn)
 	 need it anymore.  */
       if (!DECL_DECLARED_CONSTEXPR_P (fn)
 	  && !(module_maybe_has_cmi_p () && vague_linkage_p (fn)))
-	DECL_SAVED_TREE (fn) = NULL_TREE;
+	DECL_SAVED_TREE (fn) = void_node;
       return false;
     }
 
diff --git a/gcc/testsuite/g++.dg/modules/linkage-3_a.C b/gcc/testsuite/g++.dg/modules/linkage-3_a.C
new file mode 100644
index 00000000000..2bee9c87957
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/linkage-3_a.C
@@ -0,0 +1,5 @@
+// { dg-do compile { target *-*-*gnu* } }
+// { dg-additional-options "-fmodules" }
+
+export module M;
+export inline int x = 0;
diff --git a/gcc/testsuite/g++.dg/modules/linkage-3_b.C b/gcc/testsuite/g++.dg/modules/linkage-3_b.C
new file mode 100644
index 00000000000..ae145217cac
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/linkage-3_b.C
@@ -0,0 +1,9 @@
+// { dg-do compile { target *-*-*gnu* } }
+// { dg-additional-options "-fmodules" }
+// { dg-final { scan-assembler "_ZW1M1x,comdat" } }
+
+import M;
+
+int main() {
+  return x;
+}
diff --git a/gcc/testsuite/g++.dg/modules/pr119154_a.C b/gcc/testsuite/g++.dg/modules/pr119154_a.C
new file mode 100644
index 00000000000..23bd186fe19
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr119154_a.C
@@ -0,0 +1,6 @@
+// PR c++/119154
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi foo }
+
+export module foo;
+extern "C++" inline __attribute__((__gnu_inline__)) void bar() {}
diff --git a/gcc/testsuite/g++.dg/modules/pr119154_b.C b/gcc/testsuite/g++.dg/modules/pr119154_b.C
new file mode 100644
index 00000000000..1558e717761
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr119154_b.C
@@ -0,0 +1,10 @@
+// PR c++/119154
+// { dg-module-do link }
+// { dg-additional-options "-fmodules" }
+
+void bar();
+import foo;
+
+int main() {
+  bar();
+}
  

On 3/12/25 10:43 AM, Nathaniel Shead wrote:
> On Wed, Mar 12, 2025 at 08:52:19AM -0400, Jason Merrill wrote:
>> On 3/10/25 9:50 AM, Nathaniel Shead wrote:
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>>
>>> Alternatively, a more minimal fix for the PR would be to just special
>>> case the note_vague_linkage_fn calls to not be performed for gnu_inline
>>> functions, if that's a preferable fix this late into stage 4.
>>>
>>> -- >8 --
>>>
>>> Currently, note_vague_linkage_fn is called on all definitions imported
>>> from modules.  This is not correct, however; among other things, a
>>> gnu_inline function does not have vague linkage, and causes an ICE if
>>> its treated as such.
>>>
>>> There are other things that we seem to potentially miss (e.g. dllexport
>>> handling), so it seems sensible to stop trying to manage linkage in such
>>> an ad-hoc manner but to use the normal interfaces more often.
>>
>> Agreed.
>>
>>> The change to use expand_or_defer_fn exposes a checking-only ICE in
>>> trees_in::assert_definition, where we forget that we already installed a
>>> definition for a function.  We work around this by instead of clearing
>>> DECL_SAVED_TREE entirely in expand_or_defer_fn_1, we instead set it to a
>>> dummy value.  This way we can also avoid the check for !TREE_ASM_WRITTEN
>>> beforehand.
>>
>> Makes sense.
>>
>>> 	PR c++/119154
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* module.cc (trees_out::core_bools): Clear DECL_INTERFACE_KNOWN
>>> 	for external-linkage functions, except gnu_inline.
>>
>> ...but why is this change needed?  It seems like adding an ad-hoc subset of
>> vague_linkage_p while we're trying to remove ad-hocery elsewhere. And I'm
>> skeptical about doing this here for vague linkage in general; the
>> typeinfo/vtable bits are for a specific ABI requirement, not because they're
>> vague linkage.
>>
>> It does seem like we need to teach vague_linkage_p about gnu_inline, would
>> that remove the motivation for this change?
> 
> Thanks.  And right, that's a good point.  We still need a bit of this
> change (as otherwise import_export_decl would never attempt to
> redetermine whether we need to actually emit vague linkage entities,
> since DECL_INTERFACE_KNOWN would still be set from when the exporter
> processed these declarations)

Ah, so expand_or_defer_fn_1 wouldn't call tentative_decl_linkage, so 
note_vague_linkage_fn wouldn't happen.

So, we want to clear DECL_INTERFACE_KNOWN at some point.  It might make 
sense to do that in read_var_def/read_function_def instead of 
core_bools, but it's certainly more straightforward to stick with the 
existing location.

> but it seems reasonable to move this
> special-casing of gnu_inline into vague_linkage_p.
> 
> My initial concern with this was whether this would stop us from
> emitting the definition of the function, but has_definition just uses
> DECL_DECLARED_INLINE_P for FUNCTION_DECLs so it should continue working
> correctly.
> 
> Here's an updated patch which does this, and also as a drive-by fix I
> noticed while working on this, ensures that imported inline variables
> are correctly marked as COMDAT.  Maybe there's a better way to do this
> though: I'm not sure how concerned to be about !flag_weak handling?
> 
> Tested modules.exp so far on x86_64-pc-linux-gnu, OK for trunk if full
> bootstrap and regtest succeeds?
> 
> -- >8 --
> 
> Currently, note_vague_linkage_fn is called on all definitions imported
> from modules.  This is not correct, however; among other things, a
> gnu_inline function does not have vague linkage, and causes an ICE if
> its treated as such.
> 
> There are other things that we seem to potentially miss (e.g. dllexport
> handling), so it seems sensible to stop trying to manage linkage in such
> an ad-hoc manner but to use the normal interfaces more often.  While
> looking at this I also found that we seem to miss marking vague linkage
> variables as COMDAT, so this patch fixes that as well.
> 
> The change to use expand_or_defer_fn exposes a checking-only ICE in
> trees_in::assert_definition, where we forget that we already installed a
> definition for a function.  We work around this by instead of clearing
> DECL_SAVED_TREE entirely in expand_or_defer_fn_1, we instead set it to a
> dummy value.  This way we can also avoid the check for !TREE_ASM_WRITTEN
> beforehand.
> 
> 	PR c++/119154
> 
> gcc/cp/ChangeLog:
> 
> 	* decl2.cc (vague_linkage_p): Don't treat gnu_inline functions
> 	as having vague linkage.
> 	* module.cc (trees_out::core_bools): Clear DECL_INTERFACE_KNOWN
> 	for vague-linkage entities.
> 	(read_var_def): Maybe set comdat linkage for imported var
> 	definitions.
> 	(module_state::read_cluster): Use expand_or_defer_fn instead of
> 	ad-hoc linkage management.
> 	(post_load_processing): Likewise.
> 	* semantics.cc (expand_or_defer_fn_1): Don't forget that we had
> 	a definition at all.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/linkage-3_a.C: New test.
> 	* g++.dg/modules/linkage-3_b.C: New test.
> 	* g++.dg/modules/pr119154_a.C: New test.
> 	* g++.dg/modules/pr119154_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/decl2.cc                            |  4 ++-
>   gcc/cp/module.cc                           | 33 ++++++----------------
>   gcc/cp/semantics.cc                        |  2 +-
>   gcc/testsuite/g++.dg/modules/linkage-3_a.C |  5 ++++
>   gcc/testsuite/g++.dg/modules/linkage-3_b.C |  9 ++++++
>   gcc/testsuite/g++.dg/modules/pr119154_a.C  |  6 ++++
>   gcc/testsuite/g++.dg/modules/pr119154_b.C  | 10 +++++++
>   7 files changed, 42 insertions(+), 27 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/linkage-3_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/linkage-3_b.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/pr119154_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/pr119154_b.C
> 
> diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
> index fe9c56b6637..4a9fb1c3c00 100644
> --- a/gcc/cp/decl2.cc
> +++ b/gcc/cp/decl2.cc
> @@ -2482,7 +2482,9 @@ vague_linkage_p (tree decl)
>        DECL_COMDAT.  */
>     if (DECL_COMDAT (decl)
>         || (TREE_CODE (decl) == FUNCTION_DECL
> -	  && DECL_DECLARED_INLINE_P (decl))
> +	  && DECL_DECLARED_INLINE_P (decl)
> +	  /* But gnu_inline functions are always external.  */
> +	  && !lookup_attribute ("gnu_inline", DECL_ATTRIBUTES (decl)))
>         || (DECL_LANG_SPECIFIC (decl)
>   	  && DECL_TEMPLATE_INSTANTIATION (decl))
>         || (VAR_P (decl) && DECL_INLINE_VAR_P (decl)))
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 97c3549093c..5374e412b9f 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -5615,11 +5615,13 @@ trees_out::core_bools (tree t, bits_out& bits)
>   
>         {
>   	/* This is DECL_INTERFACE_KNOWN: We should redetermine whether
> -	   we need to import or export any vtables or typeinfo objects
> -	   on stream-in.  */
> +	   we need to import or export any vtables, typeinfo objects,
> +	   or other vague-linkage entities on stream-in.  */
>   	bool interface_known = t->decl_common.lang_flag_5;
>   	if (VAR_P (t) && (DECL_VTABLE_OR_VTT_P (t) || DECL_TINFO_P (t)))
>   	  interface_known = false;

If we're doing this for all vague_linkage_p, we don't also need to 
specifically handle vtables/tinfos.

OK with the above two lines removed and the comment adjusted.

> +	if (interface_known && vague_linkage_p (t))
> +	  interface_known = false;
>   	WB (interface_known);
>         }
>   
> @@ -12551,6 +12553,7 @@ trees_in::read_var_def (tree decl, tree maybe_template)
>     bool installing = maybe_dup && !initialized;
>     if (installing)
>       {
> +      DECL_INITIAL (decl) = init;
>         if (DECL_EXTERNAL (decl))
>   	DECL_NOT_REALLY_EXTERN (decl) = true;
>         if (VAR_P (decl))
> @@ -12558,13 +12561,13 @@ trees_in::read_var_def (tree decl, tree maybe_template)
>   	  DECL_INITIALIZED_P (decl) = true;
>   	  if (maybe_dup && DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (maybe_dup))
>   	    DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl) = true;
> +	  tentative_decl_linkage (decl);
>   	  if (DECL_IMPLICIT_INSTANTIATION (decl)
>   	      || (DECL_CLASS_SCOPE_P (decl)
>   		  && !DECL_VTABLE_OR_VTT_P (decl)
>   		  && !DECL_TEMPLATE_INFO (decl)))
>   	    note_vague_linkage_variable (decl);
>   	}
> -      DECL_INITIAL (decl) = init;
>         if (!dyn_init)
>   	;
>         else if (CP_DECL_THREAD_LOCAL_P (decl))
> @@ -16469,12 +16472,7 @@ module_state::read_cluster (unsigned snum)
>   	  cfun->returns_pcc_struct = aggr;
>   #endif
>   	  cfun->returns_struct = aggr;
> -
> -	  if (DECL_COMDAT (decl))
> -	    // FIXME: Comdat grouping?
> -	    comdat_linkage (decl);
> -	  note_vague_linkage_fn (decl);
> -	  cgraph_node::finalize_function (decl, true);
> +	  expand_or_defer_fn (decl);
>   	}
>   
>       }
> @@ -18953,22 +18951,7 @@ post_load_processing ()
>         dump () && dump ("Post-load processing of %N", decl);
>   
>         gcc_checking_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl));
> -
> -      if (DECL_COMDAT (decl))
> -	comdat_linkage (decl);
> -      if (!TREE_ASM_WRITTEN (decl))
> -	{
> -	  /* Cloning can cause loading -- specifically operator delete for
> -	     the deleting dtor.  */
> -	  if (maybe_clone_body (decl))
> -	    TREE_ASM_WRITTEN (decl) = 1;
> -	  else
> -	    {
> -	      /* We didn't clone the cdtor, make sure we emit it.  */
> -	      note_vague_linkage_fn (decl);
> -	      cgraph_node::finalize_function (decl, true);
> -	    }
> -	}
> +      expand_or_defer_fn (decl);
>       }
>   
>     cfun = old_cfun;
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 7c7d3e3c432..914502abe7a 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -5504,7 +5504,7 @@ expand_or_defer_fn_1 (tree fn)
>   	 need it anymore.  */
>         if (!DECL_DECLARED_CONSTEXPR_P (fn)
>   	  && !(module_maybe_has_cmi_p () && vague_linkage_p (fn)))
> -	DECL_SAVED_TREE (fn) = NULL_TREE;
> +	DECL_SAVED_TREE (fn) = void_node;
>         return false;
>       }
>   
> diff --git a/gcc/testsuite/g++.dg/modules/linkage-3_a.C b/gcc/testsuite/g++.dg/modules/linkage-3_a.C
> new file mode 100644
> index 00000000000..2bee9c87957
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/linkage-3_a.C
> @@ -0,0 +1,5 @@
> +// { dg-do compile { target *-*-*gnu* } }
> +// { dg-additional-options "-fmodules" }
> +
> +export module M;
> +export inline int x = 0;
> diff --git a/gcc/testsuite/g++.dg/modules/linkage-3_b.C b/gcc/testsuite/g++.dg/modules/linkage-3_b.C
> new file mode 100644
> index 00000000000..ae145217cac
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/linkage-3_b.C
> @@ -0,0 +1,9 @@
> +// { dg-do compile { target *-*-*gnu* } }
> +// { dg-additional-options "-fmodules" }
> +// { dg-final { scan-assembler "_ZW1M1x,comdat" } }
> +
> +import M;
> +
> +int main() {
> +  return x;
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/pr119154_a.C b/gcc/testsuite/g++.dg/modules/pr119154_a.C
> new file mode 100644
> index 00000000000..23bd186fe19
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr119154_a.C
> @@ -0,0 +1,6 @@
> +// PR c++/119154
> +// { dg-additional-options "-fmodules" }
> +// { dg-module-cmi foo }
> +
> +export module foo;
> +extern "C++" inline __attribute__((__gnu_inline__)) void bar() {}
> diff --git a/gcc/testsuite/g++.dg/modules/pr119154_b.C b/gcc/testsuite/g++.dg/modules/pr119154_b.C
> new file mode 100644
> index 00000000000..1558e717761
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr119154_b.C
> @@ -0,0 +1,10 @@
> +// PR c++/119154
> +// { dg-module-do link }
> +// { dg-additional-options "-fmodules" }
> +
> +void bar();
> +import foo;
> +
> +int main() {
> +  bar();
> +}