[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 edited 
https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

Before looking into it, I remember the user branch is only allowed for stacked 
review. If this patch is not the case, let's try to create a new PR.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

Hmm I have been doing user branches for a while, this saves me time in case I 
want to split the patch off in a separate MR.
It's already the case that this is split into test adding + fixing commits.

In any case, the user branch will be deleted as soon as this is merged.

Can we go ahead, and clarify the rules for the next time, since this is such a 
small patch and windows CI takes 3 hours?

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -181,6 +181,9 @@ Bug Fixes to C++ Support
   and (`#79745 `_)
 - Fix incorrect code generation caused by the object argument of ``static 
operator()`` and ``static operator[]`` calls not being evaluated.
   Fixes (`#67976 `_)
+- Fix incorrect merging of modules which contain using declarations which 
shadow
+  other declarations. This could manifest as ODR checker false positives.
+  Fixes (`#TODO `_)

ChuanqiXu9 wrote:

Oh, if it is the case, we should mark this as draft. Otherwise, if I don't look 
into every line and I see your funcitonal change is pretty simple, it'll be bad 
if I merge it directly...

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

@@ -181,6 +181,9 @@ Bug Fixes to C++ Support
   and (`#79745 `_)
 - Fix incorrect code generation caused by the object argument of ``static 
operator()`` and ``static operator[]`` calls not being evaluated.
   Fixes (`#67976 `_)
+- Fix incorrect merging of modules which contain using declarations which 
shadow
+  other declarations. This could manifest as ODR checker false positives.
+  Fixes (`#TODO `_)

mizvekov wrote:

We usually do small fixups like this during review / post approval, this will 
not be merged like this, don't worry.
People on the LLVM community tend to be quite flexible on these things.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 edited 
https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = baz::foo;
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl 0x[[BAZ_REDECL_ADDR:[^ ]*]] prev 0x[[BAZ_ADDR:[^ 
]*]] <{{.*}}> line:{{.*}} imported in A. hidden  baz
+// CHECK-NEXT: |-original Namespace 0x[[BAZ_ADDR]] 'baz'
+// CHECK-NEXT: |-TypeAliasDecl 0x[[ALIAS_REDECL_ADDR:[^ ]*]] prev 
0x[[ALIAS_ADDR:[^ ]*]] <{{.*}}> col:{{.*}} imported in A. hidden foo 
'char'
+// CHECK-NEXT: | `-BuiltinType 0x{{[^ ]*}} 'char'
+// CHECK-NEXT: |-UsingDecl 0x{{[^ ]*}} first 0x[[USING_ADDR:[^ ]*]] <{{.*}}> 
col:{{.*}} imported in A. hidden baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace 0x[[BAZ_REDECL_ADDR]] 'baz'
+// CHECK-NEXT: `-UsingShadowDecl 0x{{[^ ]*}} prev 0x[[SHADOW_ADDR:[^ ]*]] 
<{{.*}}> col:{{.*}} imported in A. hidden implicit TypeAlias 
0x[[ALIAS_REDECL_ADDR]] 'foo'

ChuanqiXu9 wrote:

Let's avoid checking dumpped text as much as possible. This feels bad if we 
need to update them.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

IIRC, in https://github.com/llvm/llvm-project/pull/79959, you're saying the 
patch have the potential to fix 
https://github.com/llvm/llvm-project/issues/78850? Is it true?

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -181,6 +181,9 @@ Bug Fixes to C++ Support
   and (`#79745 `_)
 - Fix incorrect code generation caused by the object argument of ``static 
operator()`` and ``static operator[]`` calls not being evaluated.
   Fixes (`#67976 `_)
+- Fix incorrect merging of modules which contain using declarations which 
shadow
+  other declarations. This could manifest as ODR checker false positives.
+  Fixes (`#TODO `_)

ChuanqiXu9 wrote:

Let's file a real issue.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -181,6 +181,9 @@ Bug Fixes to C++ Support
   and (`#79745 `_)
 - Fix incorrect code generation caused by the object argument of ``static 
operator()`` and ``static operator[]`` calls not being evaluated.
   Fixes (`#67976 `_)
+- Fix incorrect merging of modules which contain using declarations which 
shadow
+  other declarations. This could manifest as ODR checker false positives.
+  Fixes (`#TODO `_)

ChuanqiXu9 wrote:

> We usually do small fixups like this during review / post approval, this will 
> not be merged like this, don't worry.
People on the LLVM community tend to be quite flexible on these things.

Yeah, but we still need to a real issue in the end of the day, right?

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -181,6 +181,9 @@ Bug Fixes to C++ Support
   and (`#79745 `_)
 - Fix incorrect code generation caused by the object argument of ``static 
operator()`` and ``static operator[]`` calls not being evaluated.
   Fixes (`#67976 `_)
+- Fix incorrect merging of modules which contain using declarations which 
shadow
+  other declarations. This could manifest as ODR checker false positives.
+  Fixes (`#TODO `_)

ChuanqiXu9 wrote:

(I don't know why the comment got duplicated)

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -181,6 +181,9 @@ Bug Fixes to C++ Support
   and (`#79745 `_)
 - Fix incorrect code generation caused by the object argument of ``static 
operator()`` and ``static operator[]`` calls not being evaluated.
   Fixes (`#67976 `_)
+- Fix incorrect merging of modules which contain using declarations which 
shadow
+  other declarations. This could manifest as ODR checker false positives.
+  Fixes (`#TODO `_)

ChuanqiXu9 wrote:

For example, I need to what's the behavior before the patch, then I can have a 
better feeling about how should we test it.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 edited 
https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov edited 
https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov updated 
https://github.com/llvm/llvm-project/pull/80245

>From 23a0d731cfe8593c338fc8ad7920f7aa9270afaa Mon Sep 17 00:00:00 2001
From: Matheus Izvekov 
Date: Thu, 1 Feb 2024 02:07:16 -0300
Subject: [PATCH 1/2] [NFC] [clang] add test for merging of UsingShadowDecl

---
 clang/test/Modules/cxx20-decls.cppm | 44 +
 1 file changed, 44 insertions(+)
 create mode 100644 clang/test/Modules/cxx20-decls.cppm

diff --git a/clang/test/Modules/cxx20-decls.cppm 
b/clang/test/Modules/cxx20-decls.cppm
new file mode 100644
index 0..052c8e73be247
--- /dev/null
+++ b/clang/test/Modules/cxx20-decls.cppm
@@ -0,0 +1,44 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = baz::foo;
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl 0x[[BAZ_REDECL_ADDR:[^ ]*]] prev 0x[[BAZ_ADDR:[^ 
]*]] <{{.*}}> line:{{.*}} imported in A. hidden  baz
+// CHECK-NEXT: |-original Namespace 0x[[BAZ_ADDR]] 'baz'
+// CHECK-NEXT: |-TypeAliasDecl 0x[[ALIAS_REDECL_ADDR:[^ ]*]] prev 
0x[[ALIAS_ADDR:[^ ]*]] <{{.*}}> col:{{.*}} imported in A. hidden foo 
'char'
+// CHECK-NEXT: | `-BuiltinType 0x{{[^ ]*}} 'char'
+// CHECK-NEXT: |-UsingDecl 0x{{[^ ]*}} first 0x[[USING_ADDR:[^ ]*]] <{{.*}}> 
col:{{.*}} imported in A. hidden baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace 0x[[BAZ_REDECL_ADDR]] 'baz'
+// FIXME: UsingShadowDecl should have been merged
+// CHECK-NOT:  `-UsingShadowDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} <{{.*}}> 
col:{{.*}} imported in A. hidden implicit TypeAlias 
0x[[ALIAS_REDECL_ADDR]] 'foo'
+// CHECK-NEXT: `-UsingShadowDecl 0x{{[^ ]*}} <{{.*}}> col:{{.*}} imported in 
A. hidden implicit TypeAlias 0x[[ALIAS_REDECL_ADDR]] 'foo'
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl 0x[[BAZ_ADDR]] <{{.*}}> line:{{.*}} baz
+// CHECK-NEXT: |-TypeAliasDecl 0x[[ALIAS_ADDR]] <{{.*}}> col:{{.*}} referenced 
foo 'char'
+// CHECK-NEXT: | `-BuiltinType 0x{{[^ ]*}} 'char'
+// CHECK-NEXT: |-UsingDecl 0x[[USING_ADDR]] <{{.*}}> col:{{.*}} baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace 0x[[BAZ_ADDR]] 'baz'
+// CHECK-NEXT:  `-UsingShadowDecl 0x[[SHADOW_ADDR:[^ ]*]] <{{.*}}> col:{{.*}} 
implicit TypeAlias 0x[[ALIAS_ADDR]] 'foo'

>From 895c615a354d540c4e4404fa728bfad2a962c7bf Mon Sep 17 00:00:00 2001
From: Matheus Izvekov 
Date: Thu, 1 Feb 2024 02:26:10 -0300
Subject: [PATCH 2/2] [clang] fix merging of UsingShadowDecl

Previously, when deciding if two UsingShadowDecls where mergeable,
we would incorrectly only look for both pointing to the exact redecla
ration, whereas the correct thing is to look for declarations to the
same entity.

This problem has existed as far back as 2013, introduced in commit
fd8634a09de71.

This problem could manifest itself as ODR check false positives
when importing modules.

Fixes: #80252
---
 clang/docs/ReleaseNotes.rst | 3 +++
 clang/test/Modules/cxx20-decls.cppm | 6 ++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b7b11ab3a6b2e..a48672951de32 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -181,6 +181,9 @@ Bug Fixes to C++ Support
   and (`#79745 `_)
 - Fix incorrect code generation caused by the object argument of ``static 
operator()`` and ``static operator[]`` calls not being evaluated.
   Fixes (`#67976 `_)
+- Fix incorrect merging of modules which contain using declarations which 
shadow
+  other declarations. This could manifest as ODR checker false positives.
+  Fixes (`#80252 `_)
 
 Bug Fixes to AST Handling
 ^
diff --git a/clang/test/Modules/cxx20-decls.cppm 
b/clang/test/Modules/cxx20-decls.cppm
index 052c8e73be247..ee9f117278884 100644
--- a/clang/test/Modules/cxx20-decls.cppm
+++ b/clang/test/Modules/cxx20-decls.cppm
@@ -31,9 +31,7 @@ using xxx = baz::foo;
 // CHECK-NEXT: | `-BuiltinType 0x{{[^ ]*}} 'char'
 // CHECK-NEXT: |-UsingDecl 0x{{[^ ]*}} first 0x[[USING_ADDR:[^ ]*]] <{{.*}}> 
col:{{.*}} imported in A. hidden baz::foo
 // CHECK-NEXT: | `-NestedNameSpecifier Namespace 0x[[BAZ_REDECL_ADDR]] 'baz'
-// FIXME: UsingShadowDecl should have been merged
-// CHECK-NOT:  `-UsingShadowDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} <{{.*}}> 
col:{{.*}} imported in A. hidden impl

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

@@ -181,6 +181,9 @@ Bug Fixes to C++ Support
   and (`#79745 `_)
 - Fix incorrect code generation caused by the object argument of ``static 
operator()`` and ``static operator[]`` calls not being evaluated.
   Fixes (`#67976 `_)
+- Fix incorrect merging of modules which contain using declarations which 
shadow
+  other declarations. This could manifest as ODR checker false positives.
+  Fixes (`#TODO `_)

mizvekov wrote:

The MR is split in two commits, the first one adds the test, if you click on 
the last commit, you can see the change in the expectation.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = baz::foo;
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl 0x[[BAZ_REDECL_ADDR:[^ ]*]] prev 0x[[BAZ_ADDR:[^ 
]*]] <{{.*}}> line:{{.*}} imported in A. hidden  baz
+// CHECK-NEXT: |-original Namespace 0x[[BAZ_ADDR]] 'baz'
+// CHECK-NEXT: |-TypeAliasDecl 0x[[ALIAS_REDECL_ADDR:[^ ]*]] prev 
0x[[ALIAS_ADDR:[^ ]*]] <{{.*}}> col:{{.*}} imported in A. hidden foo 
'char'
+// CHECK-NEXT: | `-BuiltinType 0x{{[^ ]*}} 'char'
+// CHECK-NEXT: |-UsingDecl 0x{{[^ ]*}} first 0x[[USING_ADDR:[^ ]*]] <{{.*}}> 
col:{{.*}} imported in A. hidden baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace 0x[[BAZ_REDECL_ADDR]] 'baz'
+// CHECK-NEXT: `-UsingShadowDecl 0x{{[^ ]*}} prev 0x[[SHADOW_ADDR:[^ ]*]] 
<{{.*}}> col:{{.*}} imported in A. hidden implicit TypeAlias 
0x[[ALIAS_REDECL_ADDR]] 'foo'

mizvekov wrote:

While normally I would agree, in this case there are no existing AST tests for 
these other kinds of declarations, so I felt appropriate to broaden this test a 
little bit to cover those.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

The issue is up: https://github.com/llvm/llvm-project/issues/80252

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

> IIRC, in #79959, you're saying the patch have the potential to fix #78850? Is 
> it true?

Yeah, looking at the similarities, there is a decent chance this is the same 
issue, it's worth trying it out.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = baz::foo;
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl 0x[[BAZ_REDECL_ADDR:[^ ]*]] prev 0x[[BAZ_ADDR:[^ 
]*]] <{{.*}}> line:{{.*}} imported in A. hidden  baz
+// CHECK-NEXT: |-original Namespace 0x[[BAZ_ADDR]] 'baz'
+// CHECK-NEXT: |-TypeAliasDecl 0x[[ALIAS_REDECL_ADDR:[^ ]*]] prev 
0x[[ALIAS_ADDR:[^ ]*]] <{{.*}}> col:{{.*}} imported in A. hidden foo 
'char'
+// CHECK-NEXT: | `-BuiltinType 0x{{[^ ]*}} 'char'
+// CHECK-NEXT: |-UsingDecl 0x{{[^ ]*}} first 0x[[USING_ADDR:[^ ]*]] <{{.*}}> 
col:{{.*}} imported in A. hidden baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace 0x[[BAZ_REDECL_ADDR]] 'baz'
+// CHECK-NEXT: `-UsingShadowDecl 0x{{[^ ]*}} prev 0x[[SHADOW_ADDR:[^ ]*]] 
<{{.*}}> col:{{.*}} imported in A. hidden implicit TypeAlias 
0x[[ALIAS_REDECL_ADDR]] 'foo'

ChuanqiXu9 wrote:

In such case, I'd like to introduce unittest. For example 
https://github.com/llvm/llvm-project/blob/main/clang/unittests/Sema/SemaNoloadLookupTest.cpp,
 
https://github.com/llvm/llvm-project/blob/main/clang/unittests/Serialization/VarDeclConstantInitTest.cpp
 and 
https://github.com/llvm/llvm-project/pull/76774/files#diff-b1c26f074f25d8306b5ac4522770a2487a57194176355cf9af17688217573788.

While it may take you 10+ minutes, it will look much prettier than the current 
way.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> The issue is up: #80252

Thanks. Although I feel it is too implementor's view, (I mean the end users can 
hardly understand it), it is much better.

> Yeah, looking at the similarities, there is a decent chance this is the same 
> issue, it's worth trying it out.

Yeah, let's give it a try.


https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov updated 
https://github.com/llvm/llvm-project/pull/80245

>From 8823a7ec248c7220395f53634467aa25f5007206 Mon Sep 17 00:00:00 2001
From: Matheus Izvekov 
Date: Thu, 1 Feb 2024 02:07:16 -0300
Subject: [PATCH 1/2] [NFC] [clang] add tests for merging of UsingShadowDecl

---
 clang/test/Modules/GH80252.cppm | 42 +++
 clang/test/Modules/cxx20-decls.cppm | 44 +
 2 files changed, 86 insertions(+)
 create mode 100644 clang/test/Modules/GH80252.cppm
 create mode 100644 clang/test/Modules/cxx20-decls.cppm

diff --git a/clang/test/Modules/GH80252.cppm b/clang/test/Modules/GH80252.cppm
new file mode 100644
index 0..3212404fbe14c
--- /dev/null
+++ b/clang/test/Modules/GH80252.cppm
@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cppm -emit-module-interface -o 
%t/B.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/C.cpp -fmodule-file=A=%t/A.pcm 
-fmodule-file=B=%t/B.pcm -fsyntax-only -verify
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- bar.h
+class bar {
+  bar(baz::foo);
+};
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+#include "bar.h"
+export module B;
+
+//--- C.cpp
+#include "foo.h"
+import A;
+#include "bar.h"
+import B;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = bar;
+// FIXME: This is a false positive ODR violation.
+// expected-error@bar.h:2 {{'bar' has different definitions in different 
modules; first difference is defined here found constructor with 1st parameter 
of type 'baz::foo' (aka 'char')}}
+// expected-note@bar.h:2 {{but in 'B.' found constructor with 1st 
parameter of type 'baz::foo' (aka 'char')}}
diff --git a/clang/test/Modules/cxx20-decls.cppm 
b/clang/test/Modules/cxx20-decls.cppm
new file mode 100644
index 0..052c8e73be247
--- /dev/null
+++ b/clang/test/Modules/cxx20-decls.cppm
@@ -0,0 +1,44 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = baz::foo;
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl 0x[[BAZ_REDECL_ADDR:[^ ]*]] prev 0x[[BAZ_ADDR:[^ 
]*]] <{{.*}}> line:{{.*}} imported in A. hidden  baz
+// CHECK-NEXT: |-original Namespace 0x[[BAZ_ADDR]] 'baz'
+// CHECK-NEXT: |-TypeAliasDecl 0x[[ALIAS_REDECL_ADDR:[^ ]*]] prev 
0x[[ALIAS_ADDR:[^ ]*]] <{{.*}}> col:{{.*}} imported in A. hidden foo 
'char'
+// CHECK-NEXT: | `-BuiltinType 0x{{[^ ]*}} 'char'
+// CHECK-NEXT: |-UsingDecl 0x{{[^ ]*}} first 0x[[USING_ADDR:[^ ]*]] <{{.*}}> 
col:{{.*}} imported in A. hidden baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace 0x[[BAZ_REDECL_ADDR]] 'baz'
+// FIXME: UsingShadowDecl should have been merged
+// CHECK-NOT:  `-UsingShadowDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} <{{.*}}> 
col:{{.*}} imported in A. hidden implicit TypeAlias 
0x[[ALIAS_REDECL_ADDR]] 'foo'
+// CHECK-NEXT: `-UsingShadowDecl 0x{{[^ ]*}} <{{.*}}> col:{{.*}} imported in 
A. hidden implicit TypeAlias 0x[[ALIAS_REDECL_ADDR]] 'foo'
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl 0x[[BAZ_ADDR]] <{{.*}}> line:{{.*}} baz
+// CHECK-NEXT: |-TypeAliasDecl 0x[[ALIAS_ADDR]] <{{.*}}> col:{{.*}} referenced 
foo 'char'
+// CHECK-NEXT: | `-BuiltinType 0x{{[^ ]*}} 'char'
+// CHECK-NEXT: |-UsingDecl 0x[[USING_ADDR]] <{{.*}}> col:{{.*}} baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace 0x[[BAZ_ADDR]] 'baz'
+// CHECK-NEXT:  `-UsingShadowDecl 0x[[SHADOW_ADDR:[^ ]*]] <{{.*}}> col:{{.*}} 
implicit TypeAlias 0x[[ALIAS_ADDR]] 'foo'

