public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH RFC] c++/modules: __class_type_info and modules
@ 2023-12-18 21:31 Jason Merrill
  2023-12-18 21:57 ` Nathan Sidwell
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Merrill @ 2023-12-18 21:31 UTC (permalink / raw)
  To: gcc-patches, Nathan Sidwell; +Cc: Nathaniel Shead, Patrick Palka

Tested x86_64-pc-linux-gnu.  Does this make sense?  Did you have another theory
about how to merge these?

-- 8< --

Doing a dynamic_cast in both TUs broke because we were declaring a new
__class_type_info in _b that conflicted with the one imported in the global
module from _a.  lookup_elaborated_type has a comment that we probably don't
want to find such imports in general, but in this case it seems necessary to
make the artificial lazy declarations of RTTI types work.

gcc/cp/ChangeLog:

	* name-lookup.cc (lookup_elaborated_type): Look for bindings
	in the global namespace in the ABI namespace.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/pr106304_b.C: Add dynamic_cast.
---
 gcc/cp/name-lookup.cc                     | 10 ++++++++++
 gcc/testsuite/g++.dg/modules/pr106304_b.C |  1 +
 2 files changed, 11 insertions(+)

diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
index 09dc6ef3e5a..f15b338025d 100644
--- a/gcc/cp/name-lookup.cc
+++ b/gcc/cp/name-lookup.cc
@@ -8092,6 +8092,16 @@ lookup_elaborated_type (tree name, TAG_how how)
 	      // FIXME: This isn't quite right, if we find something
 	      // here, from the language PoV we're not supposed to
 	      // know it?
+	      // We at least need to do this in __cxxabiv1 to unify lazy
+	      // declarations of __class_type_info in build_dynamic_cast_1.
+	      if (current_namespace == abi_node)
+		{
+		  tree g = (BINDING_VECTOR_CLUSTER (*slot, 0)
+			    .slots[BINDING_SLOT_GLOBAL]);
+		  for (ovl_iterator iter (g); iter; ++iter)
+		    if (qualify_lookup (*iter, LOOK_want::TYPE))
+		      return *iter;
+		}
 	    }
 	}
     }
diff --git a/gcc/testsuite/g++.dg/modules/pr106304_b.C b/gcc/testsuite/g++.dg/modules/pr106304_b.C
index e8333909c8d..0d1da086176 100644
--- a/gcc/testsuite/g++.dg/modules/pr106304_b.C
+++ b/gcc/testsuite/g++.dg/modules/pr106304_b.C
@@ -5,4 +5,5 @@ module pr106304;
 
 void f(A& a) {
   as_b(a);
+  dynamic_cast<B*>(&a);
 }

base-commit: 5347263b347d02e875879ca40ca6e289ac178919
prerequisite-patch-id: 66735c0c7beb22586ed4b632d10ec9094bb9920c
-- 
2.39.3


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC] c++/modules: __class_type_info and modules
  2023-12-18 21:31 [PATCH RFC] c++/modules: __class_type_info and modules Jason Merrill
@ 2023-12-18 21:57 ` Nathan Sidwell
  2023-12-18 22:10   ` Jason Merrill
  0 siblings, 1 reply; 6+ messages in thread
From: Nathan Sidwell @ 2023-12-18 21:57 UTC (permalink / raw)
  To: Jason Merrill, gcc-patches; +Cc: Nathaniel Shead, Patrick Palka

On 12/18/23 16:31, Jason Merrill wrote:
> Tested x86_64-pc-linux-gnu.  Does this make sense?  Did you have another theory
> about how to merge these?

Why isn't push_abi_namespace doing the right setup here? (and I think 
get_global_binding might be similarly problematic?)

nathan

> 
> -- 8< --
> 
> Doing a dynamic_cast in both TUs broke because we were declaring a new
> __class_type_info in _b that conflicted with the one imported in the global
> module from _a.  lookup_elaborated_type has a comment that we probably don't
> want to find such imports in general, but in this case it seems necessary to
> make the artificial lazy declarations of RTTI types work.
> 
> gcc/cp/ChangeLog:
> 
> 	* name-lookup.cc (lookup_elaborated_type): Look for bindings
> 	in the global namespace in the ABI namespace.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/pr106304_b.C: Add dynamic_cast.
> ---
>   gcc/cp/name-lookup.cc                     | 10 ++++++++++
>   gcc/testsuite/g++.dg/modules/pr106304_b.C |  1 +
>   2 files changed, 11 insertions(+)
> 
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index 09dc6ef3e5a..f15b338025d 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -8092,6 +8092,16 @@ lookup_elaborated_type (tree name, TAG_how how)
>   	      // FIXME: This isn't quite right, if we find something
>   	      // here, from the language PoV we're not supposed to
>   	      // know it?
> +	      // We at least need to do this in __cxxabiv1 to unify lazy
> +	      // declarations of __class_type_info in build_dynamic_cast_1.
> +	      if (current_namespace == abi_node)
> +		{
> +		  tree g = (BINDING_VECTOR_CLUSTER (*slot, 0)
> +			    .slots[BINDING_SLOT_GLOBAL]);
> +		  for (ovl_iterator iter (g); iter; ++iter)
> +		    if (qualify_lookup (*iter, LOOK_want::TYPE))
> +		      return *iter;
> +		}
>   	    }
>   	}
>       }
> diff --git a/gcc/testsuite/g++.dg/modules/pr106304_b.C b/gcc/testsuite/g++.dg/modules/pr106304_b.C
> index e8333909c8d..0d1da086176 100644
> --- a/gcc/testsuite/g++.dg/modules/pr106304_b.C
> +++ b/gcc/testsuite/g++.dg/modules/pr106304_b.C
> @@ -5,4 +5,5 @@ module pr106304;
>   
>   void f(A& a) {
>     as_b(a);
> +  dynamic_cast<B*>(&a);
>   }
> 
> base-commit: 5347263b347d02e875879ca40ca6e289ac178919
> prerequisite-patch-id: 66735c0c7beb22586ed4b632d10ec9094bb9920c

-- 
Nathan Sidwell


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC] c++/modules: __class_type_info and modules
  2023-12-18 21:57 ` Nathan Sidwell
