[OpenCL] Added diagnostic for implicit declaration of function in OpenCL

[yaxunl]

What happens if user declared a function without parameter as void f(); instead of void f(void) then try to use it? Will this be treated as implicit declaration and results in an error instead of warning?

[Anastasia]

include/clang/Basic/DiagnosticSemaKinds.td

8317

Could this be in OpenCL group please?

test/SemaOpenCL/clang-builtin-version.cl

68

Why do we get this note now? I believe work_group_reserve_read_pipe shouldn't be available either?

[echuraev]

include/clang/Basic/DiagnosticSemaKinds.td

8317

But this is already in OpenCL group. Please, see line 8232. In this line is a comment that OpenCL warnings and errors are below.

test/SemaOpenCL/clang-builtin-version.cl

68

May be I don't understand something but I think that it is the right diagnostic message. We called work_group_reserve_read_pipe in line 29. So for this case we will get the following message:
clang-builtin-version.cl: 31 error: implicit declaration of function 'work_group_reserve_write_pipe' is invalid in OpenCL
clang-builtin-version.cl: 31 note: did you mean 'work_group_reserve_read_pipe'?
clang-builtin-version.cl: 29 note: 'work_group_reserve_read_pipe' declared here

[Anastasia]

include/clang/Basic/DiagnosticSemaKinds.td

8317

Ah yes! Sorry, I got confused by the absence of opencl in the neighboring diagnostics. :)

test/SemaOpenCL/clang-builtin-version.cl

68

But there is an error now given for the call to 'work_group_reserve_read_pipe'. Why is it still added to the declarations? I think we should prevent this.

[Anastasia]

test/SemaOpenCL/clang-builtin-version.cl

68

Also do you know why we didn't have these notes before? I don't see anything related in your change.

[echuraev]

test/SemaOpenCL/clang-builtin-version.cl

68

I run this test and got a log with errors. The full log you can find in the end of the message.
I think that the diagnostic messages are right. First error we get for implicit declaration of work_group_reserve_read_pipe. After that we call function work_group_reserve_write_pipe and get the same error about implicit declaration of function. But also we get two diagnostic messages. Compiler already know about work_group_reserve_read_pipe and make a hypothesis: "note: did you mean 'work_group_reserve_read_pipe'?" and "note: 'work_group_reserve_read_pipe' declared here".
As well, next we have declaration of work_group_commit_read_pipe. And we get an error about implicit declaration of this function and also get a note messages: "note: did you mean 'work_group_reserve_read_pipe'?" and "note: 'work_group_reserve_read_pipe' declared here".

Also do you know why we didn't have these notes before? I don't see anything related in your change.

Unfortunately, I don't know why we get these notes. They have appeared when I changed the status of diagnostic message for implicit declaration from warning to error. If we had only warning messages then we wouldn't have these notes.

clang-builtin-version.cl:35:3: error: implicit declaration of function 'work_group_reserve_read_pipe' is invalid in OpenCL
  work_group_reserve_read_pipe(tmp, tmp);
  ^

clang-builtin-version.cl:37:3: error: implicit declaration of function 'work_group_reserve_write_pipe' is invalid in OpenCL
  work_group_reserve_write_pipe(tmp, tmp);
  ^
clang-builtin-version.cl:37:3: note: did you mean 'work_group_reserve_read_pipe'?
clang-builtin-version.cl:35:3: note: 'work_group_reserve_read_pipe' declared here
  work_group_reserve_read_pipe(tmp, tmp);
  ^

clang-builtin-version.cl:50:3: error: implicit declaration of function 'work_group_commit_read_pipe' is invalid in OpenCL
  work_group_commit_read_pipe(tmp, tmp);
  ^
clang-builtin-version.cl:50:3: note: did you mean 'work_group_reserve_read_pipe'?
clang-builtin-version.cl:35:3: note: 'work_group_reserve_read_pipe' declared here
  work_group_reserve_read_pipe(tmp, tmp);
  ^

[Anastasia]

test/SemaOpenCL/clang-builtin-version.cl

68

I find those notes counterintuitive to be honest because if there is an error given for the function declaration - let's say foo(), it shouldn't have been added anywhere to any dictionary because if the next function let's say foa() is corrected to match the name from the note (i.e. foo()) it is already known that it won't be compiled too. So to me this note seems wrong because it attempts to correct to something which is already known not to be compiled. I think it's worth digging into it a bit more...

[echuraev]

I disabled adding note diagnostic for OpenCL. Addition function to dictionary occurs in function CurrectTypo. In this function take place creation of consumer by calling function makeTypoCorrectionConsumer. The function do some checks and next create consumer by the following code:

