Add check for matching HeaderFilter before emitting Diagnostic

[thorsten-klein]

Summary

Fixed issue of not considering HeaderFilter which resulted in findings although they should be filtered out.
Patch by Thorsten Klein.

[aaron.ballman]

Can you add test coverage that demonstrates the fix behaves as expected?

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

454–455

Formatting is incorrect here -- you should run the patch through clang-format (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting), and elide the braces.

[thorsten-klein]

Hello,
Can you please support how to do that? :-)

[aaron.ballman]

In D59135#1422781, @thorsten-klein wrote:

Hello,
Can you please support how to do that? :-)

All of the tests live in extra\test\clang-tidy\, so you'd add a file in there. I believe file-filter.cpp does stuff with header filters, as an example of how the files are structured. The basic goal is: make a test that fails without your patch and succeeds with it.

[thorsten-klein]

Sorry for delay. I am currently not able to build master (Error: "sort" is not a member of "llvm").
And if I try to build 6.0.1-rc3 I can build clang-tidy but I am not able to build corresponding unittests "ClangTidyTests".
I will try to solve as soon as possible.

[alexfh]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

453

That'll result in a dangling pointer. SourceLocation::printToString returns a temporary std::string that will be destroyed at the end of expression, but FileName will point to its data.

Also, the printToString method will contain much more than just the filename. SourceManager::getPresumedLoc should do a better job here.

[alexfh]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

454

I believe, this will break the handling of notes. If the notes attached to the diagnostic are in the "interesting" part of the code (see the checkFilters method below), the whole diagnostic should be treated as such.

[thorsten-klein]

Hello,
can you please support with this Pull-Request? You really have better knowledge about source code.
For our case this solution was working fine and I wanted to share with you. Maybe it will help you if I create an examplary project which reproduces the mentioned issue?

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

454

Hello,
Unfortunately the emitDiagnostic method is called also for files, which should be actually excluded (since the FileName does not match the HeaderFilter regex). This results in diagnostic output, also if there should not be any. For this purpose I added this check here at this point. If the FileName is matching the HeaderFilter regex, then all diagnostic is emitted as before.

Did you know about any issue regarding this? If this is not a working solution for you: do you have any other proposal?

PS: I have also read about this issue in another project: https://fuchsia.googlesource.com/peridot/+/HEAD/docs/lint.md
--> They say that they disable some clang-tidy checks as workaround

[lebedev.ri]

In D59135#1422822, @thorsten-klein wrote:

Sorry for delay. I am currently not able to build master (Error: "sort" is not a member of "llvm").
And if I try to build 6.0.1-rc3 I can build clang-tidy but I am not able to build corresponding unittests "ClangTidyTests".
I will try to solve as soon as possible.

The patches need to be for llvm trunk / git master.

[alexfh]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

454

Could you provide a specific example, where you see a problem? Ideally, an isolated set of files + clang-tidy arguments.

454

Unfortunately the emitDiagnostic method is called also for files, which should be actually excluded (since the FileName does not match the HeaderFilter regex).

As per my initial comment, this is done intentionally to make it possible for a note issued in an "interesting" part of the code to make the whole diagnostic "interesting". Here's a motivating example:

$ cat a.cc
#include "b.h"
$ cat b.h 
#include "c.h"
inline void b() { c(/*y=*/42); }
$ cat c.h
void c(int x);

If we're only interested in the main file, we don't get any diagnostic:

$ clang-tidy -checks=-*,bugprone-argument-comment a.cc --
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

Now if we want diagnostics for c.h, we will also get all diagnostics that are (or have notes in) c.h:

$ clang-tidy -checks=-*,bugprone-argument-comment a.cc -header-filter=c.h --
1 warning generated.
b.h:2:21: warning: argument name 'y' in comment does not match parameter name 'x' [bugprone-argument-comment]
inline void b() { c(/*y=*/42); }
                    ^~~~~~
                    /*x=*/
c.h:1:12: note: 'x' declared here
void c(int x);
           ^