@ 2023-12-18 22:10   ` Jason Merrill
  2023-12-23 19:46     ` Nathan Sidwell
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Merrill @ 2023-12-18 22:10 UTC (permalink / raw)
  To: Nathan Sidwell, gcc-patches; +Cc: Nathaniel Shead, Patrick Palka

On 12/18/23 16:57, Nathan Sidwell wrote:
> On 12/18/23 16:31, Jason Merrill wrote:
>> Tested x86_64-pc-linux-gnu.  Does this make sense?  Did you have 
>> another theory
>> about how to merge these?
> 
> Why isn't push_abi_namespace doing the right setup here? (and I think 
> get_global_binding might be similarly problematic?)

What would the right setup be?  It pushes into the global module, but 
before this change lookup doesn't find things imported into the global 
module, and so we get two independent (and so non-equivalent) declarations.

The comment for get_namespace_binding says "Users of this who, having 
found nothing, push a new decl must be prepared for that pushing to 
match an existing decl."  But if lookup_elaborated_type fails, so we 
pushtag a new type, check_module_override doesn't try to merge them 
because TREE_PUBLIC isn't set on the TYPE_DECL yet at that point, and 
they coexist until we complain about redeclaring __dynamic_cast with 
non-matching parameter types.

I tried setting TREE_PUBLIC on the TYPE_DECL, and then 
check_module_override called duplicate_decls, and rejected the 
redeclaration as a different type.