auto Consumer = llvm::make_unique<TypoCorrectionConsumer>(
     *this, TypoName, LookupKind, S, SS, std::move(CCC), MemberContext,
     EnteringContext);

In constructor TypoCorrectionConsumer occurs adding correction to dictionary (ValidatedCorrections).

[Anastasia]

lib/Sema/SemaDecl.cpp

12526

This way prevents the typo corrections completely for OpenCL which is not very desirable. I was just wondering could we prevent adding the invalid builtin function identifiers instead to the correction candidate list.

Like when work_group_reserve_read_pipe was first parsed it shouldn't have been added to the list of valid function identifiers to appear in the corrections of 'work_group_reserve_write_pipe'. I am guessing the identifier might be added when builtins are initialized...

[echuraev]

lib/Sema/SemaDecl.cpp

12526

Yes, sorry, I didn't think about it. I investigated the question how can I remove invalid functions from correction candidate list. But I have a problem and I hope that you will be able to help me.

I found that the correction is added in function void TypoCorrectionConsumer::addName (in file SemaLookup.cpp) which called from loop in function std::unique_ptr<TypoCorrectionConsumer> Sema::makeTypoCorrectionConsumer. The loop you can see below:

// For unqualified lookup, look through all of the names that we have       
// seen in this translation unit.                                           
// FIXME: Re-add the ability to skip very unlikely potential corrections.   
for (const auto &I : Context.Idents)                                        
  Consumer->FoundName(I.getKey());

But the map Context.Idents already contains names of implicit functions. So, I found that names of functions were added to this dictionary during parsing AST. After calling ConsumeToken() function in void Parser::ParseDeclarationSpecifiers (in file ParseDecl.cpp) implicit functions were added to the dictionary. But in this function I'm not able to check is the OpenCL function implicit declared or not.

As a result I tried to remove names of implicit functions before calling CorrectTypo. But unfortunately, we don't have an API for removing items from Context.Idents. So, I don't know the good way for fixing these diagnostic messages. Could you help me please? Do you have any suggestions?

Thank you in advance!

[Anastasia]

lib/Sema/SemaDecl.cpp

12526

Thanks for looking at this!

It seems like we are adding the identifiers to the map too early. So we could:
(a) postpose adding items into Context.Idents to Sema.
(b) extend IdentifierTable interface to support removing items (although it might not align well with the general concept).

Although (b) feels more natural way conceptually, it might require bigger changes than (a).

Both need discussion in cfe-dev to continue, so it should be addressed in isolation. If you undo the last change, I am happy to approve the review.

[Anastasia]

LGTM! Thanks!

include/clang/Basic/DiagnosticSemaKinds.td

 def err_static_function_scope : Error<
   "variables in function scope cannot be declared static">;
 def err_opencl_bitfields : Error<
   "bit-fields are not supported in OpenCL">;
 def err_opencl_vla : Error<
   "variable length arrays are not supported in OpenCL">;
 def err_bad_kernel_param_type : Error<
   "%0 cannot be used as the type of a kernel parameter">;
+def err_opencl_implicit_function_decl : Error<
+  "implicit declaration of function %0 is invalid in OpenCL">;
 def err_record_with_pointers_kernel_param : Error<
   "%select{struct|union}0 kernel parameters may not contain pointers">;
 def note_within_field_of_type : Note<
   "within field of type %0 declared here">;
 def note_illegal_field_declared_here : Note<
   "field of illegal %select{type|pointer type}0 %1 declared here">;
 def err_opencl_type_struct_or_union_field : Error<
   "the %0 type cannot be used to declare a structure or union field">;

lib/Sema/SemaDecl.cpp

if (NamedDecl *ExternCPrev = findLocallyScopedExternCDecl(&II)) {
     Diag(ExternCPrev->getLocation(), diag::note_previous_declaration);
     return ExternCPrev;
   }
 
   // Extension in C99.  Legal in C90, but warn about it.
   unsigned diag_id;
   if (II.getName().startswith("__builtin_"))
     diag_id = diag::warn_builtin_unknown;
+  // OpenCL v2.0 s6.9.u - Implicit function declaration is not supported.
+  else if (getLangOpts().OpenCL)
+    diag_id = diag::err_opencl_implicit_function_decl;
   else if (getLangOpts().C99)
     diag_id = diag::ext_implicit_function_decl;
   else
     diag_id = diag::warn_implicit_function_decl;
   Diag(Loc, diag_id) << &II;
 
   // Because typo correction is expensive, only do it if the implicit
   // function declaration is going to be treated as an error.
   if (Diags.getDiagnosticLevel(diag_id, Loc) >= DiagnosticsEngine::Error) {
     TypoCorrection Corrected;
     if (S &&
         (Corrected = CorrectTypo(
              DeclarationNameInfo(&II, Loc), LookupOrdinaryName, S, nullptr,
              llvm::make_unique<DeclFilterCCC<FunctionDecl>>(), CTK_NonError)))
       diagnoseTypo(Corrected, PDiag(diag::note_function_suggestion),
                    /*ErrorRecovery*/false);
   }

