Page MenuHomePhabricator

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

Authored by george.karpenkov on Jan 23 2019, 4:27 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

george.karpenkov created this revision.
NoQ added a comment.Jan 23 2019, 4:41 PM

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
41 ↗(On Diff #183205)

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

59 ↗(On Diff #183205)

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

64–66 ↗(On Diff #183205)

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

99 ↗(On Diff #183205)

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().

103–109 ↗(On Diff #183205)

Maybe just cast to NamedDecl?

125–128 ↗(On Diff #183205)

Maybe dump kind as well?

NoQ added inline comments.Jan 23 2019, 4:49 PM
clang/include/clang/Analysis/GenericCall.h
41 ↗(On Diff #183205)

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 marked an inline comment as done.Jan 23 2019, 5:24 PM
george.karpenkov added inline comments.
clang/include/clang/Analysis/GenericCall.h
41 ↗(On Diff #183205)

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 added inline comments.Jan 23 2019, 5:27 PM
clang/include/clang/Analysis/GenericCall.h
41 ↗(On Diff #183205)

What about constructing from CXXDestructorDecl?

george.karpenkov marked 4 inline comments as done.Jan 23 2019, 5:31 PM
george.karpenkov added inline comments.
clang/include/clang/Analysis/GenericCall.h
59 ↗(On Diff #183205)

How to differentiate between the two then?

99 ↗(On Diff #183205)

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.

125–128 ↗(On Diff #183205)

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

NoQ added inline comments.Jan 23 2019, 5:40 PM
clang/include/clang/Analysis/GenericCall.h
59 ↗(On Diff #183205)

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

99 ↗(On Diff #183205)

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 added inline comments.Jan 23 2019, 10:21 PM
clang/include/clang/Analysis/GenericCall.h
22 ↗(On Diff #183220)

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

24 ↗(On Diff #183220)

I would expect this to just be called Kind.

31 ↗(On Diff #183220)

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 ↗(On Diff #183220)

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

44 ↗(On Diff #183220)

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 ↗(On Diff #183220)

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.

george.karpenkov marked 6 inline comments as done.
george.karpenkov marked 5 inline comments as done.
rjmccall added inline comments.Jan 24 2019, 12:36 PM
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 marked an inline comment as done.Jan 24 2019, 1:49 PM
george.karpenkov added inline comments.
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 ↗(On Diff #183220)

Maybe AnyCall?

31 ↗(On Diff #183220)

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

44 ↗(On Diff #183220)

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 ↗(On Diff #183220)

OK, I've made this constructor private.

george.karpenkov marked 5 inline comments as done.
NoQ accepted this revision.Jan 24 2019, 3:09 PM

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?

This revision is now accepted and ready to land.Jan 24 2019, 3:09 PM
george.karpenkov marked 3 inline comments as done.
george.karpenkov retitled this revision from [analysis] Introduce a GenericCall helper class, for abstraction over different callables to [analysis] Introduce an AnyCall helper class, for abstraction over different callables.Jan 24 2019, 4:19 PM
rjmccall added inline comments.Jan 24 2019, 4:20 PM
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 ↗(On Diff #183220)

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 accepted this revision.Jan 24 2019, 5:17 PM

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.

This revision was automatically updated to reflect the committed changes.