The fact that we have a note in the "interesting" part of code (be it a header matching -header-filter, or a location matching -line-filter) means that the diagnostic is somehow related to the "interesting" code. It may well be caused by a change in the "interesting" code (e.g. if we changed the parameter name of the function c from y to x in the example above). And for this reason we consider it "interesting".

In some cases this may lead to suboptimal results, I guess, but it would be interesting to see these cases. Could you provide a specific example, where you see a problem? Ideally, an isolated set of files + clang-tidy arguments.

[thorsten-klein]

Hello @alexfh ,
Let me extend your example

$ cat a.cc 
#include "b.h"
#include "d.h"
int main(){check(nullptr);}
$ cat b.h 
#include "c.h"
inline void b() { c(/*y=*/42); }
$ cat c.h 
void c(int x);
$ cat d.h 
inline char* check(char* buffer)
{
	*buffer++=1; // Should be clang-analyzer-core.NullDereference
	return buffer;
}

Now an additional warning is found and shown (=not suppressed):

$ clang-tidy -checks=-*,clang-*,bugprone-argument-comment a.cc --
2 warnings generated.
/home/default/Temp/clang-tidy-test/d.h:3:11: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^
/home/default/Temp/clang-tidy-test/a.cc:3:12: note: Calling 'check'
int main(){check(0);}
           ^
/home/default/Temp/clang-tidy-test/d.h:3:3: note: Null pointer value stored to 'buffer'
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
         ^
/home/default/Temp/clang-tidy-test/d.h:3:11: note: Dereference of null pointer
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

How can I use -header-filter now?

With -header-filter=b.h clang-tidy shows both warnings:

$ clang-tidy -checks=-*,clang-*,bugprone-argument-comment a.cc -header-filter=b.h --
2 warnings generated.
/home/default/Temp/clang-tidy-test/b.h:2:21: warning: argument name 'y' in comment does not match parameter name 'x' [bugprone-argument-comment]
inline void b() { c(/*y=*/42); }
                    ^~~~~~
                    /*x=*/
/home/default/Temp/clang-tidy-test/c.h:1:12: note: 'x' declared here
void c(int x);
           ^
/home/default/Temp/clang-tidy-test/d.h:3:11: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^
/home/default/Temp/clang-tidy-test/a.cc:3:12: note: Calling 'check'
int main(){check(0);}
           ^
/home/default/Temp/clang-tidy-test/d.h:3:3: note: Null pointer value stored to 'buffer'
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
         ^
/home/default/Temp/clang-tidy-test/d.h:3:11: note: Dereference of null pointer
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^

With -header-filter=c.h clang-tidy shows both warnings:

$ clang-tidy -checks=-*,clang-*,bugprone-argument-comment a.cc -header-filter=c.h --
2 warnings generated.
/home/default/Temp/clang-tidy-test/b.h:2:21: warning: argument name 'y' in comment does not match parameter name 'x' [bugprone-argument-comment]
inline void b() { c(/*y=*/42); }
                    ^~~~~~
                    /*x=*/
/home/default/Temp/clang-tidy-test/c.h:1:12: note: 'x' declared here
void c(int x);
           ^
/home/default/Temp/clang-tidy-test/d.h:3:11: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^
/home/default/Temp/clang-tidy-test/a.cc:3:12: note: Calling 'check'
int main(){check(0);}
           ^
/home/default/Temp/clang-tidy-test/d.h:3:3: note: Null pointer value stored to 'buffer'
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
         ^
/home/default/Temp/clang-tidy-test/d.h:3:11: note: Dereference of null pointer
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^

With -header-filter=c.h clang-tidy shows both warnings:

$ clang-tidy -checks=-*,clang-*,bugprone-argument-comment a.cc -header-filter=d.h --
2 warnings generated.
/home/default/Temp/clang-tidy-test/d.h:3:11: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^
/home/default/Temp/clang-tidy-test/a.cc:3:12: note: Calling 'check'
int main(){check(nullptr);}
           ^