>> -- 8< --
>>
>> Doing a dynamic_cast in both TUs broke because we were declaring a new
>> __class_type_info in _b that conflicted with the one imported in the 
>> global
>> module from _a.  lookup_elaborated_type has a comment that we probably 
>> don't
>> want to find such imports in general, but in this case it seems 
>> necessary to
>> make the artificial lazy declarations of RTTI types work.
>>
>> gcc/cp/ChangeLog:
>>
>>     * name-lookup.cc (lookup_elaborated_type): Look for bindings
>>     in the global namespace in the ABI namespace.
>>
>> gcc/testsuite/ChangeLog:
>>
>>     * g++.dg/modules/pr106304_b.C: Add dynamic_cast.
>> ---
>>   gcc/cp/name-lookup.cc                     | 10 ++++++++++
>>   gcc/testsuite/g++.dg/modules/pr106304_b.C |  1 +
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
>> index 09dc6ef3e5a..f15b338025d 100644
>> --- a/gcc/cp/name-lookup.cc
>> +++ b/gcc/cp/name-lookup.cc
>> @@ -8092,6 +8092,16 @@ lookup_elaborated_type (tree name, TAG_how how)
>>             // FIXME: This isn't quite right, if we find something
>>             // here, from the language PoV we're not supposed to
>>             // know it?
>> +          // We at least need to do this in __cxxabiv1 to unify lazy
>> +          // declarations of __class_type_info in build_dynamic_cast_1.
>> +          if (current_namespace == abi_node)
>> +        {
>> +          tree g = (BINDING_VECTOR_CLUSTER (*slot, 0)
>> +                .slots[BINDING_SLOT_GLOBAL]);
>> +          for (ovl_iterator iter (g); iter; ++iter)
>> +            if (qualify_lookup (*iter, LOOK_want::TYPE))
>> +              return *iter;
>> +        }
>>           }
>>       }
>>       }
>> diff --git a/gcc/testsuite/g++.dg/modules/pr106304_b.C 
>> b/gcc/testsuite/g++.dg/modules/pr106304_b.C
>> index e8333909c8d..0d1da086176 100644
>> --- a/gcc/testsuite/g++.dg/modules/pr106304_b.C
>> +++ b/gcc/testsuite/g++.dg/modules/pr106304_b.C
>> @@ -5,4 +5,5 @@ module pr106304;
>>   void f(A& a) {
>>     as_b(a);
>> +  dynamic_cast<B*>(&a);
>>   }
>>
>> base-commit: 5347263b347d02e875879ca40ca6e289ac178919
>> prerequisite-patch-id: 66735c0c7beb22586ed4b632d10ec9094bb9920c
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC] c++/modules: __class_type_info and modules
  2023-12-18 22:10   ` Jason Merrill
@ 2023-12-23 19:46     ` Nathan Sidwell
  2024-01-12 14:09       ` Jason Merrill
  0 siblings, 1 reply; 6+ messages in thread
From: Nathan Sidwell @ 2023-12-23 19:46 UTC (permalink / raw)
  To: Jason Merrill, gcc-patches; +Cc: Nathaniel Shead, Patrick Palka

On 12/18/23 17:10, Jason Merrill wrote:
> On 12/18/23 16:57, Nathan Sidwell wrote:
>> On 12/18/23 16:31, Jason Merrill wrote:
>>> Tested x86_64-pc-linux-gnu.  Does this make sense?  Did you have another theory
>>> about how to merge these?
>>
>> Why isn't push_abi_namespace doing the right setup here? (and I think 
>> get_global_binding might be similarly problematic?)
> 
> What would the right setup be?  It pushes into the global module, but before 
> this change lookup doesn't find things imported into the global module, and so 
> we get two independent (and so non-equivalent) declarations.
> 
> The comment for get_namespace_binding says "Users of this who, having found 
> nothing, push a new decl must be prepared for that pushing to match an existing 
> decl."  But if lookup_elaborated_type fails, so we pushtag a new type, 
> check_module_override doesn't try to merge them because TREE_PUBLIC isn't set on 
> the TYPE_DECL yet at that point, and they coexist until we complain about 
> redeclaring __dynamic_cast with non-matching parameter types.
> 
> I tried setting TREE_PUBLIC on the TYPE_DECL, and then check_module_override 
> called duplicate_decls, and rejected the redeclaration as a different type.

sigh, it seems that doesn't work as intended, I guess your approace is a 
pragmatic workaround, much as I dislike special-casing particular identifier. 
Perhaps comment with an appropriate FIXME?

I've realized there's problems with completeness here -- the 'invisible' type 
may be complete, but the current TU only foreward-declares it.  Our AST can't 
represent that right now.  And I'm not sure if there are template instantiation 
issues -- is the type complete or not in any particular instantiaton?

nathan

> 
>>> -- 8< --
>>>
>>> Doing a dynamic_cast in both TUs broke because we were declaring a new
>>> __class_type_info in _b that conflicted with the one imported in the global
>>> module from _a.  lookup_elaborated_type has a comment that we probably don't
>>> want to find such imports in general, but in this case it seems necessary to
>>> make the artificial lazy declarations of RTTI types work.
>>>
>>> gcc/cp/ChangeLog:
>>>
>>>     * name-lookup.cc (lookup_elaborated_type): Look for bindings
>>>     in the global namespace in the ABI namespace.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>     * g++.dg/modules/pr106304_b.C: Add dynamic_cast.
>>> ---
>>>   gcc/cp/name-lookup.cc                     | 10 ++++++++++
>>>   gcc/testsuite/g++.dg/modules/pr106304_b.C |  1 +
>>>   2 files changed, 11 insertions(+)
>>>
>>> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
>>> index 09dc6ef3e5a..f15b338025d 100644
>>> --- a/gcc/cp/name-lookup.cc
>>> +++ b/gcc/cp/name-lookup.cc
>>> @@ -8092,6 +8092,16 @@ lookup_elaborated_type (tree name, TAG_how how)
>>>             // FIXME: This isn't quite right, if we find something
>>>             // here, from the language PoV we're not supposed to
>>>             // know it?
>>> +          // We at least need to do this in __cxxabiv1 to unify lazy
>>> +          // declarations of __class_type_info in build_dynamic_cast_1.
>>> +          if (current_namespace == abi_node)
>>> +        {
>>> +          tree g = (BINDING_VECTOR_CLUSTER (*slot, 0)
>>> +                .slots[BINDING_SLOT_GLOBAL]);
>>> +          for (ovl_iterator iter (g); iter; ++iter)
>>> +            if (qualify_lookup (*iter, LOOK_want::TYPE))
>>> +              return *iter;
>>> +        }
>>>           }
>>>       }
>>>       }
>>> diff --git a/gcc/testsuite/g++.dg/modules/pr106304_b.C 
>>> b/gcc/testsuite/g++.dg/modules/pr106304_b.C
>>> index e8333909c8d..0d1da086176 100644
>>> --- a/gcc/testsuite/g++.dg/modules/pr106304_b.C
>>> +++ b/gcc/testsuite/g++.dg/modules/pr106304_b.C
>>> @@ -5,4 +5,5 @@ module pr106304;
>>>   void f(A& a) {
>>>     as_b(a);
>>> +  dynamic_cast<B*>(&a);
>>>   }
>>>
>>> base-commit: 5347263b347d02e875879ca40ca6e289ac178919
>>> prerequisite-patch-id: 66735c0c7beb22586ed4b632d10ec9094bb9920c
>>
> 

-- 
Nathan Sidwell


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC] c++/modules: __class_type_info and modules
  2023-12-23 19:46     ` Nathan Sidwell