>From f2e37f079cecdd95ba32912aaea739be68959f9e Mon Sep 17 00:00:00 2001
From: Matheus Izvekov 
Date: Thu, 1 Feb 2024 02:26:10 -0300
Subject: [PATCH 2/2] [clang] fix merging of UsingShadowDecl

Previously, when deciding if two UsingShadowDecls where mergeable,
we would incorrectly only look for both pointing to the exact redecla
ration, whereas the correct thing is to look for declarations to the
same entity.

This problem has existed as far back as 2013, introduced in commit
fd8634a09de71.

This problem could manifest itself as ODR check false positives
when importing modules.

Fixes: #80252
---
 clang/docs/ReleaseNotes.rst | 3 +++
 clang/lib/AST/ASTContext.cpp  

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

I have rebased on top of the change which reenables GMF ODR checker on the test 
suite, so I added a new regression test that exercises the false positive ODR 
violation.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

But the test for dumpped text still looks not good. Can we get rid of that?

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

I took a look at what sort of complexity unittest would entail here. I don't 
think it's a good compromise complexity wise, it's a lot of boilerplate just to 
test a few AST nodes are correctly linked.

On the other hand, these AST tests don't look particularly out of place 
compared to a lot of other AST tests we have.

However, I have seen that there are a lot of other related merging issues, so I 
will leave the tangentially related tests for another MR.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> I took a look at what sort of complexity unittest would entail here. I don't 
> think it's a good compromise complexity wise, it's a lot of boilerplate just 
> to test a few AST nodes are correctly linked.

