[clang] cb66bf2 - Replace 'magic static' with a member variable for SCYL kernel names

[Erich Keane via cfe-commits]

Author: Erich Keane
Date: 2021-05-27T13:46:31-07:00
New Revision: cb66bf2c6d20da01ab57cb78ec5e5c0978b873be

URL: https://github.com/llvm/llvm-project/commit/cb66bf2c6d20da01ab57cb78ec5e5c0978b873be
DIFF: https://github.com/llvm/llvm-project/commit/cb66bf2c6d20da01ab57cb78ec5e5c0978b873be.diff

LOG: Replace 'magic static' with a member variable for SCYL kernel names

I discovered when merging the __builtin_sycl_unique_stable_name into my
downstream that it is actually possible for the cc1 invocation to have
more than 1 Sema instance, if you pass it multiple input files, each
gets its own Sema instance and thus ASTContext instance.  The result was
that the call to Filter the SYCL kernels was using an
ItaniumMangleContext stored via a 'magic static', so it had an invalid
reference to ASTContext when processing the 2nd failure.

The failure is unfortunately flakey/transient, but the test that fails
was added anyway.

The magic-static was switched to a unique_ptr member variable in
ASTContext that is initialized when needed.

Added: 
    clang/test/SemaSYCL/unique-stable-name-multiple-target-crash.cpp

Modified: 
    clang/include/clang/AST/ASTContext.h
    clang/lib/AST/ASTContext.cpp

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index db6d263a5e15..22588a61f6ff 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -103,6 +103,7 @@ class DynTypedNode;
 class DynTypedNodeList;
 class Expr;
 class GlobalDecl;
+class ItaniumMangleContext;
 class MangleContext;
 class MangleNumberingContext;
 class MaterializeTemporaryExpr;
@@ -3166,7 +3167,7 @@ OPT_LIST(V)
 
   void AddSYCLKernelNamingDecl(const CXXRecordDecl *RD);
   bool IsSYCLKernelNamingDecl(const NamedDecl *RD) const;
-  unsigned GetSYCLKernelNamingIndex(const NamedDecl *RD) const;
+  unsigned GetSYCLKernelNamingIndex(const NamedDecl *RD);
   /// A SourceLocation to store whether we have evaluated a kernel name already,
   /// and where it happened.  If so, we need to diagnose an illegal use of the
   /// builtin.
@@ -3185,6 +3186,10 @@ OPT_LIST(V)
   llvm::DenseMap<const DeclContext *,
                  llvm::SmallPtrSet<const CXXRecordDecl *, 4>>
       SYCLKernelNamingTypes;
+  std::unique_ptr<ItaniumMangleContext> SYCLKernelFilterContext;
+  void FilterSYCLKernelNamingDecls(
+      const CXXRecordDecl *RD,
+      llvm::SmallVectorImpl<const CXXRecordDecl *> &Decls);
 };
 
 /// Insertion operator for diagnostics.

diff  --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 75656496a946..e96f52920521 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -11715,25 +11715,27 @@ bool ASTContext::IsSYCLKernelNamingDecl(const NamedDecl *ND) const {
 
 // Filters the Decls list to those that share the lambda mangling with the
 // passed RD.
-static void FilterSYCLKernelNamingDecls(
-    ASTContext &Ctx, const CXXRecordDecl *RD,
+void ASTContext::FilterSYCLKernelNamingDecls(
+    const CXXRecordDecl *RD,
     llvm::SmallVectorImpl<const CXXRecordDecl *> &Decls) {
-  static std::unique_ptr<ItaniumMangleContext> Mangler{
-      ItaniumMangleContext::create(Ctx, Ctx.getDiagnostics())};
+
+  if (!SYCLKernelFilterContext)
+    SYCLKernelFilterContext.reset(
+        ItaniumMangleContext::create(*this, getDiagnostics()));
 
   llvm::SmallString<128> LambdaSig;
   llvm::raw_svector_ostream Out(LambdaSig);
-  Mangler->mangleLambdaSig(RD, Out);
+  SYCLKernelFilterContext->mangleLambdaSig(RD, Out);
 
-  llvm::erase_if(Decls, [&LambdaSig](const CXXRecordDecl *LocalRD) {
+  llvm::erase_if(Decls, [this, &LambdaSig](const CXXRecordDecl *LocalRD) {
     llvm::SmallString<128> LocalLambdaSig;
     llvm::raw_svector_ostream LocalOut(LocalLambdaSig);
-    Mangler->mangleLambdaSig(LocalRD, LocalOut);
+    SYCLKernelFilterContext->mangleLambdaSig(LocalRD, LocalOut);
     return LambdaSig != LocalLambdaSig;
   });
 }
 
-unsigned ASTContext::GetSYCLKernelNamingIndex(const NamedDecl *ND) const {
+unsigned ASTContext::GetSYCLKernelNamingIndex(const NamedDecl *ND) {
   assert(getLangOpts().isSYCL() && "Only valid for SYCL programs");
   assert(IsSYCLKernelNamingDecl(ND) &&
          "Lambda not involved in mangling asked for a naming index?");
@@ -11753,7 +11755,7 @@ unsigned ASTContext::GetSYCLKernelNamingIndex(const NamedDecl *ND) const {
   // doesn't use the itanium mangler, and just sets the lambda mangling number
   // incrementally, with no consideration to the signature.
   if (Target->getCXXABI().getKind() != TargetCXXABI::Microsoft)
-    FilterSYCLKernelNamingDecls(const_cast<ASTContext &>(*this), RD, Decls);
+    FilterSYCLKernelNamingDecls(RD, Decls);
 
   llvm::sort(Decls, [](const CXXRecordDecl *LHS, const CXXRecordDecl *RHS) {
     return LHS->getLambdaManglingNumber() < RHS->getLambdaManglingNumber();

diff  --git a/clang/test/SemaSYCL/unique-stable-name-multiple-target-crash.cpp b/clang/test/SemaSYCL/unique-stable-name-multiple-target-crash.cpp
new file mode 100644
index 000000000000..ec78feac8b7b
--- /dev/null
+++ b/clang/test/SemaSYCL/unique-stable-name-multiple-target-crash.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s %s -std=c++17 -triple x86_64-linux-gnu -fsycl-is-device -verify -fsyntax-only -Wno-unused
+
+// This would crash due to the double-inputs, since the 'magic static' use in
+// the AST Context SCYL Filtering would end up caching an old version of the
+// ASTContext object, which no longer exists in the second file's invocation.
+//
+// expected-no-diagnostics
+class Empty {};
+template <typename, typename F> __attribute__((sycl_kernel)) void kernel(F) {
+    __builtin_sycl_unique_stable_name(F);
+}
+
+void use() {
+  [](Empty) {
+    auto lambda = []{};
+    kernel<class i>(lambda);
+  };
+}