From b0698686d60b46cd419a16c6ba8a03ed770f5eeb Mon Sep 17 00:00:00 2001 From: Erich Keane Date: Wed, 15 Jan 2020 09:37:27 -0800 Subject: [PATCH 1/2] [SYCL] Fix regression introduced by f537293/part of #1018 fix. This fixes the immediate problem, where diagnostics were being missed in kernels. The problem ends up being that diagnostics can happen at three different times during translation for templates: 1- During Phase 1 2- During Phase 2 instantiation 3- During ConstructOpenCLKernels. f537293 suppressed 3, and did a better job at suppressing 1. However, CallExprs are always rebuilt during instantation (for technical reasons). The result was the variadic call was being diagnosed in all 3 while developing f537293. I erronously thought the best way was to simply wait for instantiation to diagnose all SYCL diagnostics. Unfortunately, this was short-sighted. Not everything in tree-transform gets rebuilt every time. One perfect example is the inline-asm (see the added example). It doesn't get rebuilt in some cases, so it was never diagnosed during instantiation. Because I had fixed Phase 1/ConstructOpenCLKernels, this left no time for it to be diagnosed! The fix was to keep suppressing 3, and simply fix the call-expr check by moving the check to Sema::checkCall. This is how Clang works around this issue for warnings. The downside is that the diagnostics will happen slightly later for this (see the sycl-restrict.cpp test, which reverted to the previous version before f537293), but invalid programs still won't be accepted. Signed-off-by: Erich Keane --- clang/lib/Sema/SemaChecking.cpp | 6 ++++++ clang/lib/Sema/SemaExpr.cpp | 6 ------ clang/lib/Sema/SemaOverload.cpp | 5 ----- clang/lib/Sema/SemaSYCL.cpp | 6 +----- clang/test/SemaSYCL/inline-asm.cpp | 5 +++++ clang/test/SemaSYCL/sycl-restrict.cpp | 5 ++--- 6 files changed, 14 insertions(+), 19 deletions(-) diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 682d2ebf97689..15d86255c5bdc 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -4458,6 +4458,12 @@ void Sema::checkCall(NamedDecl *FDecl, const FunctionProtoType *Proto, if (FD) diagnoseArgDependentDiagnoseIfAttrs(FD, ThisArg, Args, Loc); + + // Diagnose variadic calls in SYCL. + if (FD && FD ->isVariadic() && getLangOpts().SYCLIsDevice && + !isUnevaluatedContext() && !isKnownGoodSYCLDecl(FD)) + SYCLDiagIfDeviceCode(Loc, diag::err_sycl_restrict) + << Sema::KernelCallVariadicFunction; } /// CheckConstructorCall - Check a constructor call for correctness and safety diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index be815cc22a5f6..ac2a9e1cff08e 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -5404,12 +5404,6 @@ bool Sema::GatherArgumentsForCall(SourceLocation CallLoc, FunctionDecl *FDecl, // Otherwise do argument promotion, (C99 6.5.2.2p7). } else { - // Diagnose variadic calls in SYCL. - if (getLangOpts().SYCLIsDevice && !isUnevaluatedContext() && - !isKnownGoodSYCLDecl(FDecl)) - SYCLDiagIfDeviceCode(CallLoc, diag::err_sycl_restrict) - << Sema::KernelCallVariadicFunction; - for (Expr *A : Args.slice(ArgIx)) { ExprResult Arg = DefaultVariadicArgumentPromotion(A, CallType, FDecl); Invalid |= Arg.isInvalid(); diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp index 4e2e3bbf4f173..5fb59b545176d 100644 --- a/clang/lib/Sema/SemaOverload.cpp +++ b/clang/lib/Sema/SemaOverload.cpp @@ -14230,11 +14230,6 @@ Sema::BuildCallToObjectOfClassType(Scope *S, Expr *Obj, // If this is a variadic call, handle args passed through "...". if (Proto->isVariadic()) { - // Diagnose variadic calls in SYCL. - if (getLangOpts().SYCLIsDevice && !isUnevaluatedContext()) - SYCLDiagIfDeviceCode(LParenLoc, diag::err_sycl_restrict) - << Sema::KernelCallVariadicFunction; - // Promote the arguments (C99 6.5.2.2p7). for (unsigned i = NumParams, e = Args.size(); i < e; i++) { ExprResult Arg = DefaultVariadicArgumentPromotion(Args[i], VariadicMethod, diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp index 477ec98b4d48f..25a6b6b02cc19 100644 --- a/clang/lib/Sema/SemaSYCL.cpp +++ b/clang/lib/Sema/SemaSYCL.cpp @@ -1391,10 +1391,6 @@ static bool isKnownEmitted(Sema &S, FunctionDecl *FD) { if (!FD) return true; // Seen in LIT testing - // Templates are emitted when they're instantiated. - if (FD->isDependentContext()) - return false; - if (FD->hasAttr() || FD->hasAttr()) return true; @@ -1409,7 +1405,7 @@ Sema::DeviceDiagBuilder Sema::SYCLDiagIfDeviceCode(SourceLocation Loc, "Should only be called during SYCL compilation"); FunctionDecl *FD = dyn_cast(getCurLexicalContext()); DeviceDiagBuilder::Kind DiagKind = [this, FD] { - if (ConstructingOpenCLKernel || (FD && FD->isDependentContext())) + if (ConstructingOpenCLKernel) return DeviceDiagBuilder::K_Nop; else if (isKnownEmitted(*this, FD)) return DeviceDiagBuilder::K_ImmediateWithCallStack; diff --git a/clang/test/SemaSYCL/inline-asm.cpp b/clang/test/SemaSYCL/inline-asm.cpp index 6290c01367bfb..4d414e29a9aa7 100644 --- a/clang/test/SemaSYCL/inline-asm.cpp +++ b/clang/test/SemaSYCL/inline-asm.cpp @@ -23,6 +23,11 @@ template __attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) { // expected-note@+1 {{called by 'kernel_single_task __attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) { - // expected-note@+1 {{called by 'kernel_single_task}} kernelFunc(); a_type ab; a_type *p; + // expected-note@+1 {{called by 'kernel_single_task'}} use2(ab, p); } int main() { a_type ab; - // expected-note@+1 {{called by 'operator()'}} kernel_single_task([]() { usage( &addInt ); }); return 0; } From 120595a1527e07a5540fe8a0d391dd1eb9cef084 Mon Sep 17 00:00:00 2001 From: Erich Keane Date: Fri, 17 Jan 2020 06:38:52 -0800 Subject: [PATCH 2/2] [SYCL] Add test to validate the call stack, as Mariya requested. Signed-off-by: Erich Keane --- clang/test/SemaSYCL/sycl-callstack.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 clang/test/SemaSYCL/sycl-callstack.cpp diff --git a/clang/test/SemaSYCL/sycl-callstack.cpp b/clang/test/SemaSYCL/sycl-callstack.cpp new file mode 100644 index 0000000000000..2c96efe242791 --- /dev/null +++ b/clang/test/SemaSYCL/sycl-callstack.cpp @@ -0,0 +1,18 @@ +// RUN: %clang_cc1 -fcxx-exceptions -fsycl-is-device -verify -fsyntax-only -std=c++17 %s + +template +__attribute__((sycl_kernel)) +void kernel_single_task(Func kernelFunc) { + // expected-note@+1 {{called by 'kernel_single_task}} + kernelFunc(); +} + +void foo() { + // expected-error@+1 {{SYCL kernel cannot use exceptions}} + throw 3; +} + +int main() { +// expected-note@+1 {{called by 'operator()'}} +kernel_single_task([]() { foo(); }); +}