But they are much easier to maintain than dumpped AST text. I feel things get 
pretty clear with unittests. And I don't think there are a lot of complexities. 
They indeed have some boilerplates. But I think it is worhty to make things get 
tested. Also the boilerplates can be improved by refactoring and it is not so 
hurting since they are tests.

> 
> On the other hand, these AST tests don't look particularly out of place 
> compared to a lot of other AST tests we have.

But that is not good. We should try to avoid that.

> 
> However, I have seen that there are a lot of other related merging issues, so 
> I will leave the tangentially related tests for another MR.

I am not sure what's your meaning. What's your intention of testing of this 
patch itself? And I think it is not maintainable or acceptable to test the 
dumpped text in the following patches.



https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov updated 
https://github.com/llvm/llvm-project/pull/80245

>From f7340f3781a3e3094169c970aadb9b69414543f5 Mon Sep 17 00:00:00 2001
From: Matheus Izvekov 
Date: Thu, 1 Feb 2024 02:07:16 -0300
Subject: [PATCH 1/2] [NFC] [clang] add tests for merging of UsingShadowDecl

---
 clang/test/Modules/GH80252.cppm | 42 +++
 clang/test/Modules/cxx20-decls.cppm | 44 +
 2 files changed, 86 insertions(+)
 create mode 100644 clang/test/Modules/GH80252.cppm
 create mode 100644 clang/test/Modules/cxx20-decls.cppm

