[analyzer] NFC: CallDescription: Implement describing C library functions.

[NoQ]

Summary

When matching C standard library functions in the checker, it's easy to forget that they are often implemented as macros that are expanded to compiler builtins. Such builtins would have a different name, so matching the callee identifier would fail, or may sometimes have more arguments than expected (so matching the exact number of arguments would fail, but this is fine as long as we have all the arguments that we need in their respective places.

This patch adds a set of flags to the CallDescription class so that to handle various special matching rules, and adds the first flag into this set, which enables a more fuzzy matching for functions that may be implemented as builtins.

[a_sidorin]

Hi Artem,
Looks mostly good, but I have some comments inline.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1064 ↗

(On Diff #201785)

Not for this review, but why do we have a vector of const char *, not StringRefs?

1066 ↗

(On Diff #201785)

Is it a good idea to make Flags a bitfield structure?

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

379 ↗

(On Diff #201785)

!II is never false due to the newly-introduced early return.

[Szelethus]

I like the idea, but will need more time to get familiar with CallEvent to formally accept.

When matching C standard library functions in the checker, it's easy to forget that they are often implemented as macros that are expanded to compiler builtins. Such builtins would have a different name, so matching the callee identifier would fail, or may sometimes have more arguments than expected (so matching the exact number of arguments would fail, but this is fine as long as we have all the arguments that we need in their respective places.

Just a name an issue that came up recently, in the case of D59812, we probably shouldn't need to strip "__builtin_" off.

[xazax.hun]

Once Aleksei's comments are resolved it is good to go. My comments are notes and not requests.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1052 ↗

(On Diff #201785)

I wonder if this is the right name. Do checker authors know the relationship between a standard library function and builtins? Maybe something like CDF_IsCStandardLibFunc would be more prescriptive when to set this? Or do we have non standard library builtins that we might want to match using this facility? The comment above is great, I just wonder if checker authors will read that. In case it does show up in the doxygen feel free to leave the name as is.

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

368 ↗

(On Diff #201785)

In case this happens to be a performance problem we could cache the builtin id in the future. This is just a note, do not need to optimize this prematurely.

[NoQ]

Fxd.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1064 ↗

(On Diff #201785)

The constructor accepts ArrayRef<const char *> rather than ArrayRef<StringRef> because { "foo", "bar" } is not a valid initializer for ArrayRef<StringRef>. The field is the same for simplicity, as converting ArrayRef<const char *> to a vector<StringRef> is annoying and not really useful. See also D51390.

1066 ↗

(On Diff #201785)

This would prevent us from passing the flags into the constructor as CDF_Foo | CDF_Bar etc.- we'll have to specify every field on every line. I wanted to make the initializers as concise as possible to avoid line breaks in code like D62557.

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

379 ↗

(On Diff #201785)

Right!

[Charusso]

I have not seen any problem. Thanks!

[NoQ]

Rebase!

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

public:
 
   Kind getKind() const override { return CE_ObjCMessage; }
 
   static bool classof(const CallEvent *CA) {
     return CA->getKind() == CE_ObjCMessage;
   }
 };
 
+enum CallDescriptionFlags : int {
+  /// Describes a C standard function that is sometimes implemented as a macro
+  /// that expands to a compiler builtin with some __builtin prefix.
+  /// The builtin may as well have a few extra arguments on top of the requested
+  /// number of arguments.
+  CDF_MaybeBuiltin = 1 << 0,
+};
+
 /// This class represents a description of a function call using the number of
 /// arguments and the name of the function.
 class CallDescription {
   friend CallEvent;
 
   mutable IdentifierInfo *II = nullptr;
   mutable bool IsLookupDone = false;
   // The list of the qualified names used to identify the specified CallEvent,
   // e.g. "{a, b}" represent the qualified names, like "a::b".
   std::vector<const char *> QualifiedName;
   Optional<unsigned> RequiredArgs;
+  int Flags;
 
 public:
   /// 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
   /// std::basic_string<...>::c_str() and std::__1::basic_string<...>::c_str().
   ///
   /// @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
   /// name regardless the number of arguments.
+  CallDescription(int Flags, ArrayRef<const char *> QualifiedName,
+                  Optional<unsigned> RequiredArgs = None)
+      : QualifiedName(QualifiedName), RequiredArgs(RequiredArgs),
+        Flags(Flags) {}
+
+  /// Construct a CallDescription with default flags.
   CallDescription(ArrayRef<const char *> QualifiedName,
                   Optional<unsigned> RequiredArgs = None)
-      : QualifiedName(QualifiedName), RequiredArgs(RequiredArgs) {}
+      : CallDescription(0, QualifiedName, RequiredArgs) {}
 
   /// Get the name of the function that this object matches.
   StringRef getFunctionName() const { return QualifiedName.back(); }
 };
 
 /// An immutable map from CallDescriptions to arbitrary data. Provides a unified
 /// way for checkers to react on function calls.
 template <typename T> class CallDescriptionMap {

cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp

if (IsPreVisit)
     return PreImplicitCall(D, Loc, getLocationContext(), Tag);
   return PostImplicitCall(D, Loc, getLocationContext(), Tag);
 }
 
 bool CallEvent::isCalled(const CallDescription &CD) const {
   // FIXME: Add ObjC Message support.
   if (getKind() == CE_ObjCMessage)
     return false;
+
+  const IdentifierInfo *II = getCalleeIdentifier();
+  if (!II)
+    return false;
+  const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(getDecl());
+  if (!FD)
+    return false;
+
+  if (CD.Flags & CDF_MaybeBuiltin) {
+    return CheckerContext::isCLibraryFunction(FD, CD.getFunctionName()) &&
+           (!CD.RequiredArgs || CD.RequiredArgs <= getNumArgs());
+  }
+
   if (!CD.IsLookupDone) {
     CD.IsLookupDone = true;
     CD.II = &getState()->getStateManager().getContext().Idents.get(
         CD.getFunctionName());
   }
-  const IdentifierInfo *II = getCalleeIdentifier();
-  if (!II || II != CD.II)
+
+  if (II != CD.II)
     return false;
 
-  const Decl *D = getDecl();
   // If CallDescription provides prefix names, use them to improve matching
   // accuracy.
-  if (CD.QualifiedName.size() > 1 && D) {
-    const DeclContext *Ctx = D->getDeclContext();
+  if (CD.QualifiedName.size() > 1 && FD) {
+    const DeclContext *Ctx = FD->getDeclContext();
     // See if we'll be able to match them all.
     size_t NumUnmatched = CD.QualifiedName.size() - 1;
     for (; Ctx && isa<NamedDecl>(Ctx); Ctx = Ctx->getParent()) {
       if (NumUnmatched == 0)
         break;
 
       if (const auto *ND = dyn_cast<NamespaceDecl>(Ctx)) {
         if (ND->getName() == CD.QualifiedName[NumUnmatched - 1])

cfe/trunk/unittests/StaticAnalyzer/CallDescriptionTest.cpp

TEST(CallEvent, CallDescription) {
 
   // A negative test for qualified names.
   EXPECT_TRUE(tooling::runToolOnCode(
       new CallDescriptionAction({
           {{{"foo", "bar"}}, false},
           {{{"bar", "foo"}}, false},
           {{"foo"}, true},
       }), "void foo(); struct bar { void foo(); }; void test() { foo(); }"));
+
+  // Test CDF_MaybeBuiltin - a flag that allows matching weird builtins.
+  EXPECT_TRUE(tooling::runToolOnCode(
+      new CallDescriptionAction({
+          {{"memset", 3}, false},
+          {{CDF_MaybeBuiltin, "memset", 3}, true}
+      }),
+      "void foo() {"
+      "  int x;"
+      "  __builtin___memset_chk(&x, 0, sizeof(x),"
+      "                         __builtin_object_size(&x, 0));"
+      "}"));
 }
 
 } // namespace
 } // namespace ento
 } // namespace clang