@ 2024-01-12 14:09       ` Jason Merrill
  2024-01-12 17:16         ` Jason Merrill
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Merrill @ 2024-01-12 14:09 UTC (permalink / raw)
  To: Nathan Sidwell, gcc-patches; +Cc: Nathaniel Shead, Patrick Palka

[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]

On 12/23/23 14:46, Nathan Sidwell wrote:
> On 12/18/23 17:10, Jason Merrill wrote:
>> On 12/18/23 16:57, Nathan Sidwell wrote:
>>> On 12/18/23 16:31, Jason Merrill wrote:
>>>> Tested x86_64-pc-linux-gnu.  Does this make sense?  Did you have 
>>>> another theory
>>>> about how to merge these?
>>>
>>> Why isn't push_abi_namespace doing the right setup here? (and I think 
>>> get_global_binding might be similarly problematic?)
>>
>> What would the right setup be?  It pushes into the global module, but 
>> before this change lookup doesn't find things imported into the global 
>> module, and so we get two independent (and so non-equivalent) 
>> declarations.
>>
>> The comment for get_namespace_binding says "Users of this who, having 
>> found nothing, push a new decl must be prepared for that pushing to 
>> match an existing decl."  But if lookup_elaborated_type fails, so we 
>> pushtag a new type, check_module_override doesn't try to merge them 
>> because TREE_PUBLIC isn't set on the TYPE_DECL yet at that point, and 
>> they coexist until we complain about redeclaring __dynamic_cast with 
>> non-matching parameter types.
>>
>> I tried setting TREE_PUBLIC on the TYPE_DECL, and then 
>> check_module_override called duplicate_decls, and rejected the 
>> redeclaration as a different type.
> 
> sigh, it seems that doesn't work as intended, I guess your approace is a 
> pragmatic workaround, much as I dislike special-casing particular 
> identifier. Perhaps comment with an appropriate FIXME?
>
> I've realized there's problems with completeness here -- the 'invisible' 
> type may be complete, but the current TU only forward-declares it.  Our 
> AST can't represent that right now.  And I'm not sure if there are 
> template instantiation issues -- is the type complete or not in any 
> particular instantiaton?

