[clang] [C++20][Modules] Load function body from the module that gives canonical decl (PR #111992)

[Dmitry Polukhin via cfe-commits]

https://github.com/dmpolukhin updated https://github.com/llvm/llvm-project/pull/111992

>From 0d02e79ced894769cfbea45a121670e933e2c886 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Fri, 11 Oct 2024 05:35:18 -0700
Subject: [PATCH 1/2] [C++20][Modules] Load function body from the module that
 gives cannonical decl

Summary:
Fix crash from reproducer provided in https://github.com/llvm/llvm-project/pull/109167#issuecomment-2405289565

Test Plan: TBD
---
 clang/lib/Serialization/ASTReader.cpp | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index e638129897692f..f2853dcf77ae94 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -10057,15 +10057,18 @@ void ASTReader::finishPendingActions() {
       // For a function defined inline within a class template, force the
       // canonical definition to be the one inside the canonical definition of
       // the template. This ensures that we instantiate from a correct view
-      // of the template.
+      // of the template. This behaviour seems to be important only for inline
+      // friend functions. For normal member functions, it might results in
+      // selecting canonical decl from module A but body from module B.
       //
       // Sadly we can't do this more generally: we can't be sure that all
       // copies of an arbitrary class definition will have the same members
       // defined (eg, some member functions may not be instantiated, and some
       // special members may or may not have been implicitly defined).
-      if (auto *RD = dyn_cast<CXXRecordDecl>(FD->getLexicalParent()))
-        if (RD->isDependentContext() && !RD->isThisDeclarationADefinition())
-          continue;
+      if (FD->getFriendObjectKind())
+        if (auto *RD = dyn_cast<CXXRecordDecl>(FD->getLexicalParent()))
+          if (RD->isDependentContext() && !RD->isThisDeclarationADefinition())
+            continue;
 
       // FIXME: Check for =delete/=default?
       const FunctionDecl *Defn = nullptr;

>From 7c2fbfe9367b98ac36bf38b86c67f9729c9138ff Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Fri, 11 Oct 2024 09:55:35 -0700
Subject: [PATCH 2/2] Add test case

---
 ...ash-instantiated-in-scope-cxx-modules4.cpp | 110 ++++++++++++++++++
 1 file changed, 110 insertions(+)
 create mode 100644 clang/test/Headers/crash-instantiated-in-scope-cxx-modules4.cpp

diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules4.cpp b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules4.cpp
new file mode 100644
index 00000000000000..087eb135dc5f53
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules4.cpp
@@ -0,0 +1,110 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -std=c++20 -x c++ -fmodule-map-file=modules.map -fmodule-name=foo1 -emit-module modules.map -o foo1.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -x c++ -fmodule-map-file=modules.map -fmodule-name=foo2 -emit-module modules.map -o foo2.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -x c++ -fmodule-map-file=modules.map -fmodule-file=foo1.pcm -fmodule-file=foo2.pcm server.cc
+
+//--- functional
+#pragma once
+
+namespace std {
+template <class> class function {};
+} // namespace std
+
+//--- foo.h
+#pragma once
+
+class MethodHandler {
+ public:
+  virtual ~MethodHandler() {}
+  struct HandlerParameter {
+    HandlerParameter();
+  };
+  virtual void RunHandler(const HandlerParameter &param);
+};
+
+template <class RequestType, class ResponseType>
+class CallbackUnaryHandler : public MethodHandler {
+ public:
+  explicit CallbackUnaryHandler();
+
+  void RunHandler(const HandlerParameter &param) final {
+    void *call = nullptr;
+    (void)[call](bool){};
+  }
+};
+
+//--- foo1.h
+// expected-no-diagnostics
+#pragma once
+
+#include "functional"
+
+#include "foo.h"
+
+class A;
+
+class ClientAsyncResponseReaderHelper {
+   public:
+      using t = std::function<void(A)>;
+        static void SetupRequest(t finish);
+};
+
+//--- foo2.h
+// expected-no-diagnostics
+#pragma once
+
+#include "foo.h"
+
+template <class BaseClass>
+class a : public BaseClass {
+ public:
+  a() { [[maybe_unused]] CallbackUnaryHandler<int, int> a; }
+};
+
+//--- modules.map
+module "foo" {
+  export *
+  module "foo.h" {
+    export *
+    textual header "foo.h"
+  }
+}
+
+module "foo1" {
+  export *
+  module "foo1.h" {
+    export *
+    header "foo1.h"
+  }
+
+  use "foo"
+}
+
+module "foo2" {
+  export *
+  module "foo2.h" {
+    export *
+    header "foo2.h"
+  }
+
+  use "foo"
+}
+
+//--- server.cc
+// expected-no-diagnostics
+#include "functional"
+
+#include "foo1.h"
+#include "foo2.h"
+
+std::function<void()> on_emit;
+
+template <class RequestType, class ResponseType>
+class CallbackUnaryHandler;
+
+class s {};
+class hs final : public a<s> {
+  explicit hs() {}
+};