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:
Namely `enumerateObjectsUsingBlock` which takes a collection and a block as an arg and the iterates over the collection and applies the block to each element of the collection.
This is a common way to iterate over Objc collections, so let's add a model for it.
Reviewed By: skcho
Differential Revision: D24604590
fbshipit-source-id: 1ceeb4b40
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 fixes on-demand symbolic value generation of a class that inherits NSEnumerator.
Reviewed By: ngorogiannis
Differential Revision: D24504955
fbshipit-source-id: bcb20e8aa
Summary:
This diff replaces overridden method calls in ObjC when possible, ie the first parameter of the
method has a sub-class type of the method's class. For example,
when `MyEnumerator` is a sub-class of `NSEnumerator` and there is overridden `nextObject`,
```
[my_enumerator nextObject]
```
in Sil, it was translated to like
```
NSEnumerator.nextObject(my_enumerator : MyEnumerator*)
```
and the analyzer missed the overridden method. This diff replaces the function call to
```
MyEnumerator.nextObject(my_enumerator : MyEnumerator*)
```
Reviewed By: ezgicicek
Differential Revision: D24477290
fbshipit-source-id: 6842a76f8
Summary:
This diff revises `nextObject` model to handle multiple symbolic enumerators. Instead joining the
symbolic offsets of them, which sometimes introduces top, it sums the offsets. This is a sound &
conservative semantics since they are all non-negative integers.
Reviewed By: ezgicicek
Differential Revision: D24474513
fbshipit-source-id: 6707aa907
Summary:
This diff revises memory model of enumerator in ObjC to enable passing it as a parameter.
The cost checker was not able to analyze a function precisely when it gets an enumerator as a
parameter because the offset of an enumerator was available only when the analyzer knew the correct
relation between the enumerator and an array.
This diff simplifies the enumerator to have a similar value with `array->elements`, so its offset can
be taken without the relation between enumerator and array to get them.
Reviewed By: ezgicicek
Differential Revision: D24446574
fbshipit-source-id: 27cdc051e
Summary:
This diff adds an option hiding function pointers in costs to users: `cost-suppress-func-ptr` is
true by default.
Reviewed By: ezgicicek
Differential Revision: D24448212
fbshipit-source-id: 88f6b5ea1
Summary:
This diff adds a model for NSFileManager.contentsOfDirectoryAtURL as returning a constant-length
collection.
The analyzer cannot know files in a directory. We have some options to handle such unknown data.
1. Use `Unknown` value, ie `top`
2. Use a symbolic value
3. Use a constant value
We had been used the first option. An upside of this is that the analyzer can remain as sound.
However, a downside of this is the top value can be propagated to other procedures, making their
costs top, thus we may miss some cost changes of them.
The second option is to introduce a symbolic value, ie. that for the number of files. A problem is
that the symbolic value will never be concretized. As a result, the symbol can be propagated to
other procedures, increasing the coefficient of the complexity or making top costs. Note that handling multiple
symbols is somewhat limited in Inferbo's interval domain.
The last option is to introduce a constant value. I think this is the best approach we can take among above.
Even though we may have FNs when there are a lot of files in a directory, we cannot reason or expect about
that at the analysis time anyway.
Reviewed By: ezgicicek
Differential Revision: D24418099
fbshipit-source-id: bf8cf3538
Summary: This diff adds closure symbols to operation/allocation costs, when function pointer is called.
Reviewed By: ezgicicek
Differential Revision: D24308550
fbshipit-source-id: 6c5889d41
Summary:
This diff extended the polynomial domain to include symbols for closure calls.
When the closure symbol is added to the polynomial? Unknown closure is called inside a function
like,
```
foo() {
self->closure_field();
}
```
Thus, the cost of `foo` becomes `|self->flosure_field|`, rather than unknown. (Note that this
semantics is added only for autoreleasepool size at the moment.)
When the symbol is instantiated? `foo` is called with correct closure contexts.
```
goo() {
self->closure_field = ^(){ ... };
foo();
}
```
The summary of `goo` will have instantiated summary of the closure.
Reviewed By: ezgicicek
Differential Revision: D23992590
fbshipit-source-id: d1d228403
Summary:
In ObjC, `NSObject.copy` returns the object returned by `copyWithZone:` on the given class. This method must be implemented if the class complies with `NSCopying` protocol. Since we don't have access to `NSObject`'s code, to follow calls into `copyWithZone:`, we replace such `copy` calls with calls to `copyWithZone:` when a) such a method exists in the class and b) the class conforms to `NSCopying` protocol.
This is done in the preanalysis because
- we need to know if there is a `copyWithZone:` method in the class.
- so that other analyses also benefit (as opposed to doing this in cost and inferbo models).
Note that `NSObject` doesn't itself conform to `NSCopying` but all its subclasses must confrom to the protocol and support the same behavior as above.
https://developer.apple.com/documentation/objectivec/nsobject/1418807-copy
Similarly for `mutableCopy` -> `mutableCopyWithZone:` for classes implementing `NSMutableCopying` protocol.
Reviewed By: skcho
Differential Revision: D24218102
fbshipit-source-id: 42900760e
Summary: Model it similar to `NSArray.initWithArray` as copying from the given dictionary elements. Removes a FP as expected.
Reviewed By: ngorogiannis
Differential Revision: D24136868
fbshipit-source-id: ed31c3c8f
Summary:
This diff extends inferbo's domain to include closure values. The goal of this extension is to
follow missing semantics where closures are handled as values, for example, a closure is assigned to
an object field, then it is got later to call.
Due to the bottom-up nature of the analyzer, sometimes we don't know which values are written in a
field, which is the same for the other non-closure values.
Reviewed By: ezgicicek
Differential Revision: D23932186
fbshipit-source-id: 4a575d0de
Summary: Dispatch & function call mechanism was so jumbled up together. Let's refactor it to be cleaner.
Reviewed By: skcho
Differential Revision: D24049889
fbshipit-source-id: 42a218016
Summary:
The problem: current enumerator semantics does not work on symbolic enumerator that is given as a
parameter.
Reviewed By: ezgicicek
Differential Revision: D24017059
fbshipit-source-id: 378e75bb0
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: Without curly braces, it is declaration of globals, not fields.
Reviewed By: ezgicicek
Differential Revision: D23866604
fbshipit-source-id: dd685c8d6
Summary: This diff adds a test, in which autoreleasing lambda is written in an object field, then called later. The test is not analyzed precisely because it cannot follow that a lambda value is written to a field of an object as of now.
Reviewed By: ezgicicek
Differential Revision: D23843089
fbshipit-source-id: 62ba153aa
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:
Upgrade to latest clang release, needed for xcode12.
clang-8/9 won't be able to read the Xcode 12 SDK since there's annotations that will fail compilation.
Also removing unused (and hard to compile) binary `ast_exporter_bin` from facebook-clang-plugins/libtooling.
Reviewed By: ngorogiannis
Differential Revision: D23780089
fbshipit-source-id: 2314125a9
Summary:
This diff add a model of `NSArray.index_of_object_passing_test` in
autoreleasepool size model.
Worst case autorelease cost is "array size x cost of given test function (lambda)".
Reviewed By: ezgicicek
Differential Revision: D23785165
fbshipit-source-id: 95ec9700a
Summary: ObjC objects can be added to autorelease pool by `CFAutorelease`.
Reviewed By: ezgicicek
Differential Revision: D23625632
fbshipit-source-id: 694e5bffb
Summary:
This diff regards some methods of NSKeyedUnarchiver as autorelease, because they may call
autorelease.
Reviewed By: ezgicicek
Differential Revision: D23600232
fbshipit-source-id: a9fc1cc56
Summary:
This diff increases autoreleasepool size when
* caller is non-ARC-compiled
* callee is ARC-compiled
* return type is a pointer to objc object
To distinguish non-ARC-/ARC-compiled:
* extended `translation_unit_decl_info` to have a boolean field `is_objc_arc_on`
* then copied it to `ProcAttributes.t` for each procedures.
Reviewed By: ezgicicek
Differential Revision: D23565003
fbshipit-source-id: dee22ea82
Summary: This diff selects an autorelease trace that has a bigger polynomial.
Reviewed By: ezgicicek
Differential Revision: D23731155
fbshipit-source-id: 243591583
Summary:
This diff reports paths under the xcode isysroot as relative in tests.
This was a problem when another machine that has a different isysroot
directory is running the test.
Reviewed By: ezgicicek
Differential Revision: D23729222
fbshipit-source-id: 4e9681f65
Summary: This diff adds trace field for autoreleasepool size. Unlike to the other checkers, eg inferbo and operation cost, the autoreleasepool size checker should have traces for constants.
Reviewed By: ezgicicek
Differential Revision: D23678084
fbshipit-source-id: 35e6cf5f5
Summary:
The `attr_kind` column has now exactly two possible values: defined and undefined (since D22187238 (61ae2d1e1b)). This is also what should be stored in the field `is_defined` in the procedure attributes. Finally, the cfg can be `NULL` or not. This diff makes all three of these agree, in order to allow for the removal of the `attr_kind` column up the stack, since it will be equivalent to `cfg is NOT NULL`.
The changes in the tests indicate improved correctness: previously, specialising a modelled callee would result in a dummy entry in the procedures table, because all clang procedures were given a procdesc, so the specialisation would specialise the empty procdesc. Now, the model is not shadowed by the specialised dummy procdesc, and is specialised itself (and that's why it also ends up in the capture DB, which is slightly counter-intuitive).
Reviewed By: jvillard
Differential Revision: D23536931
fbshipit-source-id: 983216cb6
Summary:
We defined cost's plus as "zero+unreachable = unreachable" for the operation cost. The meaning of
the zero cost is that no statment is analyzed yet, and the unreachable(bottom) cost means a program
point is analyzed as unreachable. We thought "zero+unreachable" happens in very specific cases we
need to check, or due to an analyzer bug. For debugging purpose, we defined it to return more
specific unreachable cost.
However, in allocation/autoreleasepool costs, the zero cost is not very special value. If there is
no alloc/autorelease calls, they can have the zero cost. "zero+unreachable = unreachable" doesn't
make sense there.
This diff changes the plus as "zero+unreachable = zero" for the non-operation costs.
Reviewed By: ezgicicek
Differential Revision: D23578869
fbshipit-source-id: 5392eca1c
Summary: This diff adds some tests run with ARC/non-ARC compiled objective-c sources.
Reviewed By: ezgicicek
Differential Revision: D23542135
fbshipit-source-id: 8e089666b
Summary:
This diff adds a new experimental checker for detecting size of objects in autorelease pool in ObjC. The basic mechanism is almost the same with the previous cost calculation:
* Autorelease pool size is increased at explicit `autorelease` call
* Autorelease pool size is set as zero by the `autoreleasepool` block.
While it only supports the explicit calls as of now, we will extend the checker to handle more cases in the following diffs.
Reviewed By: ezgicicek
Differential Revision: D23473145
fbshipit-source-id: 416488176
Jules Villard
Sungkeun Cho
Ezgi Çiçek
Martin Trojer
Nikos Gorogiannis