My understanding of https://eel.is/c++draft/module#reach-4 is that this 
doesn't matter: if there is a reachable definition of the class, the 
class is complete, even if the current TU only forward-declares it.

Here's an alternate approach that handles this merging in 
check_module_override; this makes P1811 include-after-import a bit 
worse, but it's already not well supported, so perhaps that's OK for 
now.  But I'm inclined to go with my earlier patch for GCC 14.  What do 
you think?

Jason

[-- Attachment #2: 0001-c-__class_type_info-and-modules.patch --]
[-- Type: text/x-patch, Size: 4657 bytes --]

From a4ccd4664d6acb696db3263de8286721e75a0d2b Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
Date: Mon, 18 Dec 2023 15:47:10 -0500
Subject: [PATCH 1/2] c++: __class_type_info and modules
To: gcc-patches@gcc.gnu.org

Doing a dynamic_cast in both TUs broke because we were declaring a new
__class_type_info in _b that conflicted with the one imported in the global
module from _a.  check_module_override wasn't merging them because
TREE_PUBLIC wasn't set yet.  Fixing that led to errors from duplicate_decls
about the decls having different types; let's avoid that in
check_module_override.

gcc/cp/ChangeLog:

	* name-lookup.cc (pushtag): Set TREE_PUBLIC sooner.
	(check_module_override): Merge classes.
  	(lookup_elaborated_type): Update comment.

gcc/testsuite/ChangeLog:

	* g++.dg/lookup/builtin4.C: Expect warning.
	* g++.dg/lookup/hidden-class10.C: Likewise.
	* g++.dg/modules/pr106304_b.C: Add dynamic_cast.
---
 gcc/cp/name-lookup.cc                        | 33 ++++++++++++++++----
 gcc/testsuite/g++.dg/lookup/builtin4.C       |  2 +-
 gcc/testsuite/g++.dg/lookup/hidden-class10.C |  2 +-
 gcc/testsuite/g++.dg/modules/pr106304_b.C    |  1 +
 4 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
index 4e2d5b03015..37724ea0e65 100644
--- a/gcc/cp/name-lookup.cc
+++ b/gcc/cp/name-lookup.cc
@@ -3794,7 +3794,22 @@ check_module_override (tree decl, tree mvec, bool hiding,
 
       for (ovl_iterator iter (mergeable); iter; ++iter)
 	{
-	  match = duplicate_decls (decl, *iter, hiding);
+	  if (!named_module_p ()
+	      && DECL_IMPLICIT_TYPEDEF_P (decl)
+	      && DECL_IMPLICIT_TYPEDEF_P (*iter)
+	      && !IDENTIFIER_ANON_P (DECL_NAME (decl)))
+	    /* Merge classes of the same name in the global module;
+	       duplicate_decls would complain about different types.  This test
+	       is similar to the one in check_mergeable_decl, but only
+	       considers implicit typedefs, since explicit typedefs should
+	       actually have the same type.
+
+	       ??? If we eventually want to do consistency checking for P1811
+	       redefinition, we'll probably need to delay this merging until
+	       the end of the class definition.  */
+	    match = *iter;
+	  else
+	    match = duplicate_decls (decl, *iter, hiding);
 	  if (match)
 	    goto matched;
 	}
@@ -8087,11 +8102,9 @@ lookup_elaborated_type (tree name, TAG_how how)
 
 	  if (!module_purview_p ())
 	    {
-	      /* We're in the global module, perhaps there's a tag
-		 there?  */
-	      // FIXME: This isn't quite right, if we find something
-	      // here, from the language PoV we're not supposed to
-	      // know it?
+	      /* We're in the global module, perhaps there's a tag there?
+		 We'll find it in check_module_override when we try to push a
+		 new one, no need to handle it specially here.  */
 	    }
 	}
     }
