Re: [PATCH v3] c++/modules: Fix handling of extern templates in modules [PR112820]

[Jason Merrill <jason@redhat.com>]

On 1/17/24 01:33, Nathaniel Shead wrote:
> On Mon, Jan 15, 2024 at 06:10:55PM -0500, Jason Merrill wrote:
>> Under what circumstances does it make sense for CLASSTYPE_INTERFACE_ONLY to
>> be set in the context of modules, anyway?  We probably want to propagate it
>> for things in the global module so that various libstdc++ explicit
>> instantiations work the same with import std.
>>
>> For an class imported from a named module, this ties into the earlier
>> discussion about vtables and inlines that hasn't resolved yet in the ABI
>> committee.  But it's certainly significantly interface-like.  And I would
>> expect maybe_suppress_debug_info to suppress the debug info for such a class
>> on the assumption that the module unit has the needed debug info.
>>
>> Jason
>>
> 
> Here's another approach for this patch. This still only fixes the
> specific issues in the PR, I think vtable handling etc. should wait till
> stage 1 because it involves a lot of messing around in decl2.cc.
> 
> As mentioned in the commit message, after thinking more about it I don't
> think we (in general) want to propagate CLASSTYPE_INTERFACE_ONLY, even
> for declarations in the GMF. This makes sense to me because typically it
> can only be accurately determined at the end of the TU, which we haven't
> yet arrived at after importing. For instance, for a polymorphic class in
> the GMF without a key method, that we import from a module and then
> proceed to define the key method later on in this TU.

That sounds right for a module implementation unit or the GMF.

> Bootstrapped and partially regtested on x86_64-pc-linux-gnu (so far only
> modules.exp): OK for trunk if full regtesting passes?