diff --git a/clang/test/Modules/GH80252.cppm b/clang/test/Modules/GH80252.cppm
new file mode 100644
index 0..3212404fbe14c
--- /dev/null
+++ b/clang/test/Modules/GH80252.cppm
@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cppm -emit-module-interface -o 
%t/B.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/C.cpp -fmodule-file=A=%t/A.pcm 
-fmodule-file=B=%t/B.pcm -fsyntax-only -verify
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- bar.h
+class bar {
+  bar(baz::foo);
+};
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+#include "bar.h"
+export module B;
+
+//--- C.cpp
+#include "foo.h"
+import A;
+#include "bar.h"
+import B;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = bar;
+// FIXME: This is a false positive ODR violation.
+// expected-error@bar.h:2 {{'bar' has different definitions in different 
modules; first difference is defined here found constructor with 1st parameter 
of type 'baz::foo' (aka 'char')}}
+// expected-note@bar.h:2 {{but in 'B.' found constructor with 1st 
parameter of type 'baz::foo' (aka 'char')}}
diff --git a/clang/test/Modules/cxx20-decls.cppm 
b/clang/test/Modules/cxx20-decls.cppm
new file mode 100644
index 0..28af7b4bc47b4
--- /dev/null
+++ b/clang/test/Modules/cxx20-decls.cppm
@@ -0,0 +1,44 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = baz::foo;
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl {{.*}} imported in A. {{.*}} baz
+// CHECK-NEXT: |-original Namespace {{.*}} 'baz'
+// CHECK-NEXT: |-TypeAliasDecl {{.*}} foo 'char'
+// CHECK-NEXT: | `-BuiltinType {{.*}} 'char'
+// CHECK-NEXT: |-UsingDecl {{.*}} baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace {{.*}} 'baz'
+// FIXME: UsingShadowDecl should have been merged
+// CHECK-NOT:  `-UsingShadowDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} {{.*}} imported 
in A. {{.*}} 'foo'
+// CHECK-NEXT: `-UsingShadowDecl 0x{{[^ ]*}} {{.*}} imported in A. 
{{.*}} 'foo'
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl {{.*}} baz
+// CHECK-NEXT: |-TypeAliasDecl {{.*}} foo 'char'
+// CHECK-NEXT: | `-BuiltinType {{.*}} 'char'
+// CHECK-NEXT: |-UsingDecl {{.*}} baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace {{.*}} 'baz'
+// CHECK-NEXT:  `-UsingShadowDecl 0x[[SHADOW_ADDR:[^ ]*]] {{.*}} 'foo'

