From b8de0c4278b638163f6b5103b2eadcb6590f8dfd Mon Sep 17 00:00:00 2001 From: Anna Zaks Date: Fri, 27 Jun 2014 01:03:05 +0000 Subject: [PATCH] Do not inline methods of C++ containers (coming from headers). This silences false positives (leaks, use of uninitialized value) in simple code that uses containers such as std::vector and std::list. The analyzer cannot reason about the internal invariances of those data structures which leads to false positives. Until we come up with a better solution to that problem, let's just not inline the methods of the containers and allow objects to escape whenever such methods are called. This just extends an already existing flag "c++-container-inlining" and applies the heuristic not only to constructors and destructors of the containers, but to all of their methods. We have a bunch of distinct user reports all related to this issue (radar://16058651, radar://16580751, radar://16384286, radar://16795491 [PR19637]). llvm-svn: 211832 --- .../clang/StaticAnalyzer/Core/AnalyzerOptions.h | 10 ++--- clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp | 4 +- .../Core/ExprEngineCallAndReturn.cpp | 24 +++++------ clang/test/Analysis/inlining/containers.cpp | 50 +++++++++++++++++----- 4 files changed, 58 insertions(+), 30 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h index 520fed0..978c3e2 100644 --- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h +++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h @@ -202,8 +202,8 @@ private: /// \sa mayInlineCXXAllocator Optional InlineCXXAllocator; - /// \sa mayInlineCXXContainerCtorsAndDtors - Optional InlineCXXContainerCtorsAndDtors; + /// \sa mayInlineCXXContainerMethods + Optional InlineCXXContainerMethods; /// \sa mayInlineCXXSharedPtrDtor Optional InlineCXXSharedPtrDtor; @@ -303,12 +303,12 @@ public: /// accepts the values "true" and "false". bool mayInlineCXXAllocator(); - /// Returns whether or not constructors and destructors of C++ container - /// objects may be considered for inlining. + /// Returns whether or not methods of C++ container objects may be considered + /// for inlining. /// /// This is controlled by the 'c++-container-inlining' config option, which /// accepts the values "true" and "false". - bool mayInlineCXXContainerCtorsAndDtors(); + bool mayInlineCXXContainerMethods(); /// Returns whether or not the destructor of C++ 'shared_ptr' may be /// considered for inlining. diff --git a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp index 85d66ba..7944c7e 100644 --- a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp +++ b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp @@ -140,8 +140,8 @@ bool AnalyzerOptions::mayInlineCXXAllocator() { /*Default=*/false); } -bool AnalyzerOptions::mayInlineCXXContainerCtorsAndDtors() { - return getBooleanOption(InlineCXXContainerCtorsAndDtors, +bool AnalyzerOptions::mayInlineCXXContainerMethods() { + return getBooleanOption(InlineCXXContainerMethods, "c++-container-inlining", /*Default=*/false); } diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index 4619f62..3f608ba 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -708,18 +708,16 @@ static bool isContainerClass(const ASTContext &Ctx, const CXXRecordDecl *RD) { hasMember(Ctx, RD, "iterator_category"); } -/// Returns true if the given function refers to a constructor or destructor of -/// a C++ container or iterator. +/// Returns true if the given function refers to a method of a C++ container +/// or iterator. /// -/// We generally do a poor job modeling most containers right now, and would -/// prefer not to inline their setup and teardown. -static bool isContainerCtorOrDtor(const ASTContext &Ctx, - const FunctionDecl *FD) { - if (!(isa(FD) || isa(FD))) - return false; - - const CXXRecordDecl *RD = cast(FD)->getParent(); - return isContainerClass(Ctx, RD); +/// We generally do a poor job modeling most containers right now, and might +/// prefer not to inline their methods. +static bool isContainerMethod(const ASTContext &Ctx, + const FunctionDecl *FD) { + if (const CXXMethodDecl *MD = dyn_cast(FD)) + return isContainerClass(Ctx, MD->getParent()); + return false; } /// Returns true if the given function is the destructor of a class named @@ -765,9 +763,9 @@ static bool mayInlineDecl(AnalysisDeclContext *CalleeADC, // Conditionally control the inlining of methods on objects that look // like C++ containers. - if (!Opts.mayInlineCXXContainerCtorsAndDtors()) + if (!Opts.mayInlineCXXContainerMethods()) if (!Ctx.getSourceManager().isInMainFile(FD->getLocation())) - if (isContainerCtorOrDtor(Ctx, FD)) + if (isContainerMethod(Ctx, FD)) return false; // Conditionally control the inlining of the destructor of C++ shared_ptr. diff --git a/clang/test/Analysis/inlining/containers.cpp b/clang/test/Analysis/inlining/containers.cpp index 73b2957a..c757da6 100644 --- a/clang/test/Analysis/inlining/containers.cpp +++ b/clang/test/Analysis/inlining/containers.cpp @@ -103,7 +103,10 @@ public: ~MySet() { delete[] storage; } bool isEmpty() { - clang_analyzer_checkInlined(true); // expected-warning {{TRUE}} + clang_analyzer_checkInlined(true); + #if INLINE + // expected-warning@-2 {{TRUE}} + #endif return size == 0; } @@ -114,23 +117,35 @@ public: }; iterator begin() { - clang_analyzer_checkInlined(true); // expected-warning {{TRUE}} + clang_analyzer_checkInlined(true); + #if INLINE + // expected-warning@-2 {{TRUE}} + #endif return iterator(storage); } iterator end() { - clang_analyzer_checkInlined(true); // expected-warning {{TRUE}} + clang_analyzer_checkInlined(true); + #if INLINE + // expected-warning@-2 {{TRUE}} + #endif return iterator(storage+size); } typedef int *raw_iterator; raw_iterator raw_begin() { - clang_analyzer_checkInlined(true); // expected-warning {{TRUE}} + clang_analyzer_checkInlined(true); + #if INLINE + // expected-warning@-2 {{TRUE}} + #endif return storage; } raw_iterator raw_end() { - clang_analyzer_checkInlined(true); // expected-warning {{TRUE}} + clang_analyzer_checkInlined(true); + #if INLINE + // expected-warning@-2 {{TRUE}} + #endif return storage + size; } }; @@ -145,7 +160,10 @@ public: } void useIterator(iterator i) { - clang_analyzer_checkInlined(true); // expected-warning {{TRUE}} + clang_analyzer_checkInlined(true); + #if INLINE + // expected-warning@-2 {{TRUE}} + #endif } }; @@ -174,7 +192,10 @@ public: typedef IterImpl wrapped_iterator; wrapped_iterator begin() { - clang_analyzer_checkInlined(true); // expected-warning {{TRUE}} + clang_analyzer_checkInlined(true); + #if INLINE + // expected-warning@-2 {{TRUE}} + #endif return IterImpl(impl.begin()); } }; @@ -193,7 +214,10 @@ public: typedef MySet::iterator iterator; iterator start() { - clang_analyzer_checkInlined(true); // expected-warning {{TRUE}} + clang_analyzer_checkInlined(true); +#if INLINE + // expected-warning@-2 {{TRUE}} +#endif return impl.begin(); } }; @@ -212,7 +236,10 @@ public: using iterator = MySet::iterator; iterator start() { - clang_analyzer_checkInlined(true); // expected-warning {{TRUE}} + clang_analyzer_checkInlined(true); + #if INLINE + // expected-warning@-2 {{TRUE}} + #endif return impl.begin(); } }; @@ -233,7 +260,10 @@ public: }; iterator start() { - clang_analyzer_checkInlined(true); // expected-warning {{TRUE}} + clang_analyzer_checkInlined(true); + #if INLINE + // expected-warning@-2 {{TRUE}} + #endif return iterator{impl.begin().impl}; } }; -- 2.7.4