@@ -8277,10 +8290,18 @@ pushtag (tree name, tree type, TAG_how how)
 	}
       else
 	{
+	  /* Set TREE_PUBLIC now for built-in warnings and module merging.  */
+	  if (b->kind == sk_namespace
+	      && TREE_PUBLIC (b->this_entity))
+	    TREE_PUBLIC (decl) = 1;
+
 	  decl = do_pushdecl_with_scope
 	    (decl, b, /*hiding=*/(how == TAG_how::HIDDEN_FRIEND));
 	  if (decl == error_mark_node)
 	    return decl;
+	  if (TREE_TYPE (decl) != type)
+	    /* Found an imported version of the same type.  */
+	    return TREE_TYPE (decl);
 
 	  if (DECL_CONTEXT (decl) == std_node
 	      && init_list_identifier == DECL_NAME (TYPE_NAME (type))
diff --git a/gcc/testsuite/g++.dg/lookup/builtin4.C b/gcc/testsuite/g++.dg/lookup/builtin4.C
index b1785dcc7fb..e71131b5a99 100644
--- a/gcc/testsuite/g++.dg/lookup/builtin4.C
+++ b/gcc/testsuite/g++.dg/lookup/builtin4.C
@@ -10,6 +10,6 @@ namespace std
   union abort;
 }
 
-union abort;
+union abort;			// { dg-warning "built-in" }
 
 using std::abort; // { dg-error "" }
diff --git a/gcc/testsuite/g++.dg/lookup/hidden-class10.C b/gcc/testsuite/g++.dg/lookup/hidden-class10.C
index c9b5ca9f663..986241558a6 100644
--- a/gcc/testsuite/g++.dg/lookup/hidden-class10.C
+++ b/gcc/testsuite/g++.dg/lookup/hidden-class10.C
@@ -6,6 +6,6 @@
 // function name.
 
 class A {
-  friend class abort;
+  friend class abort;		// { dg-warning "built-in" }
   abort *b;	// { dg-error "type|expected" }
 };
diff --git a/gcc/testsuite/g++.dg/modules/pr106304_b.C b/gcc/testsuite/g++.dg/modules/pr106304_b.C
index e8333909c8d..0d1da086176 100644
--- a/gcc/testsuite/g++.dg/modules/pr106304_b.C
+++ b/gcc/testsuite/g++.dg/modules/pr106304_b.C
@@ -5,4 +5,5 @@ module pr106304;
 
 void f(A& a) {
   as_b(a);
+  dynamic_cast<B*>(&a);
 }
-- 
2.39.3


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC] c++/modules: __class_type_info and modules
  2024-01-12 14:09       ` Jason Merrill
@ 2024-01-12 17:16         ` Jason Merrill
  0 siblings, 0 replies; 6+ messages in thread
From: Jason Merrill @ 2024-01-12 17:16 UTC (permalink / raw)
  To: Nathan Sidwell, gcc-patches; +Cc: Nathaniel Shead, Patrick Palka

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

On 1/12/24 09:09, Jason Merrill wrote:
> On 12/23/23 14:46, Nathan Sidwell wrote:
>> On 12/18/23 17:10, Jason Merrill wrote:
>>> On 12/18/23 16:57, Nathan Sidwell wrote:
>>>> On 12/18/23 16:31, Jason Merrill wrote:
>>>>> Tested x86_64-pc-linux-gnu.  Does this make sense?  Did you have 
>>>>> another theory
>>>>> about how to merge these?
>>>>
>>>> Why isn't push_abi_namespace doing the right setup here? (and I 
>>>> think get_global_binding might be similarly problematic?)
>>>
>>> What would the right setup be?  It pushes into the global module, but 
>>> before this change lookup doesn't find things imported into the 
>>> global module, and so we get two independent (and so non-equivalent) 
>>> declarations.
>>>
>>> The comment for get_namespace_binding says "Users of this who, having 
>>> found nothing, push a new decl must be prepared for that pushing to 
>>> match an existing decl."  But if lookup_elaborated_type fails, so we 
>>> pushtag a new type, check_module_override doesn't try to merge them 
>>> because TREE_PUBLIC isn't set on the TYPE_DECL yet at that point, and 
>>> they coexist until we complain about redeclaring __dynamic_cast with 
>>> non-matching parameter types.
>>>
>>> I tried setting TREE_PUBLIC on the TYPE_DECL, and then 
>>> check_module_override called duplicate_decls, and rejected the 
>>> redeclaration as a different type.
>>
>> sigh, it seems that doesn't work as intended, I guess your approace is 
>> a pragmatic workaround, much as I dislike special-casing particular 
>> identifier. Perhaps comment with an appropriate FIXME?
>>
>> I've realized there's problems with completeness here -- the 
>> 'invisible' type may be complete, but the current TU only 
>> forward-declares it.  Our AST can't represent that right now.  And I'm 
>> not sure if there are template instantiation issues -- is the type 
>> complete or not in any particular instantiaton?
> 
> My understanding of https://eel.is/c++draft/module#reach-4 is that this 
> doesn't matter: if there is a reachable definition of the class, the 
> class is complete, even if the current TU only forward-declares it.
> 
> Here's an alternate approach that handles this merging in 
> check_module_override; this makes P1811 include-after-import a bit 
> worse, but it's already not well supported, so perhaps that's OK for 
> now.  But I'm inclined to go with my earlier patch for GCC 14.  What do 
> you think?