>From 1e6827281750a7fe33b8fa1bc66524b6f52d9080 Mon Sep 17 00:00:00 2001
From: Matheus Izvekov 
Date: Thu, 1 Feb 2024 02:26:10 -0300
Subject: [PATCH 2/2] [clang] fix merging of UsingShadowDecl

Previously, when deciding if two UsingShadowDecls where mergeable,
we would incorrectly only look for both pointing to the exact redecla
ration, whereas the correct thing is to look for declarations to the
same entity.

This problem has existed as far back as 2013, introduced in commit
fd8634a09de71.

This problem could manifest itself as ODR check false positives
when importing modules.

Fixes: #80252
---
 clang/docs/ReleaseNotes.rst | 3 +++
 clang/lib/AST/ASTContext.cpp| 2 +-
 clang/test/Modules/GH80252.cppm | 4 +---
 clang/test/Modules/cxx20-decls.cppm | 6 ++
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 53040aa0f9074..0a9cd242f9d95 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -188,6 +188,9 @@ Bug Fixes to C++ Support
   and (`#79745 `_)
 - Fix incorrect code generation caused by the object argument of ``static 
operator()`` and

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

I just pushed the change on this patch. What I meant is this patch will get, 
besides aforementioned regression test involving ODR checker failure, a simpler 
AST test that verifies just the UsingShadowDecl is correctly merged.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 commented:

I still think the testing for dumped text is not acceptable. Let's make it 
prettier in unittest.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cppm -emit-module-interface -o 
%t/B.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/C.cpp -fmodule-file=A=%t/A.pcm 
-fmodule-file=B=%t/B.pcm -fsyntax-only -verify
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- bar.h
+class bar {
+  bar(baz::foo);
+};
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;

ChuanqiXu9 wrote:

```suggestion
export module A;
namespace baz {
export using foo;
}
```

Otherwise the declarations may be discarded once we implement 
http://eel.is/c++draft/module.global.frag#4

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cppm -emit-module-interface -o 
%t/B.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/C.cpp -fmodule-file=A=%t/A.pcm 
-fmodule-file=B=%t/B.pcm -fsyntax-only -verify
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- bar.h
+class bar {
+  bar(baz::foo);
+};
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+#include "bar.h"
+export module B;

ChuanqiXu9 wrote:

```suggestion
export module B;
namespace baz {
export using foo;
}
export using ::bar;
```

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;

ChuanqiXu9 wrote:

ditto here.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

There is a lot of activity going on just in `clang/test/AST`, with multiple 
commits a day.
I think there is a time and place, and the right tradeoffs for all kinds of 
tests we have. Shall we get others opinions?
Shall we make an RFC so we get the community aligned on this new direction?

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov updated 
https://github.com/llvm/llvm-project/pull/80245

>From 7bd920a14240c85442467706236e0644f320aa5c Mon Sep 17 00:00:00 2001
From: Matheus Izvekov 
Date: Thu, 1 Feb 2024 02:07:16 -0300
Subject: [PATCH 1/2] [NFC] [clang] add tests for merging of UsingShadowDecl

---
 clang/test/Modules/GH80252.cppm | 45 +
 clang/test/Modules/cxx20-decls.cppm | 44 
 2 files changed, 89 insertions(+)
 create mode 100644 clang/test/Modules/GH80252.cppm
 create mode 100644 clang/test/Modules/cxx20-decls.cppm

diff --git a/clang/test/Modules/GH80252.cppm b/clang/test/Modules/GH80252.cppm
new file mode 100644
index 0..f4730a9874197
--- /dev/null
+++ b/clang/test/Modules/GH80252.cppm
@@ -0,0 +1,45 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cppm -emit-module-interface -o 
%t/B.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/C.cpp -fmodule-file=A=%t/A.pcm 
-fmodule-file=B=%t/B.pcm -fsyntax-only -verify
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- bar.h
+class bar {
+  bar(baz::foo);
+};
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+export using AX = baz::foo;
+
+//--- B.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+#include "bar.h"
+export module B;
+export using BX = baz::foo;
+export using BY = bar;
+
+//--- C.cpp
+#include "foo.h"
+import A;
+#include "bar.h"
+import B;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = bar;
+// FIXME: This is a false positive ODR violation.
+// expected-error@bar.h:2 {{'bar' has different definitions in different 
modules; first difference is defined here found constructor with 1st parameter 
of type 'baz::foo' (aka 'char')}}
+// expected-note@bar.h:2 {{but in 'B.' found constructor with 1st 
parameter of type 'baz::foo' (aka 'char')}}
diff --git a/clang/test/Modules/cxx20-decls.cppm 
b/clang/test/Modules/cxx20-decls.cppm
new file mode 100644
index 0..28af7b4bc47b4
--- /dev/null
+++ b/clang/test/Modules/cxx20-decls.cppm
@@ -0,0 +1,44 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = baz::foo;
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl {{.*}} imported in A. {{.*}} baz
+// CHECK-NEXT: |-original Namespace {{.*}} 'baz'
+// CHECK-NEXT: |-TypeAliasDecl {{.*}} foo 'char'
+// CHECK-NEXT: | `-BuiltinType {{.*}} 'char'
+// CHECK-NEXT: |-UsingDecl {{.*}} baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace {{.*}} 'baz'
+// FIXME: UsingShadowDecl should have been merged
+// CHECK-NOT:  `-UsingShadowDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} {{.*}} imported 
in A. {{.*}} 'foo'
+// CHECK-NEXT: `-UsingShadowDecl 0x{{[^ ]*}} {{.*}} imported in A. 
{{.*}} 'foo'
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl {{.*}} baz
+// CHECK-NEXT: |-TypeAliasDecl {{.*}} foo 'char'
+// CHECK-NEXT: | `-BuiltinType {{.*}} 'char'
+// CHECK-NEXT: |-UsingDecl {{.*}} baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace {{.*}} 'baz'
+// CHECK-NEXT:  `-UsingShadowDecl 0x[[SHADOW_ADDR:[^ ]*]] {{.*}} 'foo'

