Page MenuHomePhabricator

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

Authored by echuraev on Apr 6 2017, 1:56 AM.

Diff Detail

Event Timeline

echuraev created this revision.Apr 6 2017, 1:56 AM
yaxunl added a comment.Apr 6 2017, 7:28 AM

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 added inline comments.Apr 6 2017, 10:40 AM
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 updated this revision to Diff 94537.Apr 7 2017, 9:50 AM
echuraev added inline comments.
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 added inline comments.Apr 10 2017, 7:59 AM
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 added inline comments.Apr 10 2017, 8:05 AM
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 added inline comments.Apr 14 2017, 1:36 AM
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 added inline comments.Apr 19 2017, 11:05 AM
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 updated this revision to Diff 98958.May 15 2017, 1:46 AM

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).

echuraev marked an inline comment as done.May 15 2017, 1:46 AM
Anastasia added inline comments.May 15 2017, 9:56 AM
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 added inline comments.May 22 2017, 7:01 AM
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 added inline comments.May 24 2017, 11:39 AM
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.

echuraev updated this revision to Diff 100210.May 24 2017, 11:48 PM
Anastasia accepted this revision.May 26 2017, 6:34 AM

LGTM! Thanks!

This revision is now accepted and ready to land.May 26 2017, 6:34 AM
echuraev closed this revision.May 29 2017, 10:58 PM