Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h +++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h @@ -51,9 +51,22 @@ /// Constructs a CallDescription object. /// /// @param QualifiedName The list of the name qualifiers of the function that - /// will be matched. The user is allowed to skip any of the qualifiers. - /// For example, {"std", "basic_string", "c_str"} would match both + /// will be matched. The user is allowed to skip any of the inline namespace + /// qualifiers. For example, {"std", "basic_string", "c_str"} would match both /// std::basic_string<...>::c_str() and std::__1::basic_string<...>::c_str(). + /// We also accept 'wildcards', which is a single question mark "?". + /// + /// A successful match of a qualifier part will consume it. + /// A failed match will consume the wildcard if available. + /// Inline namespaces will consume a qualifier part only if the expected part + /// matches with the name of the inline namespace, otherwise, they will be + /// skipped and the match continues without consuming any qualifier part. + /// If at the end of the match there are qualifier parts left out, the match + /// is rejected. + /// The function name cannot be a wildcard. + /// For example: {"my", "?", "foo"} would match any name at the wildcard - + /// except inline namespaces. + /// Check the unittests for more details. /// /// @param RequiredArgs The number of arguments that is expected to match a /// call. Omit this parameter to match every occurrence of call with a given Index: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp @@ -66,9 +66,9 @@ {&CastValueChecker::evalCastOrNull, CallKind::Function}}, {{{"llvm", "dyn_cast_or_null"}, 1}, {&CastValueChecker::evalDynCastOrNull, CallKind::Function}}, - {{{"clang", "castAs"}, 0}, + {{{"clang", "?", "castAs"}, 0}, {&CastValueChecker::evalCastAs, CallKind::Method}}, - {{{"clang", "getAs"}, 0}, + {{{"clang", "?", "getAs"}, 0}, {&CastValueChecker::evalGetAs, CallKind::Method}}, {{{"llvm", "isa"}, 1}, {&CastValueChecker::evalIsa, CallKind::InstanceOf}}, Index: clang/lib/StaticAnalyzer/Core/CallDescription.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/CallDescription.cpp +++ clang/lib/StaticAnalyzer/Core/CallDescription.cpp @@ -38,6 +38,7 @@ RequiredParams(readRequiredParams(RequiredArgs, RequiredParams)), Flags(Flags) { assert(!QualifiedName.empty()); + assert(QualifiedName.back() != StringRef("?")); } /// Construct a CallDescription with default flags. Index: clang/lib/StaticAnalyzer/Core/CallEvent.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/CallEvent.cpp +++ clang/lib/StaticAnalyzer/Core/CallEvent.cpp @@ -347,27 +347,55 @@ const auto MatchQualifiedNameParts = [](const CallDescription &CD, const Decl *D) -> bool { + const auto IsSkippable = [](const DeclContext *Ctx) -> bool { + // Inline or anonymous namespaces. + if (const auto *NS = dyn_cast(Ctx)) + return NS->isInlineNamespace() || NS->isAnonymousNamespace(); + + // Struct/class instances without name, such as `struct {} obj;`. + // This is different from the anonymous structs/unions. + if (const auto *RD = dyn_cast(Ctx)) { + const DeclarationName Name = RD->getDeclName(); + return Name.isIdentifier() && Name.getAsIdentifierInfo() == nullptr; + } + return false; + }; const auto FindNextNamespaceOrRecord = [](const DeclContext *Ctx) -> const DeclContext * { while (Ctx && !isa(Ctx)) Ctx = Ctx->getParent(); return Ctx; }; + constexpr StringRef Wildcard = "?"; auto QualifierPartsIt = CD.begin_qualified_name_parts(); const auto QualifierPartsEndIt = CD.end_qualified_name_parts(); - // Match namespace and record names. Skip unrelated names if they don't - // match. + // Match the qualified name parts. const DeclContext *Ctx = FindNextNamespaceOrRecord(D->getDeclContext()); for (; Ctx && QualifierPartsIt != QualifierPartsEndIt; Ctx = FindNextNamespaceOrRecord(Ctx->getParent())) { - // If not matched just continue and try matching for the next one. - if (cast(Ctx)->getName() != *QualifierPartsIt) + const bool Mismatch = + (cast(Ctx)->getName() != *QualifierPartsIt); + const bool MatchingAgainstWildcard = (*QualifierPartsIt == Wildcard); + + if (Mismatch && IsSkippable(Ctx)) continue; + + // Otherwise, a wildcard matches anything greedily. + if (Mismatch && !MatchingAgainstWildcard) + return false; + + // We advance since we either matched, or used a wildcard. + assert(!Mismatch || MatchingAgainstWildcard); ++QualifierPartsIt; } + // We should check if any named stuff is left, which is not skippable. + for (; Ctx; Ctx = Ctx->getParent()) + if (isa(Ctx) && !IsSkippable(Ctx)) + return false; + // We matched if we consumed all expected qualifier segments. return QualifierPartsIt == QualifierPartsEndIt; }; Index: clang/test/Analysis/return-value-guaranteed.cpp =================================================================== --- clang/test/Analysis/return-value-guaranteed.cpp +++ clang/test/Analysis/return-value-guaranteed.cpp @@ -1,6 +1,10 @@ // RUN: %clang_analyze_cc1 \ // RUN: -analyzer-checker=core,apiModeling.llvm.ReturnValue \ -// RUN: -analyzer-output=text -verify=class %s +// RUN: -analyzer-output=text %s +// +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,apiModeling.llvm.ReturnValue \ +// RUN: -analyzer-output=text %s -DErrorReturnsFalse struct Foo { int Field; }; bool problem(); @@ -8,87 +12,86 @@ // We predefined the return value of 'MCAsmParser::Error' as true and we cannot // take the false-branches which leads to a "garbage value" false positive. -namespace test_classes { struct MCAsmParser { +#ifdef ErrorReturnsFalse + // We predefined 'MCAsmParser::Error' as returning true, but now it returns + // false, which breaks our invariant. Test the notes. + static bool Error() { + return false; // expected-note {{'MCAsmParser::Error' returns false}} + // expected-note@-1 {{Returning zero, which participates in a condition later}} + } +#else static bool Error(); +#endif }; +namespace test_classes { bool parseFoo(Foo &F) { if (problem()) { - // class-note@-1 {{Assuming the condition is false}} - // class-note@-2 {{Taking false branch}} + // expected-note@-1 {{Assuming the condition is false}} + // expected-note@-2 {{Taking false branch}} return MCAsmParser::Error(); } F.Field = 0; - // class-note@-1 {{The value 0 is assigned to 'F.Field'}} + // expected-note@-1 {{The value 0 is assigned to 'F.Field'}} return !MCAsmParser::Error(); - // class-note@-1 {{'MCAsmParser::Error' returns true}} - // class-note@-2 {{Returning zero, which participates in a condition later}} + // expected-note@-1 {{'MCAsmParser::Error' returns true}} + // expected-note@-2 {{Returning zero, which participates in a condition later}} } bool parseFile() { Foo F; if (parseFoo(F)) { - // class-note@-1 {{Calling 'parseFoo'}} - // class-note@-2 {{Returning from 'parseFoo'}} - // class-note@-3 {{Taking false branch}} + // expected-note@-1 {{Calling 'parseFoo'}} + // expected-note@-2 {{Returning from 'parseFoo'}} + // expected-note@-3 {{Taking false branch}} return true; } if (F.Field == 0) { - // class-note@-1 {{Field 'Field' is equal to 0}} - // class-note@-2 {{Taking true branch}} + // expected-note@-1 {{Field 'Field' is equal to 0}} + // expected-note@-2 {{Taking true branch}} // no-warning: "The left operand of '==' is a garbage value" was here. doSomething(); } (void)(1 / F.Field); - // class-warning@-1 {{Division by zero}} - // class-note@-2 {{Division by zero}} + // expected-warning@-1 {{Division by zero}} + // expected-note@-2 {{Division by zero}} return false; } } // namespace test_classes - -// We predefined 'MCAsmParser::Error' as returning true, but now it returns -// false, which breaks our invariant. Test the notes. namespace test_break { -struct MCAsmParser { - static bool Error() { - return false; // class-note {{'MCAsmParser::Error' returns false}} - // class-note@-1 {{Returning zero, which participates in a condition later}} - } -}; - bool parseFoo(Foo &F) { if (problem()) { - // class-note@-1 {{Assuming the condition is false}} - // class-note@-2 {{Taking false branch}} + // expected-note@-1 {{Assuming the condition is false}} + // expected-note@-2 {{Taking false branch}} return !MCAsmParser::Error(); } F.Field = 0; - // class-note@-1 {{The value 0 is assigned to 'F.Field'}} + // expected-note@-1 {{The value 0 is assigned to 'F.Field'}} return MCAsmParser::Error(); - // class-note@-1 {{Calling 'MCAsmParser::Error'}} - // class-note@-2 {{Returning from 'MCAsmParser::Error'}} - // class-note@-3 {{Returning zero, which participates in a condition later}} + // expected-note@-1 {{Calling 'MCAsmParser::Error'}} + // expected-note@-2 {{Returning from 'MCAsmParser::Error'}} + // expected-note@-3 {{Returning zero, which participates in a condition later}} } bool parseFile() { Foo F; if (parseFoo(F)) { - // class-note@-1 {{Calling 'parseFoo'}} - // class-note@-2 {{Returning from 'parseFoo'}} - // class-note@-3 {{Taking false branch}} + // expected-note@-1 {{Calling 'parseFoo'}} + // expected-note@-2 {{Returning from 'parseFoo'}} + // expected-note@-3 {{Taking false branch}} return true; } (void)(1 / F.Field); - // class-warning@-1 {{Division by zero}} - // class-note@-2 {{Division by zero}} + // expected-warning@-1 {{Division by zero}} + // expected-note@-2 {{Division by zero}} return false; } -} // namespace test_classes +} // namespace test_break Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp =================================================================== --- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp +++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp @@ -174,6 +174,7 @@ basic_string s; s.c_str(); })code"; + const std::string Code = (Twine{MockStdStringHeader} + AdditionalCode).str(); EXPECT_TRUE(tooling::runToolOnCode( std::unique_ptr(new CallDescriptionAction<>({ @@ -188,6 +189,7 @@ using namespace std; basic_string s("hello"); })code"; + const std::string Code = (Twine{MockStdStringHeader} + AdditionalCode).str(); EXPECT_TRUE(tooling::runToolOnCode( std::unique_ptr( @@ -216,6 +218,7 @@ aaa::bbb::Bar x; int tmp = x; })code"; + EXPECT_TRUE(tooling::runToolOnCode( std::unique_ptr(new CallDescriptionAction<>({ {{{"aaa", "bbb", "Bar", "operator int"}}, true}, @@ -239,10 +242,9 @@ v.data(); })code"; - // FIXME: We should **not** match. EXPECT_TRUE(tooling::runToolOnCode( std::unique_ptr(new CallDescriptionAction<>({ - {{{"std", "container", "data"}}, true}, + {{{"std", "container", "data"}}, false}, })), Code)); } @@ -267,11 +269,10 @@ Code)); } { - // FIXME: We should **not** skip non-inline namespaces. SCOPED_TRACE("my bar"); EXPECT_TRUE(tooling::runToolOnCode( std::unique_ptr(new CallDescriptionAction<>({ - {{{"my", "bar"}}, true}, + {{{"my", "bar"}}, false}, })), Code)); } @@ -307,7 +308,67 @@ } } -TEST(CallDescription, SkipAnonimousNamespaces) { +TEST(CallDescription, InlineNamespaces) { + constexpr auto Code = R"code( + namespace X { inline namespace X { void f(); } } + void foo() { + X::f(); + })code"; + + // One can refer to 'f' by 'X::f()' or 'X::X::f()' in code. + // Hence, it would be intuitive to have matches using any of + // {"?", "f"}, {"?", "?", "f"}. + // However, wildcards won't consume //inline// namespaces, + // thus only {"?", "f"} wildcarded call description will match. + { + SCOPED_TRACE("X X f"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"X", "X", "f"}}, true}, + })), + Code)); + } + { + // Wildcards won't consume //inline// namespaces. + SCOPED_TRACE("X ? f"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"X", "?", "f"}}, false}, + })), + Code)); + } + { + // FIXME: Since the inline namespace has the name "X", it will eagerly + // consume the "X" from the CallDescription, but after that it will end the + // match and have unmatched non-skippable name parts, thus we reject the + // match. + SCOPED_TRACE("X f"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"X", "f"}}, false}, + })), + Code)); + } + { + // The wildcard consumed the outer 'X' namespace. + SCOPED_TRACE("? f"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"?", "f"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("? ? f"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"?", "?", "f"}}, false}, + })), + Code)); + } +} + +TEST(CallDescription, SkipAnonymousNamespaces) { constexpr auto Code = R"code( namespace { namespace std { @@ -417,6 +478,7 @@ void foo() { aaa::bbb_alias::ccc::bar(); })code"; + { SCOPED_TRACE("aaa bbb ccc bar"); EXPECT_TRUE(tooling::runToolOnCode( @@ -448,6 +510,7 @@ using namespace aaa_bbb_ccc; bar(); })code"; + { SCOPED_TRACE("aaa bbb ccc bar"); EXPECT_TRUE(tooling::runToolOnCode( @@ -489,6 +552,228 @@ "}")); } +TEST(CallDescription, WildcardNamespace) { + constexpr StringRef Code = R"code( + namespace aaa { + namespace bbb { + void bar(); + }} // namespace aaa::bbb + void foo() { + aaa::bbb::bar(); + })code"; + + { + SCOPED_TRACE("? bbb bar"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"?", "bbb", "bar"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("aaa ? bar"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"aaa", "?", "bar"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("? ? bar"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"?", "?", "bar"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("? bar"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"?", "bar"}}, false}, + })), + Code)); + } + + // Negative cases: + { + SCOPED_TRACE("? ? ? bar"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"?", "?", "?", "bar"}}, false}, + })), + Code)); + } + { + SCOPED_TRACE("zzz ? bar"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"zzz", "?", "bar"}}, false}, + })), + Code)); + } + { + SCOPED_TRACE("? zzz bar"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"?", "zzz", "bar"}}, false}, + })), + Code)); + } +} + +/// Not testing anonymous structs/unions since they cannot have any methods that +/// we could match for. + +TEST(CallDescription, WildcardUnnamedStructs) { + constexpr auto Code = R"code( + namespace { + namespace std { + inline namespace { + struct { + struct foobar { + struct container { + const char *data() const { return nullptr; }; + }; // struct container + }; // struct foobar + } aaa; + } // namespace anonymous + } // namespace std + } // namespace anonymous + + void foo() { + decltype(std::aaa)::foobar::container v; + v.data(); + })code"; + + { + // This test feels odd, since one cannot refer to the nameless type of + // `aaa`. We can circumvent this limitation by using decltype. Frankly, one + // cannot declare a variable using `std::foobar::container v`. Note: + // anonymous structs/unions represent a different thing, when they are + // nameless AND there is no instance of them. + SCOPED_TRACE("without wildcard"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"std", "foobar", "container", "data"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("? foobar container data"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"?", "foobar", "container", "data"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("std ? container data"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"std", "?", "container", "data"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("std foobar ? data"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"std", "foobar", "?", "data"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("std ? ? data"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"std", "?", "?", "data"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("? foobar ? data"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"?", "foobar", "?", "data"}}, true}, + })), + Code)); + } +} + +// This is the same testcase as WildcardUnnamedStructs, but with unnamed union. +TEST(CallDescription, WildcardUnnamedUnions) { + constexpr auto Code = R"code( + namespace { + namespace std { + inline namespace { + union { + struct foobar { + struct container { + const char *data() const { return nullptr; }; + }; // struct container + }; // struct foobar + } aaa; + } // namespace anonymous + } // namespace std + } // namespace anonymous + + void foo() { + decltype(std::aaa)::foobar::container v; + v.data(); + })code"; + + { + // Same concerns apply that was for WildcardUnnamedStructs. + SCOPED_TRACE("without wildcard"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"std", "foobar", "container", "data"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("? foobar container data"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"?", "foobar", "container", "data"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("std ? container data"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"std", "?", "container", "data"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("std foobar ? data"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"std", "foobar", "?", "data"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("std ? ? data"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"std", "?", "?", "data"}}, true}, + })), + Code)); + } + { + SCOPED_TRACE("? foobar ? data"); + EXPECT_TRUE(tooling::runToolOnCode( + std::unique_ptr(new CallDescriptionAction<>({ + {{{"?", "foobar", "?", "data"}}, true}, + })), + Code)); + } +} + } // namespace } // namespace ento } // namespace clang