>From db966f66a148ab5b5ba26250cbf0dab6be5a2c29 Mon Sep 17 00:00:00 2001
From: Matheus Izvekov 
Date: Thu, 1 Feb 2024 02:26:10 -0300
Subject: [PATCH 2/2] [clang] fix merging of UsingShadowDecl

Previously, when deciding if two UsingShadowDecls where mergeable,
we would incorrectly only look for both pointing to the exact redecla
ration, whereas the correct thing is to look for declarations to the
same entity.

This problem has existed as far back as 2013, introduced in commit
fd8634a09de71.

This problem could manifest itself as ODR check false positives
when importing modules.

Fixes: #80252
---
 clang/docs/ReleaseNotes.rst | 3 +++
 clang/lib/AST/ASTContext.cpp| 2 +-
 clang/test/Modules/GH80252.cppm | 4 +---
 clang/test/Modules/cxx20-decls.cppm | 6 ++
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 53040aa0f9074..0a9cd242f9d95 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -188,6 +188,9 @@ Bug Fixes to C++ Support
   and (`#79745 `_)
 - Fix in

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cppm -emit-module-interface -o 
%t/B.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/C.cpp -fmodule-file=A=%t/A.pcm 
-fmodule-file=B=%t/B.pcm -fsyntax-only -verify
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- bar.h
+class bar {
+  bar(baz::foo);
+};
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;

mizvekov wrote:

Done, slightly different, to keep test simple and avoid introducing more 
shadowing declarations.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> There is a lot of activity going on just in `clang/test/AST`, with multiple 
> commits a day. I think there is a time and place, and the right tradeoffs for 
> all kinds of tests we have. Shall we get others opinions? Shall we make an 
> RFC so we get the community aligned on this new direction?

Sounds not bad. I just sent 
https://discourse.llvm.org/t/rfc-prefer-unittests-over-matching-dumpped-ast/76729

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;
+
+//--- B.cpp
+// expected-no-diagnostics
+#include "foo.h"
+import A;
+// Since modules are loaded lazily, force loading by performing a lookup.
+using xxx = baz::foo;
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl {{.*}} imported in A. {{.*}} baz
+// CHECK-NEXT: |-original Namespace {{.*}} 'baz'
+// CHECK-NEXT: |-TypeAliasDecl {{.*}} foo 'char'
+// CHECK-NEXT: | `-BuiltinType {{.*}} 'char'
+// CHECK-NEXT: |-UsingDecl {{.*}} baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace {{.*}} 'baz'
+// CHECK-NEXT: `-UsingShadowDecl 0x{{[^ ]*}} prev 0x[[SHADOW_ADDR:[^ ]*]] 
{{.*}} imported in A. {{.*}} 'foo'
+
+// CHECK-LABEL: Dumping baz:
+// CHECK-NEXT: NamespaceDecl {{.*}} baz
+// CHECK-NEXT: |-TypeAliasDecl {{.*}} foo 'char'
+// CHECK-NEXT: | `-BuiltinType {{.*}} 'char'
+// CHECK-NEXT: |-UsingDecl {{.*}} baz::foo
+// CHECK-NEXT: | `-NestedNameSpecifier Namespace {{.*}} 'baz'
+// CHECK-NEXT:  `-UsingShadowDecl 0x[[SHADOW_ADDR]] {{.*}} 'foo'

ChuanqiXu9 wrote:

While the RFC don't get consensus, I still don't like the test. There is only 
two `UsingShadowDecl` relevent, but we're checking too many things. I feel 
better if we can reduce the things we're checking or refactoring this into a 
unittest.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 -I %t %t/A.cppm -emit-module-interface -o 
%t/A.pcm -verify
+// RUN: %clang_cc1 -std=c++20 -I %t %t/B.cpp -fmodule-file=A=%t/A.pcm 
-fsyntax-only -verify -ast-dump-all -ast-dump-filter baz | FileCheck %s
+
+//--- foo.h
+namespace baz {
+  using foo = char;
+  using baz::foo;
+}
+
+//--- A.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module A;

