[llvm-branch-commits] [clang] [C++20] [Modules] Load friends for classes in ADL (#219094) (PR #226711)

[via llvm-branch-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-modules

Author: Avi Kivity (avikivity)

<details>
<summary>Changes</summary>

Close https://github.com/llvm/llvm-project/issues/218228

The root cause of the problem is the corresponding friend is not loaded at the point of ADL.

This patch tries to fix this simply by loading the friends at the point of ADL. Note that this may be best efficient if there are a lot of friends. We just think it is rare. If it is really possible, we can change the structure of friends from a list to a name lookup table.

(cherry picked from commit 01aedf3b34325ad2f74a77325a2da9a7e36ae3d5)

---
Full diff: https://github.com/llvm/llvm-project/pull/226711.diff


4 Files Affected:

- (modified) clang/include/clang/AST/DeclCXX.h (+4) 
- (modified) clang/lib/AST/DeclFriend.cpp (+9) 
- (modified) clang/lib/Sema/SemaLookup.cpp (+11) 
- (added) clang/test/Modules/pr218228.cppm (+95) 


``````````diff
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index 45fb99e27b137..614ad02c457b5 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -692,6 +692,10 @@ class CXXRecordDecl : public RecordDecl {
     return data().FirstFriend.isValid();
   }
 
+  bool hasLazyFriends() const { return data().FirstFriend.isOffset(); }
+
+  void loadLazyFriends();
+
   /// \c true if a defaulted copy constructor for this class would be
   /// deleted.
   bool defaultedCopyConstructorIsDeleted() const {
diff --git a/clang/lib/AST/DeclFriend.cpp b/clang/lib/AST/DeclFriend.cpp
index 6bfc2eb62b284..d90f1f1bc8f42 100644
--- a/clang/lib/AST/DeclFriend.cpp
+++ b/clang/lib/AST/DeclFriend.cpp
@@ -72,3 +72,12 @@ FriendDecl *CXXRecordDecl::getFirstFriend() const {
   Decl *First = data().FirstFriend.get(Source);
   return First ? cast<FriendDecl>(First) : nullptr;
 }
+
+void CXXRecordDecl::loadLazyFriends() {
+  assert(hasDefinition());
+  assert(hasLazyFriends());
+  ExternalASTSource *Source = getParentASTContext().getExternalSource();
+  FriendDecl *Friend = cast_or_null<FriendDecl>(data().FirstFriend.get(Source));
+  while (Friend && Friend->NextFriend.isOffset())
+    Friend = cast_or_null<FriendDecl>(Friend->NextFriend.get(Source));
+}
diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index 43129800e9813..fe2763213280f 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -3881,6 +3881,17 @@ void Sema::ArgumentDependentLookup(DeclarationName Name, SourceLocation Loc,
                                      AssociatedNamespaces,
                                      AssociatedClasses);
 
+  // Load the friend classes in case there are unloaded decls.
+  //
+  // FIXME: Currently this is inefficient if there are a lot of friends
+  // in the classes. We just expect the number of friends are limited
+  // in real world. In case we meet the case that the loading friends
+  // became a threshold, we can change the structure of friends from
+  // a list to a name lookup table.
+  for (CXXRecordDecl *Class : AssociatedClasses)
+    if (Class->hasDefinition() && Class->hasLazyFriends())
+      Class->loadLazyFriends();
+
   // C++ [basic.lookup.argdep]p3:
   //   Let X be the lookup set produced by unqualified lookup (3.4.1)
   //   and let Y be the lookup set produced by argument dependent
diff --git a/clang/test/Modules/pr218228.cppm b/clang/test/Modules/pr218228.cppm
new file mode 100644
index 0000000000000..b00ed95f465cf
--- /dev/null
+++ b/clang/test/Modules/pr218228.cppm
@@ -0,0 +1,95 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++23 -emit-module-interface %t/stdx.cc -o %t/stdx.pcm
+// RUN: %clang_cc1 -std=c++23 -emit-module-interface %t/a.cppm -o %t/a.pcm -fmodule-file=stdx=%t/stdx.pcm
+// RUN: %clang_cc1 -std=c++23 %t/b.cppm -fmodule-file=stdx=%t/stdx.pcm -fmodule-file=a=%t/a.pcm -fsyntax-only -verify
+// RUN: %clang_cc1 -std=c++23 %t/c.cpp -fmodule-file=stdx=%t/stdx.pcm -fmodule-file=a=%t/a.pcm -fsyntax-only -verify
+// RUN: %clang_cc1 -std=c++23 %t/d.cpp -fmodule-file=stdx=%t/stdx.pcm -fmodule-file=a=%t/a.pcm -fsyntax-only -verify
+
+// Test again with Reduced BMI
+// RUN: %clang_cc1 -std=c++23 -emit-reduced-module-interface %t/stdx.cc -o %t/stdx.pcm
+// RUN: %clang_cc1 -std=c++23 -emit-reduced-module-interface %t/a.cppm -o %t/a.pcm -fmodule-file=stdx=%t/stdx.pcm
+// RUN: %clang_cc1 -std=c++23 %t/b.cppm -fmodule-file=stdx=%t/stdx.pcm -fmodule-file=a=%t/a.pcm -fsyntax-only -verify
+// RUN: %clang_cc1 -std=c++23 %t/c.cpp -fmodule-file=stdx=%t/stdx.pcm -fmodule-file=a=%t/a.pcm -fsyntax-only -verify
+// RUN: %clang_cc1 -std=c++23 %t/d.cpp -fmodule-file=stdx=%t/stdx.pcm -fmodule-file=a=%t/a.pcm -fsyntax-only -verify
+
+//--- pipe.hh
+namespace demo {
+
+template <typename Derived>
+struct RAC {
+  template <typename R, typename Self>
+  friend constexpr auto operator|(R&&, Self&&);
+};
+
+template <typename A, typename Arg>
+struct P : RAC<P<A, Arg>> {
+  template <typename R>
+  constexpr auto operator()(R&& r) const {
+    return A{}(static_cast<R&&>(r));
+  }
+};
+
+struct myadapt {
+  template <class... Args>
+  constexpr auto operator()(Args&&...) const {
+    return P<myadapt, Args...>{};
+  }
+};
+
+template <typename R, typename Self>
+constexpr auto operator|(R&& r, Self&& self) {
+  return static_cast<Self&&>(self)(static_cast<R&&>(r));
+}
+
+inline constexpr myadapt adapt{};
+
+} // namespace demo
+
+//--- stdx.cc
+module;
+
+#include "pipe.hh"
+
+export module stdx;
+
+export namespace demo {
+
+using demo::myadapt;
+using demo::adapt;
+
+// To avoid another GCC-only problem
+inline void force() { struct X{}; (void)(X{} | demo::adapt(1)); }
+
+} // namespace demo
+
+//--- a.cppm
+export module a;
+import stdx;
+void g() { (void) demo::adapt('.'); }
+
+//--- b.cppm
+// expected-no-diagnostics
+export module b;
+import stdx;
+import a;
+struct view {};
+void f() { (void)(view{} | demo::adapt('.')); }
+
+//--- c.cpp
+// expected-no-diagnostics
+import stdx;
+import a;
+#include "pipe.hh"
+struct view {};
+void f() { (void)(view{} | demo::adapt('.')); }
+
+//--- d.cpp
+// expected-no-diagnostics
+#include "pipe.hh"
+import stdx;
+import a;
+struct view {};
+void f() { (void)(view{} | demo::adapt('.')); }

``````````

</details>


https://github.com/llvm/llvm-project/pull/226711