I'm going to go ahead and push this revision of my earlier patch for 
now, we can adjust as needed.

[-- Attachment #2: 0001-c-__class_type_info-and-modules-PR113038.patch --]
[-- Type: text/x-patch, Size: 2290 bytes --]

From 27521a2f4f7b859d5656e5bdd69d3f759ea4c23a Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
Date: Mon, 18 Dec 2023 15:47:10 -0500
Subject: [PATCH] c++: __class_type_info and modules [PR113038]
To: gcc-patches@gcc.gnu.org

Doing a dynamic_cast in both TUs broke because we were declaring a new
__class_type_info in _b that conflicted with the one imported in the global
module from _a.  It seems clear to me that any new class declaration in
the global module should merge with an imported definition, but for GCC 14
let's just fix this for the specific case of __class_type_info.

	PR c++/113038

gcc/cp/ChangeLog:

	* name-lookup.cc (lookup_elaborated_type): Look for bindings
	in the global namespace in the ABI namespace.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/pr106304_b.C: Add dynamic_cast.
---
 gcc/cp/name-lookup.cc                     | 16 +++++++++++++---
 gcc/testsuite/g++.dg/modules/pr106304_b.C |  1 +
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
index 26c6bc71e99..d827d337d3b 100644
--- a/gcc/cp/name-lookup.cc
+++ b/gcc/cp/name-lookup.cc
@@ -8089,9 +8089,19 @@ lookup_elaborated_type (tree name, TAG_how how)
 	    {
 	      /* We're in the global module, perhaps there's a tag
 		 there?  */
-	      // FIXME: This isn't quite right, if we find something
-	      // here, from the language PoV we're not supposed to
-	      // know it?
+
+	      /* FIXME: In general we should probably merge global module
+		 classes in check_module_override rather than here, but for
+		 GCC14 let's just fix lazy declarations of __class_type_info in
+		 build_dynamic_cast_1.  */
+	      if (current_namespace == abi_node)
+		{
+		  tree g = (BINDING_VECTOR_CLUSTER (*slot, 0)
+			    .slots[BINDING_SLOT_GLOBAL]);
+		  for (ovl_iterator iter (g); iter; ++iter)
+		    if (qualify_lookup (*iter, LOOK_want::TYPE))
+		      return *iter;
+		}
 	    }
 	}
     }
diff --git a/gcc/testsuite/g++.dg/modules/pr106304_b.C b/gcc/testsuite/g++.dg/modules/pr106304_b.C
index e8333909c8d..0d1da086176 100644
--- a/gcc/testsuite/g++.dg/modules/pr106304_b.C
+++ b/gcc/testsuite/g++.dg/modules/pr106304_b.C
@@ -5,4 +5,5 @@ module pr106304;
 
 void f(A& a) {
   as_b(a);
+  dynamic_cast<B*>(&a);
 }
-- 
2.39.3


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-01-12 17:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-18 21:31 [PATCH RFC] c++/modules: __class_type_info and modules Jason Merrill
2023-12-18 21:57 ` Nathan Sidwell
2023-12-18 22:10   ` Jason Merrill
2023-12-23 19:46     ` Nathan Sidwell
2024-01-12 14:09       ` Jason Merrill
2024-01-12 17:16         ` Jason Merrill

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).