ChuanqiXu9 wrote:

This is not resolved

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov updated 
https://github.com/llvm/llvm-project/pull/80245

>From 9763cc9e6f081bc28b74164c77a2b80ac42aec1c Mon Sep 17 00:00:00 2001
From: Matheus Izvekov 
Date: Thu, 1 Feb 2024 02:26:10 -0300
Subject: [PATCH] [clang] fix merging of UsingShadowDecl

Previously, when deciding if two UsingShadowDecls where mergeable,
we would incorrectly only look for both pointing to the exact redecla
ration, whereas the correct thing is to look for declarations to the
same entity.

This problem has existed as far back as 2013, introduced in commit
fd8634a09de71.

This problem could manifest itself as ODR check false positives
when importing modules.

Fixes: #80252
---
 clang/docs/ReleaseNotes.rst | 3 +++
 clang/lib/AST/ASTContext.cpp| 2 +-
 clang/test/Modules/cxx20-decls.cppm | 4 ++--
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 44035f48cb3f9..9dc93f53fe716 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -813,6 +813,9 @@ Bug Fixes to C++ Support
 - Clang now allows ``@$``` in raw string literals. Fixes (#GH93130).
 - Fix an assertion failure when checking invalid ``this`` usage in the wrong 
context. (Fixes #GH91536).
 - Clang no longer models dependent NTTP arguments as 
``TemplateParamObjectDecl`` s. Fixes (#GH84052).
+- Fix incorrect merging of modules which contain using declarations which 
shadow
+  other declarations. This could manifest as ODR checker false positives.
+  Fixes (`#80252 `_)
 
 Bug Fixes to AST Handling
 ^
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 06780ceba4074..73d3b152c49f1 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -6794,7 +6794,7 @@ bool ASTContext::isSameEntity(const NamedDecl *X, const 
NamedDecl *Y) const {
   // Using shadow declarations with the same target match.
   if (const auto *USX = dyn_cast(X)) {
 const auto *USY = cast(Y);
-return USX->getTargetDecl() == USY->getTargetDecl();
+return declaresSameEntity(USX->getTargetDecl(), USY->getTargetDecl());
   }
 
   // Using declarations with the same qualifier match. (We already know that
diff --git a/clang/test/Modules/cxx20-decls.cppm 
b/clang/test/Modules/cxx20-decls.cppm
index 9f0c40685b68f..0e8b59708ab4c 100644
--- a/clang/test/Modules/cxx20-decls.cppm
+++ b/clang/test/Modules/cxx20-decls.cppm
@@ -28,8 +28,8 @@ using xxx = baz::foo;
 // CHECK-NEXT: NamespaceDecl 0x[[BAZ_REDECL_ADDR:[^ ]*]] prev 0x[[BAZ_ADDR:[^ 
]*]]
 // CHECK:  TypeAliasDecl 0x[[ALIAS_REDECL_ADDR:[^ ]*]] prev 
0x[[ALIAS_ADDR:[^ ]*]]
 // FIXME: UsingShadowDecl should have been merged
-// CHECK:  UsingShadowDecl 0x{{[^ ]*}} <{{.*}}> col:{{.*}} imported in 
A. hidden implicit TypeAlias 0x[[ALIAS_REDECL_ADDR]] 'foo'
+// CHECK:  UsingShadowDecl 0x{{[^ ]*}} prev 0x[[SHADOW_ADDR:[^ ]*]] {{.*}} 
imported in A. {{.*}} 'foo'
 
 // CHECK-LABEL: Dumping baz:
 // CHECK-NEXT: NamespaceDecl 0x[[BAZ_ADDR]] <{{.*}}> line:{{.*}} baz
-// CHECK:  UsingShadowDecl 0x[[SHADOW_ADDR:[^ ]*]] <{{.*}}> col:{{.*}} 
implicit TypeAlias 0x[[ALIAS_ADDR]] 'foo'
+// CHECK:  UsingShadowDecl 0x[[SHADOW_ADDR]] {{.*}} 'foo'

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

mizvekov wrote:

I have revived this PR after a long time.

The original ODR violation bad diagnostic from the GH issue is gone from main, 
so I have removed the test for it.
My best guess is that the ODR checking must have gotten weakened somehow.
I really don't have time to dig into it though.

The original bug is still there in main, is self evident from looking at the 
source code, and can be confirmed bad and that it gets fixed by just looking at 
the AST dump.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 approved this pull request.

LG. It looks not wise to block this good PR. After all, it is a change 6 lines. 
Let's go ahead.

https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 edited 
https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] fix merging of UsingShadowDecl (PR #80245)

[Matheus Izvekov via cfe-commits]

https://github.com/mizvekov closed 
https://github.com/llvm/llvm-project/pull/80245
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits