Summary:
This diff adds fields for ObjC properties to the type environment. For example,
```
property type fieldname;
```
when the property name is "fieldname", this diff adds a struct field of the same name.
The missing type information were problematic in inferbo, since its semantics depend on types.
Reviewed By: ezgicicek
Differential Revision: D28421998
fbshipit-source-id: e24059846
Summary:
Follow similar approach as in the translation of dictionary literal to insert load instruction to catch nil insertion into collection issues. The missing load instruction was causing false negatives in biabduction. This will also help Pulse to catch nil insertion into collection issues for array literals.
Facebook
Reviewed By: skcho
Differential Revision: D28442642
fbshipit-source-id: b530ac21b
Summary:
This diff avoids dereference of C struct, in its frontend and its semantics of Pulse. In SIL, C
struct is not first-class value, thus dereferencing on it does not make sense.
Reviewed By: ezgicicek
Differential Revision: D27953258
fbshipit-source-id: 348d56338
Summary: This diff copies each field values inside setter/getter of ObjC.
Reviewed By: ezgicicek
Differential Revision: D27940521
fbshipit-source-id: 9977cae75
Summary:
We use `procedure_name` which is coming from `Procname.get_method` in explanation of cost issues. For blocks, procedure name includes a prefix `objc_block` and a suffix with `_x` where x is the block counter. However, displaying this name to the user is not pretty. Especially when we have nested blocks, procedure name looks like `objc_blockobjc_blockdirectUIMessageFromContentAndMetadata_10_23`.
This diff drops the block index suffix and replaces `objc_block` with a prettier version `^`(signifying block).
so instead, in the cost report, we will have `^^blockdirectUIMessageFromContentAndMetadata`.
Reviewed By: skcho
Differential Revision: D26945333
fbshipit-source-id: 9d135423c
Summary: Variables captured by reference do not have correct type in objc blocks. They are missing one reference. This diff sets the correct type of captured reference variables inside procdesc, similarly as we already have for cpp lambdas. The translation of block's body will then take into account the type of captured variable from procdesc.
Reviewed By: ezgicicek
Differential Revision: D26945575
fbshipit-source-id: 06a9d9cc6
Summary:
This diff resets the id generator before generating ObjC getter/setter, so parsed results are the
same without regard to the generation order. Note that the order may change when we change the type
of Procname.t since their hash values are used for the hash set of procnames.
Reviewed By: ngorogiannis
Differential Revision: D26277348
fbshipit-source-id: a66d77845
Summary:
Dear Infer team,
To contribute to Infer community, I would like to integrate infer#'s language agnostic layer into Infer.
Please help to review, discuss and consider to merge this feature.
Thanks,
Xiaoyu
Pull Request resolved: https://github.com/facebook/infer/pull/1361
Reviewed By: skcho
Differential Revision: D25928458
Pulled By: jvillard
fbshipit-source-id: 7726150b8
Summary: This diff adds an additional parameter of struct return type in ObjC's methods. The additional parameter had been supported only in C/C++ functions/methods for 5 years (D2865091 (ec80d40bdd)). If there is no specific reason not to do that, let's do it and fix the incorrect frontend translations.
Reviewed By: jvillard
Differential Revision: D26049748
fbshipit-source-id: 414b3011f
Summary:
When there was an assignment of C struct, `x = y;`, it was translated to the statements of load and store.
```
n$0 = *y
*x = n$0
```
However, this is incorrect in Sil, because a struct is not a value that can be assigned to registers. This diff fixes the translation as assignments of each field values :
```
n$0 = *y.field1
*x.field1 = n$0
n$0 = *y.field2
*x.field2 = n$0
...
```
It copies field values of C structs on:
* assign statement
* return statement
* declarations.
It supports nested structs.
Reviewed By: jvillard
Differential Revision: D25952894
fbshipit-source-id: 355f8db9c
Summary:
The frontend of ObjC regarding to captured variable was incorrect: it set capturing mode as
by-reference always, but it actually translaged as if all captured variables were passed with
by-value. This diff fixes this based on the document.
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Blocks/Articles/bxVariables.html
* global variable: by reference
* local variable: by value
* `static` local variable: by reference
* `__block` local variable: by reference
* parameter: by value
Reviewed By: jvillard
Differential Revision: D25306122
fbshipit-source-id: ec499d705
Summary:
This was left as a TODO before: where to place calls to destructors for
C++ temporaries that are only conditionally creating when evaluating an
expression. This can happen inside the branches of a conditional
operation `b?e:f` or in potentially-short-circuited conditions on the
righ-hand side of `&&` and `||` operators.
Following the compilation scheme of clang (observed by looking at the
generated LLVM bitcode), we instrument the program with "marker"
variables, so that for instance `X x = true?X():y;` becomes (following
the execution on the true branch):
```
marker1 = 0; // initialize all markers to 0
PRUNE(true) // entering true branch
X::X(&temporary); // create temporary...
marker1 = 1; // ...triggers setting its marker to 1
X::X(&x, &temporary); // finish expression
if (marker1) {
X::~X(&temporary); // conditionally destroy the temporary
}
```
In this diff, you'll find code for:
- associating markers to temporaries that need them
- code to initialize markers to 0 before full-expressions
- code to conditionally destroy temporaries based on the values of the
markers once the full-expression has finished evaluating
Reviewed By: da319
Differential Revision: D24954070
fbshipit-source-id: cf15df7f7
Summary:
Split the translation of return more aggressively between:
1. the instruction that has to happen before the translation of the sub-expr
2. the sub-expr
3. the instruction that has to happen after the sub-expr
This is needed for the next diff which creates potentially large CFGs in
(2).
Reviewed By: da319
Differential Revision: D24954071
fbshipit-source-id: a7e7e2527
Summary:
This is several inter-connected changes together to keep the tests
happy.
The ConditionalOperator `b?t:e` is translated by first creating a
placeholder variable to temporarily store the result of the evaluation
in each branch, then the real thing we want to assign to reads that
variable. But, there are situations where that changes the semantics of
the expression, namely when the value created is a struct on the stack
(eg, a C++ temporary). This is because in SIL we cannot assign the
*address* of a program variable, only its contents, so by the time we're
out of the conditional operator we cannot set the struct value
correctly anymore: we can only set its content, which we did, but that
results in a "shifted" struct value that is one dereference away from
where it should be.
So a batch of changes concern `conditionalOperator_trans`:
- instead of systematically creating a temporary for the conditional,
use the `trans_state.var_exp_typ` provided from above if available
when translating `ConditionalOperator`
- don't even set anything if that variable was already initialized by
merely translating the branch expression, eg when it's a constructor
- fix long-standing TODO to propagate these initialization facts
accurately for ConditionalOperator (used by `init_expr_trans` to also
figure out if it should insert a store to the variable being
initialised or not)
The rest of the changes adapt some relevant other constructs to deal
with conditionalOperator properly now that it can set the current
variable itself, instead of storing stuff inside a temp variable. This
change was a problem because some constructs, eg a variable declaration,
will insert nodes that set up the variable before calling its
initialization, and now the initialization happens *before* that setup,
in the translation of the inner conditional operator, which naturally
creates nodes above the current one.
- add a generic helper to force a sequential order between two
translation results, forcing node creation if necessary
- use that in `init_expr_trans` and `cxxNewExpr_trans`
- adjust many places where `var_exp_typ` was incorrectly not reset when translating sub-expressions
The sequentiality business creates more nodes when used, and the
conditionalOperator business uses fewer temporary variables, so the
frontend results change quite a bit.
Note that biabduction tests were invaluable in debugging this. There
could be other constructs to adjust similarly to cxxNewExpr that were
not covered by the tests though.
Added tests in pulse that exercises the previous bug.
Reviewed By: da319
Differential Revision: D24796282
fbshipit-source-id: 0790c8d17
Summary:
This diff revises the translation of message expression's arguments in ObjC frontend. In the
frontend, it massages the arguments when calling a static method, so the class or object value is
not given to the static method as the first parameter.
The problem is that it used a raise-exception-and-catch way to detect where we remove the first
parameter. This way of using an exception is not only hard to understand, but also incorrectly
removed the first parameter, with breaking abstract semantics sometimes. (See the added test.) This diff
avoids using the exception.
Reviewed By: jvillard
Differential Revision: D24565513
fbshipit-source-id: 0a84ca394
Summary:
The `make clean` did not remove objects and dot files, so
```
infer/tests/codetoanalyze/objc/frontend$ make test
infer/tests/codetoanalyze/objc/frontend$ make clean
infer/tests/codetoanalyze/objc/frontend$ make test
```
the second `make test` did nothing. This diff adds additional regular
expressions to clean all objects and dot files generated.
Reviewed By: ngorogiannis
Differential Revision: D24566169
fbshipit-source-id: b8c50c922
Summary:
This diff keeps closure parameters in closure-specializated procedures.
What the closure-specialization is doing is a propagation of concrete closures. For example, it
translates:
```
foo(block b) {
b();
}
goo() {
foo(^{...});
}
```
to
```
foo_new() {
(^{...})();
}
goo() {
foo_new();
}
```
However, if `foo` addresses `b` as a normal value like
```
foo(block b) {
block c = b;
}
```
this is translated to
```
foo_new() {
block c = b;
}
```
Note that the closure parameter of `foo` is removed, thus `b` becomes a free variable. Not good.
To avoid the situation, this diff keeps the closure parameters intact.
Reviewed By: da319
Differential Revision: D23905580
fbshipit-source-id: 014989fbf
Summary:
This diff substitutes closure parameter when it is given via variable. For example,
```
x = ^{ ... }
foo(x)
```
this diff substitutes the closure variable `x`,
```
x = ^{ ... }
foo(^{ ... })
```
so that the specialization of `foo` can be done by `CCallSpecializaedWithClosures.process`.
Reviewed By: jvillard
Differential Revision: D23814595
fbshipit-source-id: a89f1530f
Summary:
This diff adds translation of `dictionaryWithObjects:forKeys:count:`. In the previous implementation it was
translated as if it was `dictionaryWtihObjectsAndKeys:`, but their function parameters are different.
In this diff, it translates an array literal `NSDictionary* a = @ [ @"firstName": @"Foo", @"lastName":@"Bar" ];` to
```
n$1=NSString.stringWithUTF8:(@"firstName")
n$2=NSNumber.stringWithUTF8:(@"Foo")
n$3=NSNumber.stringWithUTF8:(@"lastName")
n$4=NSNumber.stringWithUTF8:(@"Bar")
temp1[0]:objc_object*=n$1
temp1[1]:objc_object*=n$3
temp2[0]:objc_object*=n$2
temp2[1]:objc_object*=n$4
n$3=NSDictionary.dictionaryWithObjects:forKeys:count:(temp2:objc_object* const [2*8], temp1:objc_object*const [2*8], 2:int)
```
where `temp` is an additional local variable declared as array.
See,
https://developer.apple.com/documentation/foundation/nsdictionary/1574184-dictionarywithobjects?language=objchttps://developer.apple.com/documentation/foundation/nsdictionary/1574181-dictionarywithobjectsandkeys
{F316854542}
Reviewed By: skcho
Differential Revision: D23447042
fbshipit-source-id: 14b7c3f2b
Summary:
This diff finishes the migration from the specialization of methods that take blocks as arguments. Here we delete all the old code and change the way we model dispatch functions so that the tests pass.
- Remove the code for specializing the methods in biabduction.
- Remove the call flags `cf_with_block_parameters` that was only used in this algorithm.
- Removes models for dispatch functions.
- Adds models for dispatch functions as program transformation only in biabduction. To be added in other checkers in the future.
Reviewed By: ngorogiannis
Differential Revision: D23345342
fbshipit-source-id: b5e8542ce
Summary:
This preanalysis in general aims to create specialized clones of methods that have blocks as arguments and that are called with concrete closures, and then call these clone methods instead of the original ones.
One complication is what happens with the captured variables in the closure. What we do is we add them to the formals of the cloned method and passed them through to the concrete blocks.
We do this transformation in two steps:
1. Go through all the callers of methods with blocks as parameters, and create the clone methods. In this preanalysis we only create the attributes for the new method, not the code. We also update the call instructions in the callers to represent a call to the cloned method with updated arguments: we don't need to pass closures arguments anymore, we instead pass the captured variables as new arguments.
2. We add the corresponding code to the newly created clones: this means swapping the call to the block variable with a call to the corresponding block. Moreover, we add some of the new formals (that correspond to the captured variables) to the arguments of the call.
This diff implements step 1 of the analysis. The next diff D23216021 implements step 2.
Reviewed By: ngorogiannis
Differential Revision: D23109204
fbshipit-source-id: 91a5eb16b
Summary:
This diff translates for-in block in objc as a simple for-loop. For example,
`for (item_type item in items) { body }` is translated to
```
NSEnumerator *enumerator = [items objectEnumerator];
item_type item;
while (item = [enumerator nextObject]) { body }
```
Reviewed By: ezgicicek
Differential Revision: D22841524
fbshipit-source-id: 296ee84df
Summary:
This diff adds translation of `arrayWithObjects:count:`. In the previous implementation it was
translated as if it was `arrayWithObjects:`, but their function parameters are different.
In this diff, it translates an array literal `NSArray* a = @ [ 2, 3 ];` to
```
n$1=NSNumber.numberWithInt:(2:int)
n$2=NSNumber.numberWithInt:(3:int)
temp[0]:objc_object*=n$1
temp[1]:objc_object*=n$2
n$3=NSArray.arrayWithObjects:count:(temp:objc_object* const [2*8],2:int)
a:NSArray*=n$3
```
where `temp` is an additional local variable declared as array.
See,
https://developer.apple.com/documentation/foundation/nsarray/1460145-arraywithobjectshttps://developer.apple.com/documentation/foundation/nsarray/1460096-arraywithobjects?language=objc
Reviewed By: jvillard
Differential Revision: D22631305
fbshipit-source-id: 5be0a55d4
Summary:
We update the type of captured variables to include information about capture mode (`ByReference` or `ByValue`) both for procdesc attributes and the closure expression.
For lambda: closure expression now contains correct capture mode for capture variables. Procdesc still does not contain information about captured variables which we will address in the next diff.
For objc blocks: at the moment all captured variables have mode `ByReference`. Added TODOs to fix this.
Reviewed By: jvillard
Differential Revision: D22572054
fbshipit-source-id: 4c88678ee
Summary: These models for Memory Leaks have been ported to Pulse, so we can remove the models in biabduction and corresponding tests.
Reviewed By: skcho
Differential Revision: D22206287
fbshipit-source-id: e17499ad3
Summary:
Move the implementation of implicit getters and setters from the biabduction to the clang frontend so these methods are accessible to all the checkers.
*Background*: In Objective-C when properties are created in the interface of a class, the compiler creates automatically the instance variable for it and also the getter and setter in the implementation of the class. In the frontend we collect the information about which method is the implicit getter and setter of which instance variable (we get the method declaration but not the implementation), and here we add the implicit implementation.
Reviewed By: skcho
Differential Revision: D22187238
fbshipit-source-id: 76e0508ed
Summary:
This models ARC implementation of dealloc, see https://clang.llvm.org/docs/AutomaticReferenceCounting.html#dealloc. Dealloc methods can be added to ObjC classes to free C memory for example, but the deallocation of the ObjC instance variables of the object is done automatically. So here we add this explicitly to Infer:
1. First, we add an empty dealloc method when it is not written explicitly.
2. For each dealloc method (including the implicitly added ones) we add calls to dealloc of the ObjC instance variables.
Reviewed By: jvillard
Differential Revision: D21883546
fbshipit-source-id: f5d4930f2
Summary:
This function had been computing the name for ObjC methods wrong, with only the class name. This was causing wrong error messages in Pulse.
The main issue was that `Procname.to_simplified_string` was writing `Classname::methodname` for ObjC methods, which is not the convention. This confused the `hashable_name` funtion. So changing the method name to `Classname.methodname` which is more standard, and this also fixes `hashable_name`.
Reviewed By: ngorogiannis, jvillard
Differential Revision: D21570880
fbshipit-source-id: 13ed62cf8
Summary:
Revert incomplete/incorrect translation of `synchronized` in ObjC.
The current translation is incomplete because
```
syncrhonized(foo){
return;
}
```
should be translated as
```
__set_locked_attribute(foo);
__delete_locked_attribute(foo);
return;
__delete_locked_attribute(foo);
```
but instead we get
```
__set_locked_attribute(foo);
return;
__delete_locked_attribute(foo);
```
The same applies for `break`/`continue` etc
Reviewed By: skcho
Differential Revision: D19718882
fbshipit-source-id: fc49ef529
Summary: I noticed when looking into a false positive of strongSelf Not Checked, that there were some inconsistencies in the translation of if statements with an and, with an extra redundant join only if using a method in the condition that returned an object. So I could repro the problem and investigate and found the place of the inconsistency in the translation. This diff fixes it without changing things too much.
Reviewed By: jvillard
Differential Revision: D19518368
fbshipit-source-id: 47a6a778c
Summary:
Sometimes clang9 does not return a boxing method (a name of function to apply), e.g., [@("str")].
To solve the issue, this diff uses "unknownSelector:" instead of giving up the translation.
Reviewed By: dulmarod
Differential Revision: D18831844
fbshipit-source-id: b9324ba39
Summary:
This also prints the CFGs *after* pre-analysis for individual procedures
in infer-out/captured/<filename>/<proc>.dot. One can also look up the
CFGs before pre-analysis in infer-out/captured/proc_cfgs_frontend.dot.
Context: I want to add a pre-analysis that needs to look at proc
attributes inter-procedurally. For this to make sense it has to happen
*after* all of capture, and before analysis.
Thus, this diff brings back the lazy running of the pre-analysis like in
D15803492, except that we still make sure to run the pre-analyses
systematically regardless of the checkers being run by running the
pre-analysis from ondemand.ml. Also we don't need to re-introduce the
"did_preanalysis" proc attribute for the same reason that the
pre-analysis is now run once and for all by ondemand.ml (instead of each
individual checker back in the days).
This has the benefit of running the pre-analysis only when needed, and
the drawback that several concurrent processes analysing the same proc
descs will duplicate work. Since pre-analyses are supposed to be very
fast I assume that neither is a big deal. If they become more expensive
then the benefit gets bigger and the drawback is just the same as with
regular analyses.
Reviewed By: skcho
Differential Revision: D18573920
fbshipit-source-id: de350eaef
Summary:
The synthetic methods from `topl.Property` are now nonempty: they
simulate a nondeterministic automaton.
Reviewed By: jvillard
Differential Revision: D15668471
fbshipit-source-id: 050408283
Summary:
- take advantage more structured attributes in the exported AST
- circumvent new format of `if` and `switch`
- a few new features/nodes but nothing major there
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz, martintrojer
Differential Revision: D15453572
fbshipit-source-id: c0c24345f
Summary:
Instead of emitting an ad-hoc builtin on variable declaration emit a new
metadata instruction. This allows us to remove the code matching on that
ad-hoc builtin that had to be inserted in several checkers.
Inferbo & pulse used that information meaningfully and had to undergo
some minor changes to cope with the new metada instruction.
Reviewed By: ezgicicek
Differential Revision: D14833100
fbshipit-source-id: 9b3009d22
Summary:
This seems generally useful. Force people to do it in the future even if
they want to avoid having to update the frontend tests.
Reviewed By: mbouaziz
Differential Revision: D14324758
fbshipit-source-id: cdef3f72a
Summary:
Before, the liveness pre-analysis would place extra instructions in the
CFG for either:
1. marking an `Ident.t` as dead, or
2. marking a `Pvar.t` as `= 0`
But we have no way of marking pvars dead without setting them to 0. This
is bad because setting pvars to 0 is not possible everywhere they are
dead. Indeed, we only do it when we haven't seen their address being
taken anyway. This prevents the following situation, recorded in our tests:
```
int address_taken() {
int** x;
int* y;
int i = 7;
y = &i;
x = &y;
// if we don't reason about taken addresses while adding nullify instructions,
// we'll add
// `nullify(y)` here and report a false NPE on the next line
return **x;
}
```
So we want to mark pvars as dead without nullifying them. This diff
extends the `Remove_temps` SIL instruction to accept pvars as well, and
so renames it to `ExitScope`.
Reviewed By: da319
Differential Revision: D13102953
fbshipit-source-id: aa7f03a52
Summary:
It enables the translation of casting expression. As of now, it
translates only the castings of pointers to integer types, in order to
avoid too much of change, which may mess the checkers up.
Reviewed By: jvillard
Differential Revision: D12920568
fbshipit-source-id: a5489df24
Summary:
Useful to understand the changes in the pre-analysis, or to inspect the
CFG that checkers actually get.
This means that the pre-analysis always runs when we output the dotty,
but I don't really see a reason why not. In fact, we could probably
*always* store the CFGs as pre-analysed.
Reviewed By: mbouaziz
Differential Revision: D13102952
fbshipit-source-id: 89f3102ec
Summary:
When initialising a variable via semi-exotic means, the frontend loses
the information that the variable was initialised. For instance, it
translates:
```
struct Foo { int i; };
...
Foo s = {42};
```
as:
```
s.i := 42
```
This can be confusing for backends that need to know that `s` actually
got initialised, eg pulse.
The solution implemented here is to insert of dummy call to
`__variable_initiazition`:
```
__variable_initialization(&s);
s.i := 42;
```
Then checkers can recognise that this builtin function does what its
name says.
Reviewed By: mbouaziz
Differential Revision: D12887122
fbshipit-source-id: 6e7214438
Sungkeun Cho
Daiva Naudziuniene
Radu Grigore
Ezgi Çiçek
Xiaoyu Liu
Jules Villard
Qianyi Shu
Dulma Churchill
Nikos Gorogiannis
Josh Berdine
David Lively
Dino Distefano
Mehdi Bouaziz