[clang] [C++20] [Modules] Profiling non dependent noexcept expression by pointer (PR #224528)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

<details>
<summary>Changes</summary>

Another approach to solve https://github.com/llvm/llvm-project/issues/191361 and https://github.com/llvm/llvm-project/issues/224180 than https://github.com/llvm/llvm-project/pull/222148

I want to avoid competing PR but I do feel the original one's approach seems too workaround. And I want to emphasize that, in my experience, finding the root cause is much more harder than find the solution to me. Many thanks to @<!-- -->ispeters

For the approach itself, I described them in the comments of the PR.

Note that the real problem is we were profiling something is not completely deserialized, so that even if it is not crash, the logic is not correct.

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


3 Files Affected:

- (modified) clang/lib/AST/Type.cpp (+13-1) 
- (added) clang/test/Modules/concept-specialization-deserialization.cppm (+37) 
- (modified) clang/unittests/Serialization/LoadSpecLazilyTest.cpp (+42) 


``````````diff
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 98901502cf37a..123028d214100 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -4094,7 +4094,19 @@ void FunctionProtoType::Profile(llvm::FoldingSetNodeID &ID, QualType Result,
   if (epi.ExceptionSpec.Type == EST_Dynamic) {
     for (QualType Ex : epi.ExceptionSpec.Exceptions)
       ID.AddPointer(Ex.getAsOpaquePtr());
-  } else if (isComputedNoexcept(epi.ExceptionSpec.Type)) {
+  } else if (epi.ExceptionSpec.Type == EST_NoexceptTrue ||
+             epi.ExceptionSpec.Type == EST_NoexceptFalse) {
+    // If the exception type has already been determined, we can use the
+    // address of the expression as profiling results instead of profiling the
+    // expression.
+    //
+    // This is not only an optimization but avoids an access on uninitlaized
+    // fields during the profiling.
+    //
+    // See clang/test/Modules/concept-specialization-deserialization.cppm for
+    // an example.
+    ID.AddPointer(epi.ExceptionSpec.NoexceptExpr);
+  } else if (epi.ExceptionSpec.Type == EST_DependentNoexcept) {
     // getFunctionTypeInternal compares noexcept expressions after the lookup,
     // so the key only needs their canonical form.
     epi.ExceptionSpec.NoexceptExpr->Profile(ID, Context, /*Canonical=*/true);
diff --git a/clang/test/Modules/concept-specialization-deserialization.cppm b/clang/test/Modules/concept-specialization-deserialization.cppm
new file mode 100644
index 0000000000000..876008e1e5ece
--- /dev/null
+++ b/clang/test/Modules/concept-specialization-deserialization.cppm
@@ -0,0 +1,37 @@
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=a=%t/a.pcm -fsyntax-only %t/use.cpp -verify
+//
+// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=a=%t/a.pcm -fsyntax-only %t/use.cpp -verify
+
+//--- a.cppm
+export module a;
+template <class> concept C = true;
+
+template <class T> int fn() noexcept(C<T>);
+export using t = decltype(fn<int>());
+
+//--- use.cpp
+// expected-no-diagnostics
+import a;
+
+// During the deserialization process of fn<int>, the C<int> in noexcept expression
+// may be not completely deserialized. This test makes sure that we can handle the case.
+//
+// The ordering is:
+//
+//  Deserialize C<int>
+//
+//  Deserializing C<int>'s template argument
+//
+//  Read SubstTemplateTypeParmType::AssociatedDecl in readSubstTemplateTypeParmType
+//
+//  Deserialize fn<int>
+//
+//  FunctionProtoType::Profile
+//
+//  but C<int>'s template arguments is not deserialized yet.
+t x;
diff --git a/clang/unittests/Serialization/LoadSpecLazilyTest.cpp b/clang/unittests/Serialization/LoadSpecLazilyTest.cpp
index f55925aeae1f2..4ae85f4f910e6 100644
--- a/clang/unittests/Serialization/LoadSpecLazilyTest.cpp
+++ b/clang/unittests/Serialization/LoadSpecLazilyTest.cpp
@@ -6,6 +6,8 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Type.h"
 #include "clang/Driver/CreateInvocationFromArgs.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendAction.h"
@@ -95,6 +97,46 @@ class LoadSpecLazilyTest : public ::testing::Test {
 
 enum class CheckingMode { Forbidden, Required };
 
+TEST_F(LoadSpecLazilyTest, ConceptSpecializationInExceptionSpecification) {
+  GenerateModuleInterface("M", R"cpp(
+export module M;
+template <class> concept C = true;
+template <class T> int fn() noexcept(C<T>);
+template <class T> int gn() noexcept(!C<T>);
+export using t = decltype(fn<int>());
+export using u = decltype(gn<int>());
+  )cpp");
+
+  auto AST = buildASTFromCodeWithArgs(
+      "import M;\nt x;\nu y;",
+      {"-std=c++20", "-fprebuilt-module-path=" + TestDir.str().str()});
+  ASSERT_TRUE(AST);
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  ASTContext &Ctx = AST->getASTContext();
+
+  // Reading C<int>'s substituted argument loads fn<int>, including its
+  // exception specification, before the argument has been populated. If
+  // constructing the function type profiles C<int> at that point, its
+  // FoldingSet key changes once deserialization completes.
+  SmallVector<const FunctionProtoType *, 4> Types;
+  bool SawNoexceptTrue = false, SawNoexceptFalse = false;
+  for (const auto *T : Ctx.getTypes()) {
+    const auto *F = dyn_cast<FunctionProtoType>(T);
+    if (F && (F->getExceptionSpecType() == EST_NoexceptTrue ||
+              F->getExceptionSpecType() == EST_NoexceptFalse)) {
+      SawNoexceptTrue |= F->getExceptionSpecType() == EST_NoexceptTrue;
+      SawNoexceptFalse |= F->getExceptionSpecType() == EST_NoexceptFalse;
+      Types.push_back(F);
+    }
+  }
+  EXPECT_TRUE(SawNoexceptTrue);
+  EXPECT_TRUE(SawNoexceptFalse);
+  for (const FunctionProtoType *F : Types)
+    EXPECT_EQ(F, Ctx.getFunctionType(F->getReturnType(), F->getParamTypes(),
+                                     F->getExtProtoInfo())
+                     .getTypePtr());
+}
+
 class DeclsReaderListener : public ASTDeserializationListener {
   StringRef SpeficiedName;
   CheckingMode Mode;

``````````

</details>


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