Page MenuHomePhabricator

[analyzer] Support function argument constructors.

Authored by NoQ on Jul 17 2018, 11:56 AM.



I've got a simple example working, but there's quite a bit of work remaining on this patch for now.

Argument constructor is being called with a target region that is a parameter variable region that corresponds to the stack frame of the future function call. The region is tracked within the program state until we evaluate the call itself in order to ensure that bindings are not garbage-collected.

The annoying part is, of course, to make sure we guess the future stack frame correctly. I need to re-use some code and add some assertions to make sure i'm doing it right, and additionally it requires mapping an expression back to CFG, which i currently do via a CFGStmtMap instead of passing it directly (i'm not sure i'll be committing into finding the beautiful solution here, because there are other bigger issues for now).

The other annoying part is to match parameters to arguments, because operator calls have one of these shifted by 1, and also there are C-style variadic functions. I need to make a reusable code to make this easier.

Support for constructors of placement arguments of operator new() is still pending, even in CFG. Not hard, but also annoying.

Last but not least, once we inline the constructors, we need to invalidate the parameter region itself (not just regions reachable from it!) when the call is evaluated conservatively because (D48249). I marked these tests as FIXME for now but i plan to fix them before committing. That's also blocked on matching arguments to parameters.

Diff Detail


Event Timeline

NoQ created this revision.Jul 17 2018, 11:56 AM

Cool stuff! I didn't look at things line-by-line, but here's a few comments anyway.