test/SemaOpenCL/clang-builtin-version.cl

 // RUN: %clang_cc1 %s -fblocks -verify -pedantic -fsyntax-only -ferror-limit 100
 
 // Confirm CL2.0 Clang builtins are not available in earlier versions
 
 kernel void dse_builtins() {
   int tmp;
-  enqueue_kernel(tmp, tmp, tmp, ^(void) { // expected-warning{{implicit declaration of function 'enqueue_kernel' is invalid in C99}}
+  enqueue_kernel(tmp, tmp, tmp, ^(void) { // expected-error{{implicit declaration of function 'enqueue_kernel' is invalid in OpenCL}}
     return;
   });
-  unsigned size = get_kernel_work_group_size(^(void) { // expected-warning{{implicit declaration of function 'get_kernel_work_group_size' is invalid in C99}}
+  unsigned size = get_kernel_work_group_size(^(void) { // expected-error{{implicit declaration of function 'get_kernel_work_group_size' is invalid in OpenCL}}
     return;
   });
-  size = get_kernel_preferred_work_group_size_multiple(^(void) { // expected-warning{{implicit declaration of function 'get_kernel_preferred_work_group_size_multiple' is invalid in C99}}
+  size = get_kernel_preferred_work_group_size_multiple(^(void) { // expected-error{{implicit declaration of function 'get_kernel_preferred_work_group_size_multiple' is invalid in OpenCL}}
     return;
   });
 }
 
 void pipe_builtins() {
   int tmp;
 
-  read_pipe(tmp, tmp);  // expected-warning{{implicit declaration of function 'read_pipe' is invalid in C99}}
-  write_pipe(tmp, tmp); // expected-warning{{implicit declaration of function 'write_pipe' is invalid in C99}}
-
-  reserve_read_pipe(tmp, tmp);  // expected-warning{{implicit declaration of function 'reserve_read_pipe' is invalid in C99}}
-  reserve_write_pipe(tmp, tmp); // expected-warning{{implicit declaration of function 'reserve_write_pipe' is invalid in C99}}
-
-  work_group_reserve_read_pipe(tmp, tmp);  // expected-warning{{implicit declaration of function 'work_group_reserve_read_pipe' is invalid in C99}}
-  work_group_reserve_write_pipe(tmp, tmp); // expected-warning{{implicit declaration of function 'work_group_reserve_write_pipe' is invalid in C99}}
-
-  sub_group_reserve_write_pipe(tmp, tmp); // expected-warning{{implicit declaration of function 'sub_group_reserve_write_pipe' is invalid in C99}}
-  sub_group_reserve_read_pipe(tmp, tmp);  // expected-warning{{implicit declaration of function 'sub_group_reserve_read_pipe' is invalid in C99}}
-
-  commit_read_pipe(tmp, tmp);  // expected-warning{{implicit declaration of function 'commit_read_pipe' is invalid in C99}}
-  commit_write_pipe(tmp, tmp); // expected-warning{{implicit declaration of function 'commit_write_pipe' is invalid in C99}}
-
-  work_group_commit_read_pipe(tmp, tmp);  // expected-warning{{implicit declaration of function 'work_group_commit_read_pipe' is invalid in C99}}
-  work_group_commit_write_pipe(tmp, tmp); // expected-warning{{implicit declaration of function 'work_group_commit_write_pipe' is invalid in C99}}
-
-  sub_group_commit_write_pipe(tmp, tmp); // expected-warning{{implicit declaration of function 'sub_group_commit_write_pipe' is invalid in C99}}
-  sub_group_commit_read_pipe(tmp, tmp);  // expected-warning{{implicit declaration of function 'sub_group_commit_read_pipe' is invalid in C99}}
-
-  get_pipe_num_packets(tmp); // expected-warning{{implicit declaration of function 'get_pipe_num_packets' is invalid in C99}}
-  get_pipe_max_packets(tmp); // expected-warning{{implicit declaration of function 'get_pipe_max_packets' is invalid in C99}}
+  foo(void); // expected-error{{implicit declaration of function 'foo' is invalid in OpenCL}}
+  // expected-note@-1{{'foo' declared here}}
+  // expected-error@-2{{expected expression}}
+  boo(); // expected-error{{implicit declaration of function 'boo' is invalid in OpenCL}}
+  // expected-note@-1{{did you mean 'foo'?}}
+
+  read_pipe(tmp, tmp);  // expected-error{{implicit declaration of function 'read_pipe' is invalid in OpenCL}}
+  write_pipe(tmp, tmp); // expected-error{{implicit declaration of function 'write_pipe' is invalid in OpenCL}}
+
+  reserve_read_pipe(tmp, tmp);  // expected-error{{implicit declaration of function 'reserve_read_pipe' is invalid in OpenCL}}
+  // expected-note@-1{{'reserve_read_pipe' declared here}}
+  reserve_write_pipe(tmp, tmp); // expected-error{{implicit declaration of function 'reserve_write_pipe' is invalid in OpenCL}}
+  // expected-note@-1{{did you mean 'reserve_read_pipe'?}}
+
+  work_group_reserve_read_pipe(tmp, tmp);  // expected-error{{implicit declaration of function 'work_group_reserve_read_pipe' is invalid in OpenCL}}
+  // expected-note@-1 2{{'work_group_reserve_read_pipe' declared here}}
+  work_group_reserve_write_pipe(tmp, tmp); // expected-error{{implicit declaration of function 'work_group_reserve_write_pipe' is invalid in OpenCL}}
+  // expected-note@-1{{did you mean 'work_group_reserve_read_pipe'?}}
+  // expected-note@-2{{'work_group_reserve_write_pipe' declared here}}
+
+  sub_group_reserve_write_pipe(tmp, tmp); // expected-error{{implicit declaration of function 'sub_group_reserve_write_pipe' is invalid in OpenCL}}
+  // expected-note@-1{{did you mean 'work_group_reserve_write_pipe'?}}
+  sub_group_reserve_read_pipe(tmp, tmp);  // expected-error{{implicit declaration of function 'sub_group_reserve_read_pipe' is invalid in OpenCL}}
+
+  commit_read_pipe(tmp, tmp);  // expected-error{{implicit declaration of function 'commit_read_pipe' is invalid in OpenCL}}
+  // expected-note@-1{{'commit_read_pipe' declared here}}
+  commit_write_pipe(tmp, tmp); // expected-error{{implicit declaration of function 'commit_write_pipe' is invalid in OpenCL}}
+  // expected-note@-1{{did you mean 'commit_read_pipe'?}}
+
+  work_group_commit_read_pipe(tmp, tmp);  // expected-error{{implicit declaration of function 'work_group_commit_read_pipe' is invalid in OpenCL}}
+  // expected-note@-1{{'work_group_commit_read_pipe' declared here}}
+  // expected-note@-2{{did you mean 'work_group_reserve_read_pipe'?}}
+  work_group_commit_write_pipe(tmp, tmp); // expected-error{{implicit declaration of function 'work_group_commit_write_pipe' is invalid in OpenCL}}
+  // expected-note@-1{{'work_group_commit_write_pipe' declared here}}
+  // expected-note@-2{{did you mean 'work_group_commit_read_pipe'?}}
+
+  sub_group_commit_write_pipe(tmp, tmp); // expected-error{{implicit declaration of function 'sub_group_commit_write_pipe' is invalid in OpenCL}}
+  // expected-note@-1{{did you mean 'work_group_commit_write_pipe'?}}
+  sub_group_commit_read_pipe(tmp, tmp);  // expected-error{{implicit declaration of function 'sub_group_commit_read_pipe' is invalid in OpenCL}}
+
+  get_pipe_num_packets(tmp); // expected-error{{implicit declaration of function 'get_pipe_num_packets' is invalid in OpenCL}}
+  // expected-note@-1{{'get_pipe_num_packets' declared here}}
+  get_pipe_max_packets(tmp); // expected-error{{implicit declaration of function 'get_pipe_max_packets' is invalid in OpenCL}}
+  // expected-note@-1{{did you mean 'get_pipe_num_packets'?}}
 }

test/SemaOpenCL/to_addr_builtin.cl

 // RUN: %clang_cc1 -verify -fsyntax-only %s
 // RUN: %clang_cc1 -verify -fsyntax-only -cl-std=CL2.0 %s
 
 void test(void) {
   global int *glob;
   local int *loc;
   constant int *con;
   typedef constant int const_int_ty;
   const_int_ty *con_typedef;
 
   glob = to_global(glob, loc);
 #if __OPENCL_C_VERSION__ < CL_VERSION_2_0
-  // expected-warning@-2{{implicit declaration of function 'to_global' is invalid in C99}}
+  // expected-error@-2{{implicit declaration of function 'to_global' is invalid in OpenCL}}
   // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *' from 'int'}}
 #else
   // expected-error@-5{{invalid number of arguments to function: 'to_global'}}
 #endif
 
   int x;
   glob = to_global(x);
 #if __OPENCL_C_VERSION__ < CL_VERSION_2_0