[analysis] Introduce an AnyCall helper class, for abstraction over different callables

[george.karpenkov]

Summary

A lot of code, particularly in the analyzer, has to perform a lot of duplication to handle functions/ObjCMessages/destructors/constructors in a generic setting.
The analyzer already has a CallEvent helper class abstracting over such calls, but it's not always suitable, since it's tightly coupled to other analyzer classes (ExplodedNode, ProgramState, etc.) and it's not always possible to construct.

This change introduces a very simple, very lightweight helper class to do simple generic operations over callables.

In future, parts of CallEvent could be changed to use this class to avoid some duplication.

[NoQ]

it's not always suitable, since it's tightly coupled to other analyzer classes (ExplodedNode, ProgramState, etc.) and it's not always possible to construct

And, most importantly, it means a completely different thing :) i.e., an event that occurs on a specific execution path rather than a syntactic call site that we're trying to describe here.

clang/include/clang/Analysis/GenericCall.h

42

Maybe just make two constructors - one from Expr, one from Decl? 'Cause you never use both of these.

60

Hmm, i think for an unknown block pointer CD would also be null.

65–67

We don't have this case in the Analyzer. I guess we simply don't support overloaded delete operators up there?

100

This is often not what the user wants, especially for calls that return references (because expressions of reference type don't exist).

Cf. CallExpr::getCallReturnType().

104–110

Maybe just cast to NamedDecl?

126–129

Maybe dump kind as well?

[NoQ]

clang/include/clang/Analysis/GenericCall.h

42

Another reason why i think it's a good idea is that you'll be able to pass Exprs or Decls directly into functions that accept a GenericCall, without explicitly calling a GenericCall constructor.

[george.karpenkov]

clang/include/clang/Analysis/GenericCall.h

42

I'm against two constructors, and definitely against constructing GenericCall from Decl implicitly - that's very confusing, because it will not do what you think it would do (it would always construct a destructor)

[NoQ]

clang/include/clang/Analysis/GenericCall.h

42

What about constructing from CXXDestructorDecl?

[george.karpenkov]

clang/include/clang/Analysis/GenericCall.h

60

How to differentiate between the two then?

100

Again I'm not sure that's the best approach here. I would be fine with switching over decl types and getting types from there.

126–129

Not sure - it's a lot of boilerplate, and debuggers would show it just fine.

[NoQ]

clang/include/clang/Analysis/GenericCall.h

60

CodeGen seems to rely upon CE->getCallee()->getType()->getAs<BlockPointerType>()

100

I think if you simply delegate all the work to CallExpr;:getCallReturnType() when possible, it'll cover all cases except a few Objective-C++ cases which can be FIXME'd out. In fact, probably the whole thing can be FIXME'd out.

[rjmccall]

clang/include/clang/Analysis/GenericCall.h

22

CallSite, maybe? I don't think we have precedent for this Generic prefix.

24

I would expect this to just be called Kind.

31

Documentation! What are each of these cases, and what do each of them guarantee vis-a-vis E and D? Does Destructor correspond only to a direct destructor call, or does it include a delete, or does it also include implicit calls on scope exit? If the latter, how do you want to represent __attribute__((cleanup)) calls?

Do you not want to distinguish between functions and C++ instance methods? And if you do distinguish the latter, do you want to distinguish direct calls vs. virtual calls vs. pointer-to-member calls? I'm asking more than suggesting here — my intuition would say that functions and methods should be distinguished, but maybe the different method cases shouldn't be, especially since this also seems not to distinguish direct function calls from function-pointer calls.

Do you want to distinguish between complete-object and base-subobject construction and destruction?

C++ refers to these last two cases as "allocation functions" and "deallocation functions", for what it's worth.

36

Comments explaining when these are set, please. (I think D is set whenever there's a statically-known callee, right?)

44

Please be more concrete about what happens when a single expression potentially corresponds to multiple calls, such as a delete or new expression. Maybe you should have factories to get the other call sites associated with the expression.

50

This feels like it ought to be a different constructor, or better yet a factory method like "forDestructorCall", and then you won't need the InputDecl parameter at all.

[rjmccall]

clang/include/clang/Analysis/AnyCall.h

36 ↗

(On Diff #183362)

So this is not used for explicit a.~A() calls or the destructor call in delete? Worth clarifying.

41 ↗

(On Diff #183362)

Please clarify that this is the call to operator new associated with a new expression, and analogously with Deallocator.

Also, you're inconsistent about periods in these comments.

62 ↗

(On Diff #183362)

I really think that having a constructor that requires an Expr* and a factory method that takes a CXXDestructorDecl is the right way to go. And again, you should clarify which call site will be used for expressions that have multiple calls associated with them.

...actually, I'm not sure I like this constructor at all. It's *almost* a way to check whether a particular expression is a call, except there's no null case so it has no option but to assert. Why not have a different constructor for each of these cases? Most construction sites will know which Expr they have statically, and it becomes an obvious place to hang documentation about which call the ensuing CallSite refers to. You can have a factory that returns an Optional<CallSite> that checks whether an expression is a known call site.

Factory methods can be implemented with a private constructor.

107 ↗

(On Diff #183362)

Documentation! It's important to know that this is (1) just the formal parameters and (2) only if this is a direct call (including virtual calls).

134 ↗

(On Diff #183362)

This seems nothing but treacherous. The behavior with reference returns is especially problematic: it's a reference type if this is a direct call, otherwise it's the underlying referent with no indication of the difference?

You should require an ASTContext and build the right result in all cases. This will also allow you to return the correct thing for destructor calls instead of crashing.

137 ↗

(On Diff #183362)

Documentation! Also, D can be null here.

[george.karpenkov]

clang/include/clang/Analysis/AnyCall.h

62 ↗

(On Diff #183362)

I really think that having a constructor that requires an Expr* and a factory method that takes a CXXDestructorDecl is the right way to go. And again, you should clarify which call site will be used for expressions that have multiple calls associated with them.

This constructor is private and is effectively an implementation detail.
So that's effectively what we have: a constructor and a factory method.

clang/include/clang/Analysis/GenericCall.h

22

Maybe AnyCall?

31

Thanks for your comments!
I have added some documentation. For simplicity, I don't want to handle CXX methods separately.

44

Sorry, could you clarify what happens in those cases?
Would it be alright not to handle them for the first iteration of this class?

50

OK, I've made this constructor private.

[NoQ]

Hmm, looks correct to me now.

clang/include/clang/Analysis/AnyCall.h

73–84 ↗

(On Diff #183409)

D = CE->getCalleeDecl();
K = (CE->getCallee()->getType()->getAs<BlockPointerType>())) ? Block : Function;

105 ↗

(On Diff #183409)

CXXDestructorDecl?

152–153 ↗

(On Diff #183409)

dyn_cast_or_null?

[rjmccall]

clang/include/clang/Analysis/AnyCall.h

62 ↗

(On Diff #183362)

It just feels more natural to only have implicit conversions from the specific subclasses where this is known to be valid.

clang/include/clang/Analysis/GenericCall.h

44

Hmm. For new it's actually fine because the constructor call (if any) is already separately modeled as a CXXConstructExpr in the operand. I don't know if you care about modeling the call to operator delete that happens along the exception path.

For CXXDeleteExpr, there's always a call to the operator delete, and then there might also be a call to the destructor. I guess you could represent that destructor call like every destructor call, completely dis-associated from an expression, but you might also consider adding a Kind that represents the destructor call that's performed as part of the delete. Also note that with virtual destructors you might actually not know which operator delete is being called by delete statically.

I think it's fine to ignore these details in the first draft as long as you aren't designing the type in a way that prevents these things from being expressed. That's why I'm talking a lot about how clients construct one of these.

[rjmccall]

One minor note, but feel free to commit after fixing that.

clang/include/clang/Analysis/AnyCall.h

68 ↗

(On Diff #183441)

Indentation is wrong in this function.

clang/include/clang/Analysis/GenericCall.h

+//=== GenericCall.h - Abstraction over different callables --------*- C++ -*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// A utility class for performing generic operations over different callables.
+//
+//===----------------------------------------------------------------------===//
+//
+#ifndef LLVM_CLANG_ANALYSIS_GENERIC_CALL_H
+#define LLVM_CLANG_ANALYSIS_GENERIC_CALL_H
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/AST/ExprObjC.h"
+
+namespace clang {
+
+class GenericCall {
+public:
+  enum CallKind {
+    Function,
+    ObjCMethod,
+    Block,
+    Destructor,
+    Constructor,
+    Allocator,
+    Deallocator
+  };
+
+private:
+  const Expr *E = nullptr;
+  const Decl *D = nullptr;
+  CallKind K;
+
+public:
+
+  /// Construct a generic call.
+  /// \param E Call expression, if exists (e.g. does not exist for destructors)
+  /// \param InputDecl Declaration of the called function: can be left empty
+  /// iff {@code E} is not empty.
+  GenericCall(const Expr *E,
+              const Decl *InputDecl = nullptr) : E(E) {
+    if (!E) {
+      assert(InputDecl);
+      D = InputDecl;
+      K = Destructor;
+    } else if (const auto *ME = dyn_cast<ObjCMessageExpr>(E)) {
+      D = ME->getMethodDecl();
+      K = ObjCMethod;
+    } else if (const auto *CE = dyn_cast<CallExpr>(E)) {
+      const Decl *CD = CE->getCalleeDecl();
+      if (CD && isa<FunctionDecl>(CD)) {
+        D = CD;
+        K = Function;
+      } else if (CD && isa<BlockDecl>(CD)) {
+        D = CD;
+        K = Block;
+      } else {
+        D = nullptr;
+        K = Function;
+      }
+    } else if (const auto *CXNE = dyn_cast<CXXNewExpr>(E)) {
+      D = CXNE->getOperatorNew();
+      K = Allocator;
+    } else if (const auto *CXDE = dyn_cast<CXXDeleteExpr>(E)) {
+      D = CXDE->getOperatorDelete();
+      K = Deallocator;
+    } else if (const auto *CXCE = dyn_cast<CXXConstructExpr>(E)) {
+      D = CXCE->getConstructor();
+      K = Constructor;
+    } else {
+      llvm_unreachable("Unexpected type");
+    }
+  }
+
+  ArrayRef<ParmVarDecl *> parameters() const {
+    if (!D)
+      return None;
+
+    if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+      return FD->parameters();
+    } else if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
+      return MD->parameters();
+    } else if (const auto *CD = dyn_cast<CXXConstructorDecl>(D)) {
+      return CD->parameters();
+    } else if (const auto *BD = dyn_cast<BlockDecl>(D)) {
+      return BD->parameters();
+    } else {
+      return None;
+    }
+  }
+
+  using param_const_iterator = ArrayRef<ParmVarDecl *>::const_iterator;
+  param_const_iterator param_begin() const { return parameters().begin(); }
+  param_const_iterator param_end() const { return parameters().end(); }
+  size_t param_size() const { return parameters().size(); }
+  bool param_empty() const { return parameters().empty(); }
+
+  QualType getReturnType() const {
+    return E->getType();
+  }
+
+  const IdentifierInfo *getIdentifier() const {
+    if (const auto *ND = dyn_cast<NamedDecl>(D)) {
+      return ND->getIdentifier();
+    } else {
+      return nullptr;
+    }
+  }
+
+  const Decl *getDecl() const {
+    return D;
+  }
+
+  const Expr *getExpr() const {
+    return E;
+  }
+
+  CallKind getKind() const {
+    return K;
+  }
+
+  void dump() const {
+    if (E)
+      E->dump();
+    if (D)
+      D->dump();
+  }
+};
+
+}
+
+#endif // LLVM_CLANG_ANALYSIS_GENERIC_CALL_H