Output OpenCL version in Clang diagnostics

[rivanvx]

Summary

Whether OpenCL does or does not support the static storage class specifier and generic type qualifier is version dependent and diagnostics should note that.

This patch changes "OpenCL does not support" to "OpenCL version X.Y does not support" and also changes one message that contains "OpenCL2.0 version" to contain "OpenCL version 2.0" for consistent version reporting across all diagnostics including OpenCL version.

[rivanvx]

Fixed wrong search and replace in tests.

[rivanvx]

Fix formatting.

[yaxunl]

LGTM

[Anastasia]

lib/Parse/ParseDecl.cpp

3514

Could we use StringRef instead to make this a big cleaner?

Perhaps, we could even use to_string instead of Twine...

[Anastasia]

Please, also add cfe-commits as Subscriber.

Thanks!
Anastasia

[rivanvx]

Replace Twine with std::string and use llvm::to_string().

[Anastasia]

lib/Parse/ParseDecl.cpp

3516

I think it will be nicer to use string (not char*) here too.

[rivanvx]

lib/Parse/ParseDecl.cpp

3516

Other Spec are const char*, so I did it for consistency. But I don't care either way.

[rivanvx]

@Anastasia would you still prefer to make VerSpec a std::string?

[Anastasia]

lib/Parse/ParseDecl.cpp

3516

Yes, it will be shorter. StringRef should work too I think.

Thanks!

[rivanvx]

I am neither aware how to convert ints to StringRef nor how to concatenate StringRefs. Apologies if I missed something in the API.

In any case, this approach looks pretty clean to me.

[rivanvx]

Make that int const as well.

[pxli168]

LGTM, thanks!

[Anastasia]

LGTM! Thanks!

[Anastasia]

Please, move cfe-commits to Subscribers list!

[rivanvx]

Thanks for the reviews!

[rivanvx]

Please, can anyone push this?

[Anastasia]

Sure! Will do! Thanks!

I am thinking to factor out the version computation string into a common function, because we might use it in the other places too.

[rivanvx]

Could we solve that at a later point? There is one more place where such code is already used, but this would enlarge the scope of this patch.

If yes, I am wiling to factor it out after this is merged.

[Anastasia]

Sure. Committed in r269305!

Thanks!

[rivanvx]

@Anastasia I looked into introducing a separate getOpenCLVersion() function (or perhaps three - major version, minor version and version string). This would have to be used in lib/CodeGen/TargetInfo.cpp and lib/Parse/ParseDecl.cpp, and I am undecided on where should one put this code. One option would be in Parse/Parser.h inside class Parser, and then TargetInfo.cpp would have to include Parser.h, unless we decide to declare it inside AST/ASTContext.h.

In any case, this has so far two usages, and they are different (major and minor version in TargetInfo.cpp vs version string in ParseDecl.cpp). Therefore, I would propose to leave this as is for now, and rethink it after the same code has to be used in more places.

[Anastasia]

In D19780#442305, @rivanvx wrote:

@Anastasia I looked into introducing a separate getOpenCLVersion() function (or perhaps three - major version, minor version and version string). This would have to be used in lib/CodeGen/TargetInfo.cpp and lib/Parse/ParseDecl.cpp, and I am undecided on where should one put this code. One option would be in Parse/Parser.h inside class Parser, and then TargetInfo.cpp would have to include Parser.h, unless we decide to declare it inside AST/ASTContext.h.

In any case, this has so far two usages, and they are different (major and minor version in TargetInfo.cpp vs version string in ParseDecl.cpp). Therefore, I would propose to leave this as is for now, and rethink it after the same code has to be used in more places.

Sure! It seems to make sense to me to have more use cases first. ;) Thanks for looking at it!

[Anastasia]

r269305

include/clang/Basic/DiagnosticSemaKinds.td

def err_opencl_builtin_pipe_invalid_arg : Error<
   "invalid argument type to function %0 (expecting %1 having %2)">;
 def err_opencl_builtin_pipe_invalid_access_modifier : Error<
   "invalid pipe access modifier (expecting %0)">;
 
 // OpenCL access qualifier
 def err_opencl_invalid_access_qualifier : Error<
   "access qualifier can only be used for pipe and image type">;
 def err_opencl_invalid_read_write : Error<
-  "access qualifier %0 can not be used for %1 %select{|earlier than OpenCL2.0 version}2">;
+  "access qualifier %0 can not be used for %1 %select{|earlier than OpenCL version 2.0}2">;
 def err_opencl_multiple_access_qualifiers : Error<
   "multiple access qualifiers">;
 
 // OpenCL Section 6.8.g
 def err_opencl_unknown_type_specifier : Error<
-  "OpenCL does not support the '%0' %select{type qualifier|storage class specifier}1">;
+  "OpenCL version %0 does not support the '%1' %select{type qualifier|storage class specifier}2">;
 
 // OpenCL v2.0 s6.12.5 Blocks restrictions
 def err_opencl_block_storage_type : Error<
   "the __block storage type is not permitted">;
 def err_opencl_invalid_block_declaration : Error<
   "invalid block variable declaration - must be %select{const qualified|initialized}0">;
 def err_opencl_extern_block_declaration : Error<
   "invalid block variable declaration - using 'extern' storage class is disallowed">;

lib/Parse/ParseDecl.cpp

 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/PrettyDeclStackTrace.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/SemaDiagnostic.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/ScopedPrinter.h"
 
 using namespace clang;
 
 //===----------------------------------------------------------------------===//
 // C99 6.7: Declarations.
 //===----------------------------------------------------------------------===//
 
 /// ParseTypeName
#include "clang/Basic/OpenCLImageTypes.def"
     // If the specifier wasn't legal, issue a diagnostic.
     if (isInvalid) {
       assert(PrevSpec && "Method did not return previous specifier!");
       assert(DiagID);
 
       if (DiagID == diag::ext_duplicate_declspec)
         Diag(Tok, DiagID)
           << PrevSpec << FixItHint::CreateRemoval(Tok.getLocation());
-      else if (DiagID == diag::err_opencl_unknown_type_specifier)
-        Diag(Tok, DiagID) << PrevSpec << isStorageClass;
-      else
+      else if (DiagID == diag::err_opencl_unknown_type_specifier) {
+        const int OpenCLVer = getLangOpts().OpenCLVersion;
+        std::string VerSpec = llvm::to_string(OpenCLVer / 100) +
+                              std::string (".") +
+                              llvm::to_string((OpenCLVer % 100) / 10);
+        Diag(Tok, DiagID) << VerSpec << PrevSpec << isStorageClass;
+      } else
         Diag(Tok, DiagID) << PrevSpec;
     }
 
     DS.SetRangeEnd(Tok.getLocation());
     if (DiagID != diag::err_bool_redeclaration)
       ConsumeToken();
 
     AttrsLastTime = false;

test/Parser/opencl-cl20.cl

 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL2.0 -DCL20
 
 #ifdef CL20
 // expected-no-diagnostics
 #endif
 
 __generic int * __generic_test(__generic int *arg) {
   __generic int *var;
   return var;  
 }
 #ifndef CL20
-// expected-error@-5 {{OpenCL does not support the '__generic' type qualifier}}
-// expected-error@-6 {{OpenCL does not support the '__generic' type qualifier}}
-// expected-error@-6 {{OpenCL does not support the '__generic' type qualifier}}
+// expected-error@-5 {{OpenCL version 1.0 does not support the '__generic' type qualifier}}
+// expected-error@-6 {{OpenCL version 1.0 does not support the '__generic' type qualifier}}
+// expected-error@-6 {{OpenCL version 1.0 does not support the '__generic' type qualifier}}
 #endif
 
 generic int * generic_test(generic int *arg) {
   generic int *var;
   return var;  
 }
 #ifndef CL20
-// expected-error@-5 {{OpenCL does not support the 'generic' type qualifier}}
-// expected-error@-6 {{OpenCL does not support the 'generic' type qualifier}}
-// expected-error@-6 {{OpenCL does not support the 'generic' type qualifier}}
+// expected-error@-5 {{OpenCL version 1.0 does not support the 'generic' type qualifier}}
+// expected-error@-6 {{OpenCL version 1.0 does not support the 'generic' type qualifier}}
+// expected-error@-6 {{OpenCL version 1.0 does not support the 'generic' type qualifier}}
 #endif

test/Parser/opencl-storage-class.cl

 // RUN: %clang_cc1 %s -verify -fsyntax-only
 
 void test_storage_class_specs()
 {
-  static int a;    // expected-error {{OpenCL does not support the 'static' storage class specifier}}
-  register int b;  // expected-error {{OpenCL does not support the 'register' storage class specifier}}
-  extern int c;    // expected-error {{OpenCL does not support the 'extern' storage class specifier}}
-  auto int d;      // expected-error {{OpenCL does not support the 'auto' storage class specifier}}
+  static int a;    // expected-error {{OpenCL version 1.0 does not support the 'static' storage class specifier}}
+  register int b;  // expected-error {{OpenCL version 1.0 does not support the 'register' storage class specifier}}
+  extern int c;    // expected-error {{OpenCL version 1.0 does not support the 'extern' storage class specifier}}
+  auto int d;      // expected-error {{OpenCL version 1.0 does not support the 'auto' storage class specifier}}
 
 #pragma OPENCL EXTENSION cl_clang_storage_class_specifiers : enable
   static int e; // expected-error {{static local variable must reside in constant address space}}
   register int f;
   extern int g; // expected-error {{extern variable must reside in constant address space}}
   auto int h;
 }

test/SemaOpenCL/invalid-access-qualifier.cl

 // RUN: %clang_cc1 -verify %s
 // RUN: %clang_cc1 -verify -cl-std=CL2.0 -DCL20 %s
 
 void test1(read_only int i){} // expected-error{{access qualifier can only be used for pipe and image type}}
 
 void test2(read_only write_only image1d_t i){} // expected-error{{multiple access qualifiers}}
 
 void test3(read_only read_only image1d_t i){} // expected-error{{multiple access qualifiers}}
 
 #ifdef CL20
 void test4(read_write pipe int i){} // expected-error{{access qualifier 'read_write' can not be used for 'pipe int'}}
 #else
-void test4(__read_write image1d_t i) {} // expected-error{{access qualifier '__read_write' can not be used for '__read_write image1d_t' earlier than OpenCL2.0 version}}
+void test4(__read_write image1d_t i) {} // expected-error{{access qualifier '__read_write' can not be used for '__read_write image1d_t' earlier than OpenCL version 2.0}}
 #endif

test/SemaOpenCL/storageclass.cl

 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL1.2
 
 static constant int G1 = 0;
 constant int G2 = 0;
 int G3 = 0;        // expected-error{{program scope variable must reside in constant address space}}
 global int G4 = 0; // expected-error{{program scope variable must reside in constant address space}}
 
 void kernel foo() {
   // static is not allowed at local scope before CL2.0
   static int S1 = 5;          // expected-error{{variables in function scope cannot be declared static}}
   static constant int S2 = 5; // expected-error{{variables in function scope cannot be declared static}}
 
   constant int L1 = 0;
   local int L2;
 
-  auto int L3 = 7; // expected-error{{OpenCL does not support the 'auto' storage class specifier}}
+  auto int L3 = 7; // expected-error{{OpenCL version 1.2 does not support the 'auto' storage class specifier}}
   global int L4;   // expected-error{{function scope variable cannot be declared in global address space}}
 }
 
 static void kernel bar() { // expected-error{{kernel functions cannot be declared static}}
 }
 
 void f() {
   constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
   local int L2;        // expected-error{{non-kernel function variable cannot be declared in local address space}}
   {
     constant int L1 = 0; // expected-error{{non-kernel function variable cannot be declared in constant address space}}
     local int L2;        // expected-error{{non-kernel function variable cannot be declared in local address space}}
   }
   global int L3; // expected-error{{function scope variable cannot be declared in global address space}}
   extern constant float L4;
 }