[C++4OpenCL] Add missing OpenCL specific diagnostics in templates

[olestrohm]

Summary

For templates diagnosis must be done after specialization instead of during parsing.

I've left the check during parsing as well, because it provides slightly better error messages, though some are lost now that NewVD is set as invalid, instead of just setting D invalid.

This fixes: https://bugs.llvm.org/show_bug.cgi?id=48887

[Anastasia]

I've left the check during parsing as well, because it provides slightly better error messages, though some are lost now that NewVD is set as invalid, instead of just setting D invalid.

Yes, when there are multiple errors we don't provide the diagnostics consistently so I think it's fine.

clang/lib/Sema/SemaDecl.cpp

6827

I find this function a bit confusing I would just move its body inline where it is currently called.

Although actually could this code be moved to where we do similar check for static or extern
https://clang.llvm.org/doxygen/SemaDecl_8cpp_source.html#l07911

You would then need to change this slightly i.e. to call VarDecl::getTSCSpec instead.

clang/test/SemaOpenCLCXX/template-diagnostics.clcpp

27 ↗

(On Diff #338876)

Let's move this into the OpenCL C test for clk_event_t diagnostic test/SemaOpenCL/clk_event_t.cl. I would add a FIXME comment next to it stating that it is not clear whether this should be diagnosed or not.

[olestrohm]

Inlined the thread_local check and moved static clk_event_t into the appropriate test.
I did not move the thread_local check since NewVD had the wrong value for TSCS, so D was required.

[olestrohm]

Ran git-clang-format.

[Anastasia]

LGTM, let's just rename template-diagnostics.clcpp into something like template-opencl-types.clcpp.

[olestrohm]

Renamed test as suggested.

[svenvh]

I've left the check during parsing as well, because it provides slightly better error messages

Do you have an example of what error messages get worse if you remove the parsing check? Just wondering if there is an easy way to fix that and avoid having the check twice.

[olestrohm]

The only ones that change (in the test cases at least) are as follows:
Here Old is with the current change, and New is with the call to diagnoseOpenCLTypes at parsing removed.

In event_t.cl, event_t glb_evt; in program scope has this difference:

Old:
  the '__private event_t' type cannot be used to declare a program scope variable
New:
  program scope variable must reside in constant address space

In template-opencl-types.clcpp, __local event_t e; inside a function has this difference:

Old:
  the event_t type can only be used with __private address space qualifier
New:
  non-kernel function variable cannot be declared in local address space

The first on explains that event_t can't be used in program scope at all, which is better I think.
However the second error actually gets slightly better.

I think these errors are primarily emitted between the two checks, so I don't know if it's easy to put the check somewhere that gives better results.

[svenvh]

In D100860#2709057, @olestrohm wrote:

The first on explains that event_t can't be used in program scope at all, which is better I think.

Agreed, removing the parser check gives a worse diagnostic here.

The underlying problem might actually be that the diagnostic for program scope variables is firing too early. I tried to address a similar issue with D53705, which got stalled unfortunately. Perhaps we should revisit that and try to see if postponing the program scope variable diagnostic allows us to remove the parser check without getting a worse diagnostic.

[olestrohm]

I noticed that I could move the new check above the diagnostic for program scope variables, and the check during parsing can then be removed, while maintaining good diagnostics.

This does change the diagnostic for event_t mentioned above, however it's not a bad diagnostic, and actually matches what happens in non-kernel functions, so I think it's okay.
I would prefer the previous diagnostic for event_t, but that originates in the other branch of the same if statement as the program scope diagnostic, so it's not easy to separate them out and then insert the new check between them.

[svenvh]

LGTM, thanks!

Please make sure that the commit message corresponds to the new shape of your patch: you might want to update the description of this review.

clang/lib/Sema/SemaDecl.cpp

 static bool isDeclExternC(const Decl *D) {
   if (const auto *FD = dyn_cast<FunctionDecl>(D))
     return FD->isExternC();
   if (const auto *VD = dyn_cast<VarDecl>(D))
     return VD->isExternC();
 
   llvm_unreachable("Unknown type of decl!");
 }
+
 /// Returns true if there hasn't been any invalid type diagnosed.
-static bool diagnoseOpenCLTypes(Scope *S, Sema &Se, Declarator &D,
-                                DeclContext *DC, QualType R) {
+static bool diagnoseOpenCLTypes(Sema &Se, VarDecl *NewVD) {
+  DeclContext *DC = NewVD->getDeclContext();
+  QualType R = NewVD->getType();
+
   // OpenCL v2.0 s6.9.b - Image type can only be used as a function argument.
   // OpenCL v2.0 s6.13.16.1 - Pipe type can only be used as a function
   // argument.
   if (R->isImageType() || R->isPipeType()) {
-    Se.Diag(D.getIdentifierLoc(),
+    Se.Diag(NewVD->getLocation(),
             diag::err_opencl_type_can_only_be_used_as_function_parameter)
         << R;
-    D.setInvalidType();
+    NewVD->setInvalidDecl();
     return false;
   }
 
   // OpenCL v1.2 s6.9.r:
   // The event type cannot be used to declare a program scope variable.
   // OpenCL v2.0 s6.9.q:
   // The clk_event_t and reserve_id_t types cannot be declared in program
   // scope.
-  if (NULL == S->getParent()) {
+  if (NewVD->hasGlobalStorage() && !NewVD->isStaticLocal()) {
     if (R->isReserveIDT() || R->isClkEventT() || R->isEventT()) {
-      Se.Diag(D.getIdentifierLoc(),
+      Se.Diag(NewVD->getLocation(),
               diag::err_invalid_type_for_program_scope_var)
           << R;
-      D.setInvalidType();
+      NewVD->setInvalidDecl();
       return false;
     }
   }
 
   // OpenCL v1.0 s6.8.a.3: Pointers to functions are not allowed.
   if (!Se.getOpenCLOptions().isAvailableOption("__cl_clang_function_pointers",
                                                Se.getLangOpts())) {
     QualType NR = R.getCanonicalType();
     while (NR->isPointerType() || NR->isMemberFunctionPointerType() ||
            NR->isReferenceType()) {
       if (NR->isFunctionPointerType() || NR->isMemberFunctionPointerType() ||
           NR->isFunctionReferenceType()) {
-        Se.Diag(D.getIdentifierLoc(), diag::err_opencl_function_pointer)
+        Se.Diag(NewVD->getLocation(), diag::err_opencl_function_pointer)
             << NR->isReferenceType();
-        D.setInvalidType();
+        NewVD->setInvalidDecl();
         return false;
       }
       NR = NR->getPointeeType();
     }
   }
 
   if (!Se.getOpenCLOptions().isAvailableOption("cl_khr_fp16",
                                                Se.getLangOpts())) {
     // OpenCL v1.2 s6.1.1.1: reject declaring variables of the half and
     // half array type (unless the cl_khr_fp16 extension is enabled).
     if (Se.Context.getBaseElementType(R)->isHalfType()) {
-      Se.Diag(D.getIdentifierLoc(), diag::err_opencl_half_declaration) << R;
-      D.setInvalidType();
+      Se.Diag(NewVD->getLocation(), diag::err_opencl_half_declaration) << R;
+      NewVD->setInvalidDecl();
       return false;
     }
   }
 
   // OpenCL v1.2 s6.9.r:
   // The event type cannot be used with the __local, __constant and __global
   // address space qualifiers.
   if (R->isEventT()) {
     if (R.getAddressSpace() != LangAS::opencl_private) {
-      Se.Diag(D.getBeginLoc(), diag::err_event_t_addr_space_qual);
-      D.setInvalidType();
+      Se.Diag(NewVD->getBeginLoc(), diag::err_event_t_addr_space_qual);
+      NewVD->setInvalidDecl();
       return false;
     }
   }
 
-  // C++ for OpenCL does not allow the thread_local storage qualifier.
-  // OpenCL C does not support thread_local either, and
-  // also reject all other thread storage class specifiers.
-  DeclSpec::TSCS TSC = D.getDeclSpec().getThreadStorageClassSpec();
-  if (TSC != TSCS_unspecified) {
-    bool IsCXX = Se.getLangOpts().OpenCLCPlusPlus;
-    Se.Diag(D.getDeclSpec().getThreadStorageClassSpecLoc(),
-            diag::err_opencl_unknown_type_specifier)
-        << IsCXX << Se.getLangOpts().getOpenCLVersionTuple().getAsString()
-        << DeclSpec::getSpecifierName(TSC) << 1;
-    D.setInvalidType();
-    return false;
-  }
-
   if (R->isSamplerT()) {
     // OpenCL v1.2 s6.9.b p4:
     // The sampler type cannot be used with the __local and __global address
     // space qualifiers.
     if (R.getAddressSpace() == LangAS::opencl_local ||
         R.getAddressSpace() == LangAS::opencl_global) {
-      Se.Diag(D.getIdentifierLoc(), diag::err_wrong_sampler_addressspace);
-      D.setInvalidType();
+      Se.Diag(NewVD->getLocation(), diag::err_wrong_sampler_addressspace);
+      NewVD->setInvalidDecl();
     }
 
     // OpenCL v1.2 s6.12.14.1:
     // A global sampler must be declared with either the constant address
     // space qualifier or with the const qualifier.
     if (DC->isTranslationUnit() &&
         !(R.getAddressSpace() == LangAS::opencl_constant ||
           R.isConstQualified())) {
-      Se.Diag(D.getIdentifierLoc(), diag::err_opencl_nonconst_global_sampler);
-      D.setInvalidType();
+      Se.Diag(NewVD->getLocation(), diag::err_opencl_nonconst_global_sampler);
+      NewVD->setInvalidDecl();
     }
-    if (D.isInvalidType())
+    if (NewVD->isInvalidDecl())
       return false;
   }
+
   return true;
 }
 
 template <typename AttrTy>
 static void copyAttrFromTypedefToDecl(Sema &S, Decl *D, const TypedefType *TT) {
   const TypedefNameDecl *TND = TT->getDecl();
   if (const auto *Attribute = TND->getAttr<AttrTy>()) {
     AttrTy *Clone = Attribute->clone(S.Context);
     Clone->setInherited(true);
     D->addAttr(Clone);
   }
 }
 
else {
       if (NewTemplate)
         NewTemplate->setModulePrivate();
       for (auto *B : Bindings)
         B->setModulePrivate();
     }
   }
 
   if (getLangOpts().OpenCL) {
-
     deduceOpenCLAddressSpace(NewVD);
 
-    diagnoseOpenCLTypes(S, *this, D, DC, NewVD->getType());
+    DeclSpec::TSCS TSC = D.getDeclSpec().getThreadStorageClassSpec();
+    if (TSC != TSCS_unspecified) {
+      bool IsCXX = getLangOpts().OpenCLCPlusPlus;
+      Diag(D.getDeclSpec().getThreadStorageClassSpecLoc(),
+           diag::err_opencl_unknown_type_specifier)
+          << IsCXX << getLangOpts().getOpenCLVersionTuple().getAsString()
+          << DeclSpec::getSpecifierName(TSC) << 1;
+      NewVD->setInvalidDecl();
+    }
   }
 
   // Handle attributes prior to checking for duplicates in MergeVarDecl
   ProcessDeclAttributes(S, NewVD, D);
 
   // FIXME: This is probably the wrong location to be doing this and we should
   // probably be doing this for more attributes (especially for function
   // pointer attributes such as format, warn_unused_result, etc.). Ideally
if (getLangOpts().OpenCLVersion == 120 &&
                                             getLangOpts()) &&
       NewVD->isStaticLocal()) {
     Diag(NewVD->getLocation(), diag::err_static_function_scope);
     NewVD->setInvalidDecl();
     return;
   }
 
   if (getLangOpts().OpenCL) {
+    if (!diagnoseOpenCLTypes(*this, NewVD))
+      return;
+
     // OpenCL v2.0 s6.12.5 - The __block storage type is not supported.
     if (NewVD->hasAttr<BlocksAttr>()) {
       Diag(NewVD->getLocation(), diag::err_opencl_block_storage_type);
       return;
     }
 
     if (T->isBlockPointerType()) {
       // OpenCL v2.0 s6.12.5 - Any block declaration must be const qualified and
       // can't use 'extern' storage class.
       if (!T.isConstQualified()) {
         Diag(NewVD->getLocation(), diag::err_opencl_invalid_block_declaration)
             << 0 /*const*/;
         NewVD->setInvalidDecl();
         return;
       }
       if (NewVD->hasExternalStorage()) {
         Diag(NewVD->getLocation(), diag::err_opencl_extern_block_declaration);
         NewVD->setInvalidDecl();
         return;
       }
     }
+
     // OpenCL C v1.2 s6.5 - All program scope variables must be declared in the
     // __constant address space.
     // OpenCL C v2.0 s6.5.1 - Variables defined at program scope and static
     // variables inside a function can also be declared in the global
     // address space.
     // C++ for OpenCL inherits rule from OpenCL C v2.0.
     // FIXME: Adding local AS in C++ for OpenCL might make sense.
     if (NewVD->isFileVarDecl() || NewVD->isStaticLocal() ||

clang/test/SemaOpenCL/clk_event_t.cl

 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL2.0
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=clc++
 
 // Taken from opencl-c.h
 #define CLK_NULL_EVENT (__builtin_astype(((__SIZE_MAX__)), clk_event_t))
 
 global clk_event_t ce; // expected-error {{the '__global clk_event_t' type cannot be used to declare a program scope variable}}
 
 int clk_event_tests() {
   event_t e;
   clk_event_t ce1;
   clk_event_t ce2;
   clk_event_t ce3 = CLK_NULL_EVENT;
 
+  // FIXME: Not obvious if this should give an error as if it was in program scope.
+  static clk_event_t ce4;
+
   if (e == ce1) { // expected-error {{invalid operands to binary expression ('__private event_t' and '__private clk_event_t')}}
     return 9;
   }
 
   if (ce1 != ce2) {
     return 1;
   }
   else if (ce1 == CLK_NULL_EVENT || ce2 != CLK_NULL_EVENT) {
     return 0;
   }
 
   return 2;
 }

clang/test/SemaOpenCL/event_t.cl

 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
 
-event_t glb_evt; // expected-error {{the '__private event_t' type cannot be used to declare a program scope variable}} expected-error{{program scope variable must reside in constant address space}}
+event_t glb_evt; // expected-error {{the '__private event_t' type cannot be used to declare a program scope variable}}
 
 constant struct evt_s {
   event_t evt; // expected-error {{the 'event_t' type cannot be used to declare a structure or union field}}
 } evt_str = {0};
 
 void foo(event_t evt); // expected-note {{passing argument to parameter 'evt' here}}
 
 void kernel ker(event_t argevt) { // expected-error {{'__private event_t' cannot be used as the type of a kernel parameter}}
   event_t e;
-  constant event_t const_evt; // expected-error {{the event_t type can only be used with __private address space qualifier}} expected-error{{variable in constant address space must be initialized}}
+  constant event_t const_evt; // expected-error{{the '__constant event_t' type cannot be used to declare a program scope variable}}
   foo(e);
   foo(0);
   foo(5); // expected-error {{passing 'int' to parameter of incompatible type 'event_t'}}
   foo((event_t)1); // expected-error {{cannot cast non-zero value '1' to 'event_t'}}
 }
 

clang/test/SemaOpenCL/sampler_t.cl

 
 constant struct sampler_s {
   sampler_t smp; // expected-error{{the 'sampler_t' type cannot be used to declare a structure or union field}}
 } sampler_str = {0};
 
 sampler_t bad(void); //expected-error{{declaring function return value of type 'sampler_t' is not allowed}}
 
 sampler_t global_nonconst_smp = 0; // expected-error {{global sampler requires a const or constant address space qualifier}}
-#ifdef CHECK_SAMPLER_VALUE
-// expected-warning@-2{{sampler initializer has invalid Filter Mode bits}}
-#endif
 
 const sampler_t glb_smp10 = CLK_ADDRESS_CLAMP_TO_EDGE | CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
 const constant sampler_t glb_smp11 = CLK_ADDRESS_CLAMP_TO_EDGE | CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
 
 void kernel ker(sampler_t argsmp) {
   local sampler_t smp; // expected-error{{sampler type cannot be used with the __local and __global address space qualifiers}}
   const sampler_t const_smp5 = 1.0f; // expected-error{{initializing 'const sampler_t' with an expression of incompatible type 'float'}}
   const sampler_t const_smp6 = 0x100000000LL; // expected-error{{sampler_t initialization requires 32-bit integer, not 'long long'}}

clang/test/SemaOpenCLCXX/template-opencl-types.clcpp

+// RUN: %clang_cc1 %s -pedantic -verify -fsyntax-only
+
+template<typename T>
+T global_variable; // expected-error{{the '__global clk_event_t' type cannot be used to declare a program scope variable}}
+
+clk_event_t global_event; // expected-error{{the '__global clk_event_t' type cannot be used to declare a program scope variable}}
+
+template<typename T>
+void templ() {
+  T loc;
+  // expected-error@-1{{type '__private __read_only image1d_t' can only be used as a function parameter in OpenCL}}
+  // expected-error@-2{{declaring variable of type '__private half' is not allowed}}
+  // expected-error@-3{{the event_t type can only be used with __private address space qualifier}}
+}
+
+void foo() {
+  templ<image1d_t>(); // expected-note{{in instantiation of function template specialization 'templ<__read_only image1d_t>' requested here}}
+  templ<half>(); // expected-note{{in instantiation of function template specialization 'templ<half>' requested here}}
+  templ<__local event_t>(); // expected-note{{in instantiation of function template specialization 'templ<__local event_t>' requested here}}
+
+  image1d_t img; // expected-error{{type '__private __read_only image1d_t' can only be used as a function parameter in OpenCL}}
+  half h; // expected-error{{declaring variable of type '__private half' is not allowed}}
+  __local event_t e; // expected-error{{the event_t type can only be used with __private address space qualifier}}
+
+  (void) global_variable<clk_event_t>; // expected-note{{in instantiation of variable template specialization 'global_variable' requested here}}
+}