[cfe-dev] Clang static analyzer: ignore library headers?
Anna Zaks
ganna at apple.com
Thu Oct 10 11:50:32 PDT 2013
On Oct 9, 2013, at 6:40 PM, Anna Zaks <ganna at apple.com> wrote:
> Hi Aemon,
>
> Below are some high-level comments.
>
> Thanks for working on this!
> Anna.
> On Oct 3, 2013, at 7:07 PM, Aemon Cannon <aemoncannon at gmail.com> wrote:
>
>> I have a work-in-progress patch I'd like to share. Feedback appreciated.
>
> Generally, we send patches to cfe-dev.
>
I ment to say "we send patches to cfe-commits mailing list (http://clang.llvm.org/hacking.html)".
>> Goal:
>> I want to be able to analyze the code in my headers while ignoring code from third-party headers that are included in my sources. Adds a new analyzer option, analyzer-ignore-headers, for explicitly blacklisting headers by prefix.
>>
>> TODO:
>> * Silence warnings from code in blacklisted headers when that code is inlined into sources.
>> * Check case-sensitivity of the local filesystem and choose string Compare implementation accordingly.
>>
>> Misc:
>> * I decided against the -isystem option because I wanted something more targeted that doesn't require modifying the build command.
>> * I'm not doing any path expansion or normalization, just a dumb string comparison. Is this sufficient?
>>
>>
>> Thanks!
>>
>> diff --git a/include/clang/Driver/CC1Options.td b/include/clang/Driver/CC1Options.td
>> index 9acbd48..cc64c34 100644
>> --- a/include/clang/Driver/CC1Options.td
>> +++ b/include/clang/Driver/CC1Options.td
>> @@ -111,6 +111,9 @@ def analyzer_checker_help : Flag<["-"], "analyzer-checker-help">,
>> def analyzer_config : Separate<["-"], "analyzer-config">,
>> HelpText<"Choose analyzer options to enable">;
>>
>> +def analyzer_ignore_headers : Separate<["-"], "analyzer-ignore-headers">,
>> + HelpText<"Ignore headers by prefix.">;
>> +
>
> We are moving toward not exposing/implementing analyzer options support in the clang Driver code, but rather keeping it within the analyzer. You can see that some of the options are implemented that way (for example, see AnalyzerOptions::mayInlineCXXMemberFunction). Would it be possible to implement "analyzer-ignore-headers" in the same way?
>
>> //===----------------------------------------------------------------------===//
>> // Migrator Options
>> //===----------------------------------------------------------------------===//
>> diff --git a/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h b/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
>> index 618782e..45e75d1 100644
>> --- a/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
>> +++ b/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
>> @@ -122,6 +122,7 @@ public:
>>
>> /// \brief Pair of checker name and enable/disable.
>> std::vector<std::pair<std::string, bool> > CheckersControlList;
>> + std::vector<std::string> HeaderBlacklist;
>>
>> /// \brief A key-value table of use-specified configuration values.
>> ConfigTable Config;
>> @@ -267,6 +268,8 @@ public:
>> /// \sa CXXMemberInliningMode
>> bool mayInlineCXXMemberFunction(CXXInlineableMemberKind K);
>>
>> + bool headerIsBlacklisted(StringRef IncludePath);
>> +
>> /// Returns true if ObjectiveC inlining is enabled, false otherwise.
>> bool mayInlineObjCMethod();
>>
>> diff --git a/lib/Frontend/CompilerInvocation.cpp b/lib/Frontend/CompilerInvocation.cpp
>> index b0d7a0a..6ed2abc 100644
>> --- a/lib/Frontend/CompilerInvocation.cpp
>> +++ b/lib/Frontend/CompilerInvocation.cpp
>> @@ -280,6 +280,28 @@ static bool ParseAnalyzerArgs(AnalyzerOptions &Opts, ArgList &Args,
>> }
>> }
>>
>> + // Go through the analyzer header blacklist.
>> + for (arg_iterator it = Args.filtered_begin(OPT_analyzer_ignore_headers),
>> + ie = Args.filtered_end(); it != ie; ++it) {
>> + const Arg *A = *it;
>> + A->claim();
>> + StringRef headerList = A->getValue();
>> + SmallVector<StringRef, 4> headerVals;
>> + headerList.split(headerVals, ",");
>> + for (unsigned i = 0, e = headerVals.size(); i != e; ++i) {
>> + StringRef val = headerVals[i];
>> + if (val.empty()) {
>> + // TODO(aemon): properly report problem
>> + // Diags.Report(SourceLocation(),
>> + // diag::err_analyzer_header_no_value) << headerVals[i];
>> + Success = false;
>> + break;
>> + }
>> + Opts.HeaderBlacklist.push_back(val.str());
>> + }
>> + }
>> + std::sort(Opts.HeaderBlacklist.begin(), Opts.HeaderBlacklist.end());
>> +
>> return Success;
>> }
>>
>> diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt
>> index 392995e..123e84f 100644
>> --- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt
>> +++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt
>> @@ -50,6 +50,8 @@ add_clang_library(clangStaticAnalyzerCheckers
>> ObjCMissingSuperCallChecker.cpp
>> ObjCSelfInitChecker.cpp
>> ObjCUnusedIVarsChecker.cpp
>> + OwnershipChecker.cpp
>> + TracingChecker.cpp
>> PointerArithChecker.cpp
>> PointerSubChecker.cpp
>> PthreadLockChecker.cpp
>> diff --git a/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp b/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>> index 9dcf58b..3cfde27 100644
>> --- a/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>> +++ b/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
>> @@ -96,6 +96,16 @@ AnalyzerOptions::mayInlineCXXMemberFunction(CXXInlineableMemberKind K) {
>> return CXXMemberInliningMode >= K;
>> }
>>
>> +bool AnalyzerOptions::headerIsBlacklisted(StringRef IncludePath) {
>> + if (HeaderBlacklist.size() == 0) return false;
>> + std::vector<std::string>::const_iterator SI =
>> + lower_bound(HeaderBlacklist.begin(), HeaderBlacklist.end(),
>> + IncludePath);
>> + if (SI != HeaderBlacklist.end() && *SI == IncludePath) return true;
>> + if (SI == HeaderBlacklist.begin()) return false;
>> + return IncludePath.startswith(*(--SI));
>> +}
>> +
>> static StringRef toString(bool b) { return b ? "true" : "false"; }
>>
>> bool AnalyzerOptions::getBooleanOption(StringRef Name, bool DefaultVal) {
>> diff --git a/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp b/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
>> index 9efe997..0a13906 100644
>> --- a/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
>> +++ b/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
>> @@ -414,13 +414,23 @@ void AnalysisConsumer::HandleTopLevelDeclInObjCContainer(DeclGroupRef DG) {
>> }
>>
>> void AnalysisConsumer::storeTopLevelDecls(DeclGroupRef DG) {
>> + SourceManager &SM = Ctx->getSourceManager();
>> for (DeclGroupRef::iterator I = DG.begin(), E = DG.end(); I != E; ++I) {
>> -
>> // Skip ObjCMethodDecl, wait for the objc container to avoid
>> // analyzing twice.
>> if (isa<ObjCMethodDecl>(*I))
>> continue;
>>
>
> I'd add this to AnalysisConsumer::getModeForDecl(), where the analysis mode/policy is defined, instead of AnalysisConsumer::storeTopLevelDecls().
>
>> + SourceLocation SL = (*I)->getLocation();
>> + if (!SM.isInMainFile(SL) && SL.isFileID()) {
>> + if (const char* IncludePath = SM.getBufferName(SL)) {
>> + StringRef S(IncludePath);
>> + if (Opts->headerIsBlacklisted(S)) {
>> + continue;
>> + }
>> + }
>> + }
>> +
>> LocalTUDecls.push_back(*I);
>> }
>> }
>
> There is no test for the command line case.
>
>> diff --git a/unittests/Frontend/AnalyzerOptionsTest.cpp b/unittests/Frontend/AnalyzerOptionsTest.cpp
>> new file mode 100644
>> index 0000000..489772c
>> --- /dev/null
>> +++ b/unittests/Frontend/AnalyzerOptionsTest.cpp
>> @@ -0,0 +1,57 @@
>> +//===- unittests/Frontend/AnalyzerOptionsTest.cpp - AnalyzerOptions tests ---===//
>> +//
>> +// The LLVM Compiler Infrastructure
>> +//
>> +// This file is distributed under the University of Illinois Open Source
>> +// License. See LICENSE.TXT for details.
>> +//
>> +//===----------------------------------------------------------------------===//
>> +
>> +#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
>> +#include "gtest/gtest.h"
>> +
>> +using namespace llvm;
>> +using namespace clang;
>> +
>> +namespace {
>> +
>> +TEST(AnalyzerOptionsTest, EmptyBlacklist) {
>> + AnalyzerOptions options;
>> + ASSERT_FALSE(options.headerIsBlacklisted("C:\\abc\\hello.h"));
>> + ASSERT_FALSE(options.headerIsBlacklisted("bla.h"));
>> +}
>> +
>> +TEST(AnalyzerOptionsTest, Simple) {
>> + AnalyzerOptions options;
>> + options.HeaderBlacklist.push_back("C:\\abc\\");
>> + options.HeaderBlacklist.push_back("/system/include");
>> + std::sort(options.HeaderBlacklist.begin(), options.HeaderBlacklist.end());
>> + ASSERT_TRUE(options.headerIsBlacklisted("C:\\abc\\hello.h"));
>> + ASSERT_FALSE(options.headerIsBlacklisted("/system/hello.h"));
>> +}
>> +
>> +TEST(AnalyzerOptionsTest, LongerList) {
>> + AnalyzerOptions options;
>> + options.HeaderBlacklist.push_back("D:\\abc\\");
>> + options.HeaderBlacklist.push_back("C:\\abc\\");
>> + options.HeaderBlacklist.push_back("/var/log/system");
>> + options.HeaderBlacklist.push_back("/system/include");
>> + options.HeaderBlacklist.push_back("/home/tmp/");
>> + options.HeaderBlacklist.push_back("../things/");
>> + std::sort(options.HeaderBlacklist.begin(), options.HeaderBlacklist.end());
>> + ASSERT_TRUE(options.headerIsBlacklisted("C:\\abc\\hello.h"));
>> + ASSERT_TRUE(options.headerIsBlacklisted("/var/log/system/../hello"));
>> + ASSERT_TRUE(options.headerIsBlacklisted("../things/foo.c"));
>> + ASSERT_FALSE(options.headerIsBlacklisted("D:\\ab\\"));
>> +}
>> +
>> +TEST(AnalyzerOptionsTest, CaseSensitive) {
>> + AnalyzerOptions options;
>> + options.HeaderBlacklist.push_back("C:\\abc\\");
>> + options.HeaderBlacklist.push_back("/system/include");
>> + std::sort(options.HeaderBlacklist.begin(), options.HeaderBlacklist.end());
>> + ASSERT_FALSE(options.headerIsBlacklisted("c:\\abc\\hello.h"));
>> + ASSERT_FALSE(options.headerIsBlacklisted("/system/Include/hello.h"));
>> +}
>> +
>> +} // anonymous namespace
>> diff --git a/unittests/Frontend/CMakeLists.txt b/unittests/Frontend/CMakeLists.txt
>> index c65a163..39b4064 100644
>> --- a/unittests/Frontend/CMakeLists.txt
>> +++ b/unittests/Frontend/CMakeLists.txt
>> @@ -8,6 +8,7 @@ set(LLVM_LINK_COMPONENTS
>>
>> add_clang_unittest(FrontendTests
>> FrontendActionTest.cpp
>> + AnalyzerOptionsTest.cpp
>> )
>> target_link_libraries(FrontendTests
>> clangFrontend
>>
>>
>>
>>
>>
>> On Tue, Sep 17, 2013 at 12:14 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>>
>> On Sep 16, 2013, at 22:34 , Ben Pope <benpope81 at gmail.com> wrote:
>>
>>> On 17/09/13 01:16, Anna Zaks wrote:
>>>> We don't have a list of known third-party headers anywhere, but having this seems generally useful.
>>>
>>> Determining them based on -I vs -isystem would seem to be a sensible
>>> distinction.
>>>
>>> At least, that's what I use to include third party libraries when I want
>>> to ignore warnings they produce. It's entirely possible I'm abusing
>>> that feature.
>>
>> It's a bit of an abuse but likely not a harmful one. Huh, we don't actually have that mode now. Regular mode runs path-sensitive checks on the main source file and non-path-sensitive checks on the main source file + non-system headers. -analyzer-opt-analyze-headers mode runs path-sensitive checks everywhere. Patches welcome?
>>
>> As for an explicit blacklist, we've gotten that request before, but only for main source files: PR2706 and <rdar://problem/9063762>. A header file filter would be slightly different.
>>
>> Jordan
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20131010/ded561d3/attachment.html>
More information about the cfe-dev
mailing list