310 ↗(On Diff #155939)

Remember that this might be null, for a function pointer call, or may not match up with the actual executed body, for a virtual call (cf. CallEvent::getRuntimeDefinition). What happens in those cases?

338 ↗(On Diff #155939)

Are you sure this isn't just a member vs. non-member function problem? Or static vs. non-static member?

NoQ added a comment.Jul 18 2018, 12:00 PM

Np, i only poked you because you were particularly curious about this case^^

310 ↗(On Diff #155939)

Yup, that's one of the dirty parts. It seems that getDecl()'s overrides are very smart (i.e., for function pointer calls they lookup the value of the function pointer sub-expression in the Environment), but i'm not sure it works correctly in the general case. For now it's untested.

338 ↗(On Diff #155939)

Yeah, i guess we should just solve these problems in a general case: "How do i obtain the function that's actually called", "How do i obtain its parameter that corresponds to a certain argument", etc.

This one seems to have failed only for operator calls, and they are indeed specifically squishy sometimes, but i didn't actually check it carefully.

NoQ added inline comments.Jul 18 2018, 4:55 PM
310 ↗(On Diff #155939)

Ok, so facts here are as follows. getRuntimeDefinition().getDecl() returns a definition that can be inlined. If there's no such definition, it returns a null decl. getDecl(), on the other hand, always succeeds, but doesn't always return a definition, it may return a forward declaration instead.

So if we are going to inline a function, getRuntimeDefinition() is necessary. If not, getDecl() used to be sufficient because previously we only cared about number of arguments and their types, which is enough for invalidation purposes. That is, when we aren't inlining, we don't care if we're using the right override; they're all the same to us.

But for the purposes of this patch we do actually care because we want to eventually obtain the exact ParmVarDecl that corresponds to the exact function declaration that is going to be "called". Then, again, it wouldn't immediately matter because that decl isn't used for any sort of evaluation. Once we start invalidating the argument storage, as per aforementioned pr37459, we'll need to make sure that we're using the same decl here and during invalidation, but not necessarily the correct one. Finally, when we start writing checkers that check specific overrides of virtual functions, we'll need to make sure that we're in fact using the correct decl, so that when a checker gets the correct ParmVarDecl it finds the bindings it expects to see within it.

So for now Call.getRuntimeDefinition().getDecl() || Call.getDecl() is going to work, but i might end up trying to come up with a getRuntimeDeclaration() method that also takes everything into account but isn't forced to return a definition, so that we were also correct.

NoQ added inline comments.Jul 20 2018, 4:52 PM
338 ↗(On Diff #155939)

Call.parameters() is pretty good, but here we have an AST problem: operator declaration is a CXXMethodDecl and it accepts this as, well, this, while CXXOperatorCallExpr is not a sub-class of CXXMemberCallExpr, so it accepts this as its first argument instead. So there's AST inconsistency that's annoying to work around.

NoQ updated this revision to Diff 156656.Jul 20 2018, 6:19 PM

Handle the decl problem (somehow) and the operator problem (more or less, with the help of D49627), add respective tests.

Passing a non-POD object into a variadic function is implementation-defined with no requirement to work whatsoever. For the test case with bar5(7, S(7)), the AST is roughly as follows:

|     |-ExprWithCleanups 0x7f9d1b013830 <line:1041:3, col:15> 'void'
|     | `-CallExpr 0x7f9d1b012da0 <col:3, col:15> 'void'
|     |   |-ImplicitCastExpr 0x7f9d1b012d88 <col:3> 'void (*)(int, ...)' <FunctionToPointerDecay>
|     |   | `-DeclRefExpr 0x7f9d1b012d30 <col:3> 'void (int, ...)' lvalue Function 0x7f9d1b0112c0 'bar5' 'void (int, ...)'
|     |   |-IntegerLiteral 0x7f9d1b012c58 <col:8> 'int' 7
|     |   `-BinaryOperator 0x7f9d1b013808 <col:11, col:14> 'arguments::S' ','
|     |     |-CallExpr 0x7f9d1b012fe0 <col:11, col:14> 'void'
|     |     | `-ImplicitCastExpr 0x7f9d1b012fc8 <col:11> 'void (*)() __attribute__((noreturn)) throw()' <BuiltinFnToFnPtr>
|     |     |   `-DeclRefExpr 0x7f9d1b012f48 <col:11> '<builtin fn type>' Function 0x7f9d1b012e60 '__builtin_trap' 'void () __attribute__((noreturn)) throw()'
|     |     `-CXXFunctionalCastExpr 0x7f9d1b012d08 <col:11, col:14> 'arguments::S' functional cast to struct arguments::S <ConstructorConversion>
|     |       `-CXXBindTemporaryExpr 0x7f9d1b012ce8 <col:11, col:14> 'arguments::S' (CXXTemporary 0x7f9d1b012ce0)
|     |         `-CXXConstructExpr 0x7f9d1b012ca8 <col:11, col:14> 'arguments::S' 'void (int)'
|     |           `-IntegerLiteral 0x7f9d1b012c88 <col:13> 'int' 7

(notice the __builtin_trap). So we won't analyze much here. We still need to support non-variadic arguments of variadic functions, but i think this can wait.

NoQ updated this revision to Diff 156960.Jul 23 2018, 7:29 PM

Move future stack frame lookup to CallEvent and split it out into D49715.

MTC added a subscriber: MTC.Jul 26 2018, 8:06 PM
NoQ updated this revision to Diff 157790.Jul 27 2018, 3:24 PM

Fix the regression in temporaries.cpp/ with IntPtr by invalidating parameter variable regions, as intended. Delay cleanup for that purpose. Next i'll add some lifetime extension address tests and move it out of WIP.

NoQ updated this revision to Diff 158150.Jul 30 2018, 6:05 PM

Add tests for making sure that the address of the object doesn't jump around.

I'm removing the WIP: tag. The patch should be ready for review.

NoQ retitled this revision from [analyzer] WIP: Support function argument constructors. to [analyzer] Support function argument constructors..Jul 30 2018, 6:06 PM
NoQ updated this revision to Diff 158818.Aug 2 2018, 12:39 PM

Rebase. Fix a bug in isArgumentConstructedDirectly() that accidentally slipped through D49715.

NoQ updated this revision to Diff 158833.Aug 2 2018, 2:04 PM

Stop re-binding arguments to parameter regions in enterStackFrame()->addParameterValuesToBindings() when these are constructed directly in the parameter region. This was mostly harmless because it's just re-binding the same value, but because that becomes a lazy binding, and RegionStore is known to be bad with handling partial/overlapping lazy bindings, *some* functional change is observed, as demonstrated by the test.

NoQ updated this revision to Diff 158846.Aug 2 2018, 3:28 PM

Apparently, there are not 2 but 3 different ways of enumerating arguments/parameters: CallEvent uses its own, which keeps arguments and parameters in sync, but is incompatible with argument numbering in AST expressions.

Fix finishArgumentConstruction() to convert CallEvent index to AST index using a newly introduced helper function, getASTArgumentIndex(). Add a test.

NoQ updated this revision to Diff 159145.Aug 3 2018, 5:34 PM

Finish argument construction when Objective-C message has a nil receiver (there's a separate code path to model this situation). Tested as testNilReceiverCleanup(). While debugging this, found a potentially hazardous piece of code in NilArgChecker and documented it.

NoQ updated this revision to Diff 159442.Aug 6 2018, 7:11 PM

Fix getCalleeAnalysisDeclContext() to work correctly when a virtual function's definition is located below its call site and is being called on a non-region. Because definition lookup. Logic in this function is becoming really weird. I hope to get to it some day for a proper refactoring.

NoQ added a comment.Aug 6 2018, 7:19 PM

I guess i should have skipped argument constructor support completely when the call doesn't have a runtime definition. It's not very useful anyway. But too late.

NoQ updated this revision to Diff 159843.Aug 8 2018, 6:34 PM

Found more crashes and decided to give up on argument constructors of functions without definitions, at least for now. Because the function would not be inlined anyway, loss of precision shouldn't be that bad (unless a checker tries to model the function, in this case we'd rather have constructors working correctly). It hasn't been too hard to give up on this.

Also make sure to use the definition's parameters() instead of Call.getDecl()'s parameters(), to avoid conflicting ParmVarDecls.

I see no more known crashes and the patch seems to be working as intended.

Looks reasonable in general.
For all the FIXME cases would it make sense to add logging there and explicitly test for those?

323 ↗(On Diff #159843)

first two if's could be joined with &&

This revision is now accepted and ready to land.Aug 10 2018, 11:41 AM
This revision was automatically updated to reflect the committed changes.