/home/default/Temp/clang-tidy-test/d.h:3:3: note: Null pointer value stored to 'buffer'
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
         ^
/home/default/Temp/clang-tidy-test/d.h:3:11: note: Dereference of null pointer
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

How can I suppress warning for my header file d.h so that only warning from b.h is shown?

[alexfh]

In D59135#1458265, @thorsten-klein wrote:

Hello @alexfh ,
Let me extend your example

$ cat a.cc 
#include "b.h"
#include "d.h"
int main(){check(nullptr);}
$ cat b.h 
#include "c.h"
inline void b() { c(/*y=*/42); }
$ cat c.h 
void c(int x);
$ cat d.h 
inline char* check(char* buffer)
{
	*buffer++=1; // Should be clang-analyzer-core.NullDereference
	return buffer;
}

Now an additional warning is found and shown (=not suppressed):

$ clang-tidy -checks=-*,clang-*,bugprone-argument-comment a.cc --
2 warnings generated.
/home/default/Temp/clang-tidy-test/d.h:3:11: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^
/home/default/Temp/clang-tidy-test/a.cc:3:12: note: Calling 'check'
int main(){check(0);}
           ^
/home/default/Temp/clang-tidy-test/d.h:3:3: note: Null pointer value stored to 'buffer'
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
         ^
/home/default/Temp/clang-tidy-test/d.h:3:11: note: Dereference of null pointer
        *buffer++=1; // Should be clang-analyzer-core.NullDereference
                 ^
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

The behavior in the case above seems to be correct. The null pointer dereference is happening in d.h:3:11, which is not included into the default header-filter (which is empty == no headers are considered "interesting" on their own). However, the (possible) cause of the problem is in a.cc:3:12, which is considered "interesting". Thus the whole diagnostic with all notes attached to it is displayed to the user.

The general rule is: a warning (together with all of its notes) is displayed if its location or location of any of it's notes is inside a the main file or a (non-system, unless -system-headers option is present) header matching the regex configured via the -header-filter option. The -line-filter works in a very similar way, but the whole main file is not whitelisted by default, only the ranges of lines in it that are parts of the -line-filter.

How can I use -header-filter now?

This depends on what you're trying to achieve.

With -header-filter=b.h clang-tidy shows both warnings:

This aligns well with the logic I described above.

With -header-filter=c.h clang-tidy shows both warnings:

Same here: the bugprone-argument-comment warning is shown due to the -header-filter=c.h option. The clang-analyzer-core.NullDereference will be shown regardless of the -header-filter value, because it is related to the main file.

With -header-filter=c.h clang-tidy shows both warnings:

It looks like you wanted to say that with -header-filter=d.h only the clang-analyzer-core.NullDereference warning is shown. Seems correct. See above.

How can I suppress warning for my header file d.h so that only warning from b.h is shown?

The warning in d.h is related to the main file (since it has a note in the main file). It will be displayed regardless of the -header-filter. This is by design.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

void ClangTidyDiagnosticConsumer::HandleDiagnostic(
   ClangTidyDiagnosticRenderer Converter(
       Context.getLangOpts(), &Context.DiagEngine->getDiagnosticOptions(),
       Errors.back());
   SmallString<100> Message;
   Info.FormatDiagnostic(Message);
   FullSourceLoc Loc;
   if (Info.getLocation().isValid() && Info.hasSourceManager())
     Loc = FullSourceLoc(Info.getLocation(), Info.getSourceManager());
+
+  StringRef FileName(Loc.printToString(Loc.getManager()));
+  if(getHeaderFilter()->match(FileName))
+  {
     Converter.emitDiagnostic(Loc, DiagLevel, Message, Info.getRanges(),
                            Info.getFixItHints());
+  }
 
   if (Info.hasSourceManager())
     checkFilters(Info.getLocation(), Info.getSourceManager());
 }
 
 bool ClangTidyDiagnosticConsumer::passesLineFilter(StringRef FileName,
                                                    unsigned LineNumber) const {
   if (Context.getGlobalOptions().LineFilter.empty())