Please add a reference to ABI issue 170 
(https://github.com/itanium-cxx-abi/cxx-abi/issues/170).  OK with that 
change if Nathan doesn't have any further comments this week.

> -- >8 --
> 
> Currently, extern templates are detected by looking for the
> DECL_EXTERNAL flag on a TYPE_DECL. However, this is incorrect:
> TYPE_DECLs don't actually set this flag, and it happens to work by
> coincidence due to TYPE_DECL_SUPPRESS_DEBUG happening to use the same
> underlying bit. This however causes issues with other TYPE_DECLs that
> also happen to have suppressed debug information.
> 
> Instead, this patch reworks the logic so CLASSTYPE_INTERFACE_ONLY is
> always emitted into the module BMI and can then be used to check for an
> extern template correctly.
> 
> Otherwise, for other declarations we want to redetermine this: even for
> declarations from the GMF, we may change our mind on whether to import
> or export depending on decisions made later in the TU after importing so
> we shouldn't decide this now, or necessarily reuse what the module we'd
> imported had decided.
> 
> 	PR c++/112820
> 	PR c++/102607
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (trees_out::lang_type_bools): Write interface_only
> 	and interface_unknown.
> 	(trees_in::lang_type_bools): Read the above flags.
> 	(trees_in::decl_value): Reset CLASSTYPE_INTERFACE_* except for
> 	extern templates.
> 	(trees_in::read_class_def): Remove buggy extern template
> 	handling.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/debug-2_a.C: New test.
> 	* g++.dg/modules/debug-2_b.C: New test.
> 	* g++.dg/modules/debug-2_c.C: New test.
> 	* g++.dg/modules/debug-3_a.C: New test.
> 	* g++.dg/modules/debug-3_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/module.cc                         | 36 +++++++++++++-----------
>   gcc/testsuite/g++.dg/modules/debug-2_a.C |  9 ++++++
>   gcc/testsuite/g++.dg/modules/debug-2_b.C |  8 ++++++
>   gcc/testsuite/g++.dg/modules/debug-2_c.C |  9 ++++++
>   gcc/testsuite/g++.dg/modules/debug-3_a.C |  8 ++++++
>   gcc/testsuite/g++.dg/modules/debug-3_b.C |  9 ++++++
>   6 files changed, 63 insertions(+), 16 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_b.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/debug-2_c.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/debug-3_b.C
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 350ad15dc62..efc1d532a6e 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -5806,10 +5806,8 @@ trees_out::lang_type_bools (tree t)
>   
>     WB ((lang->gets_delete >> 0) & 1);
>     WB ((lang->gets_delete >> 1) & 1);
> -  // Interfaceness is recalculated upon reading.  May have to revisit?
> -  // How do dllexport and dllimport interact across a module?
> -  // lang->interface_only
> -  // lang->interface_unknown
> +  WB (lang->interface_only);
> +  WB (lang->interface_unknown);
>     WB (lang->contains_empty_class_p);
>     WB (lang->anon_aggr);
>     WB (lang->non_zero_init);
> @@ -5877,9 +5875,8 @@ trees_in::lang_type_bools (tree t)
>     v = b () << 0;
>     v |= b () << 1;
>     lang->gets_delete = v;
> -  // lang->interface_only
> -  // lang->interface_unknown
> -  lang->interface_unknown = true; // Redetermine interface
> +  RB (lang->interface_only);
> +  RB (lang->interface_unknown);
>     RB (lang->contains_empty_class_p);
>     RB (lang->anon_aggr);
>     RB (lang->non_zero_init);
> @@ -8246,6 +8243,22 @@ trees_in::decl_value ()
>   	/* Set the TEMPLATE_DECL's type.  */
>   	TREE_TYPE (decl) = TREE_TYPE (inner);
>   
> +      /* Redetermine whether we need to import or export this declaration
> +	 for this TU.  But for extern templates we know we must import:
> +	 they'll be defined in a different TU.
> +	 FIXME How do dllexport and dllimport interact across a module?
> +	 May have to revisit?  */
> +      if (type
> +	  && CLASS_TYPE_P (type)
> +	  && TYPE_LANG_SPECIFIC (type)
> +	  && !(CLASSTYPE_EXPLICIT_INSTANTIATION (type)
> +	       && CLASSTYPE_INTERFACE_KNOWN (type)
> +	       && CLASSTYPE_INTERFACE_ONLY (type)))
> +	{
> +	  CLASSTYPE_INTERFACE_ONLY (type) = false;
> +	  CLASSTYPE_INTERFACE_UNKNOWN (type) = true;
> +	}
> +
>         /* Add to specialization tables now that constraints etc are
>   	 added.  */
>         if (mk == MK_partial)
> @@ -12070,15 +12083,6 @@ trees_in::read_class_def (tree defn, tree maybe_template)
>     bool installing = maybe_dup && !TYPE_SIZE (type);
>     if (installing)
>       {
> -      if (DECL_EXTERNAL (defn) && TYPE_LANG_SPECIFIC (type))
> -	{
> -	  /* We don't deal with not-really-extern, because, for a
> -	     module you want the import to be the interface, and for a
> -	     header-unit, you're doing it wrong.  */
> -	  CLASSTYPE_INTERFACE_UNKNOWN (type) = false;
> -	  CLASSTYPE_INTERFACE_ONLY (type) = true;
> -	}
> -
>         if (maybe_dup != defn)
>   	{
>   	  // FIXME: This is needed on other defns too, almost
> diff --git a/gcc/testsuite/g++.dg/modules/debug-2_a.C b/gcc/testsuite/g++.dg/modules/debug-2_a.C
> new file mode 100644
> index 00000000000..eed0905542b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/debug-2_a.C
> @@ -0,0 +1,9 @@
> +// PR c++/112820
> +// { dg-additional-options "-fmodules-ts -g" }
> +// { dg-module-cmi io }
> +
> +export module io;
> +
> +export struct error {
> +  virtual const char* what() const noexcept;
> +};
> diff --git a/gcc/testsuite/g++.dg/modules/debug-2_b.C b/gcc/testsuite/g++.dg/modules/debug-2_b.C
> new file mode 100644
> index 00000000000..fc9afbc02e0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/debug-2_b.C
> @@ -0,0 +1,8 @@
> +// PR c++/112820
> +// { dg-additional-options "-fmodules-ts -g" }
> +
> +module io;
> +
> +const char* error::what() const noexcept {
> +  return "bla";
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/debug-2_c.C b/gcc/testsuite/g++.dg/modules/debug-2_c.C
> new file mode 100644
> index 00000000000..37117f69dcd
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/debug-2_c.C
> @@ -0,0 +1,9 @@
> +// PR c++/112820
> +// { dg-module-do link }
> +// { dg-additional-options "-fmodules-ts -g" }
> +
> +import io;
> +
> +int main() {
> +  error{};
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/debug-3_a.C b/gcc/testsuite/g++.dg/modules/debug-3_a.C
> new file mode 100644
> index 00000000000..9e33d8260fd
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/debug-3_a.C
> @@ -0,0 +1,8 @@
> +// PR c++/102607
> +// { dg-additional-options "-fmodules-ts -g" }
> +// { dg-module-cmi mod }
> +
> +export module mod;
> +export struct B {
> +  virtual ~B() = default;
> +};
> diff --git a/gcc/testsuite/g++.dg/modules/debug-3_b.C b/gcc/testsuite/g++.dg/modules/debug-3_b.C
> new file mode 100644
> index 00000000000..03c78b71b5d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/debug-3_b.C
> @@ -0,0 +1,9 @@
> +// PR c++/102607
> +// { dg-module-do link }
> +// { dg-additional-options "-fmodules-ts -g" }
> +
> +import mod;
> +int main() {
> +  struct D : B {};
> +  (void)D{};
> +}