From cb66bf2c6d20da01ab57cb78ec5e5c0978b873be Mon Sep 17 00:00:00 2001 From: Erich Keane Date: Thu, 27 May 2021 13:40:15 -0700 Subject: [PATCH] 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. --- clang/include/clang/AST/ASTContext.h | 7 ++++++- clang/lib/AST/ASTContext.cpp | 20 ++++++++++--------- ...ique-stable-name-multiple-target-crash.cpp | 18 +++++++++++++++++ 3 files changed, 35 insertions(+), 10 deletions(-) create mode 100644 clang/test/SemaSYCL/unique-stable-name-multiple-target-crash.cpp 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 @@ public: 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 @@ private: llvm::DenseMap> SYCLKernelNamingTypes; + std::unique_ptr SYCLKernelFilterContext; + void FilterSYCLKernelNamingDecls( + const CXXRecordDecl *RD, + llvm::SmallVectorImpl &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 &Decls) { - static std::unique_ptr 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(*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 __attribute__((sycl_kernel)) void kernel(F) { + __builtin_sycl_unique_stable_name(F); +} + +void use() { + [](Empty) { + auto lambda = []{}; + kernel(lambda); + }; +}