Summary:
Following the previous diff, the reason we mistakingly introduced new block as complexity increases is because when we are computing the hash of block, we return the string `block` to compute the hash for all block.
This diff fixes this case by returning the non-verbose name of the block when returning hashable name.
Reviewed By: ezgicicek
Differential Revision: D23729710
fbshipit-source-id: a345d3045
Summary:
When inspecting silent introduced issues, we found that an introduced issue is about a complexity increase of a block that is only created in the current diff. Based on the trace view, we find out that this is caused by infer mistakingly consider another block that exists in the previous diff as the same block that is newly created in the current diff.
This diff adds a test case that reproduces this case, and this will be fixed in the next diff.
facebook
Trace view: https://www.internalfb.com/intern/traceview/?id=109896337
Reviewed By: ezgicicek
Differential Revision: D23681550
fbshipit-source-id: 78341268b
Summary:
If the issue is related to the use of an unvetted third party method
that can not be trusted, we record this fact.
Reviewed By: ngorogiannis
Differential Revision: D23705626
fbshipit-source-id: 851328fe5
Summary:
It was inconsistent before (we recorded it only for meta issues).
Now we always record it, which will simplify further diffs.
In the next diffs, we are going to add more fields inside `nullsafe_extra`.
Reviewed By: ngorogiannis
Differential Revision: D23705592
fbshipit-source-id: 8bbb0e7c8
Summary:
This can be useful to make pulse forget about tricky parts of the code.
Treat "skipped" procedures as unknown so heuristics for mutating the
return value and parameters passed by reference are applied.
Reviewed By: ezgicicek
Differential Revision: D23729410
fbshipit-source-id: d7a4924a8
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:
Handle the case of
- computing the range of `[lb, ub]` where `lb` contains length path and `ub` is constant. E.g. `[a.length, 2]` would give `3` range
- simplifying `c1 +/- min(c2, a.length)` to `c1 +/- min(c2, 0)`
since the length of a collection/array is always nonnegative.
This also removes some existing FPs in tests results.
Facebook
Context: we stumbled upon this issue in ObjC results TV109717121 which is corresponding to the added test case.
Reviewed By: skcho
Differential Revision: D23648807
fbshipit-source-id: 1e89ea246
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:
For complexity issues from O(m) to O(n), we only include the trace of the current complexity O(n). However, this makes it difficult to understand what the original complexity O(m) was. Especially in fixed issues where n=1, we only get a constant cost with no trace attached, so it is difficult to see how the symbol m disappeared.
This diff includes the traces for the previous cost in the cost issues.
Reviewed By: skcho
Differential Revision: D23680360
fbshipit-source-id: 3f2b21b20
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: Polynomial category zero corresponds to unreachable but "zero" is a misnomer and rather confusing. Let's fix it.
Reviewed By: skcho
Differential Revision: D23597735
fbshipit-source-id: f0c96ed26
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
Summary:
This diff extends the cost_item json format to print the autoreleasepool_size field. Not yet, there
is no semantics for that code kind, so the results will always be zero with no traces.
Reviewed By: ezgicicek
Differential Revision: D23540665
fbshipit-source-id: 94442e376
Summary: There is still a bug in using marshalled values as keys (exposed up the diff stack) where structurally equal procnames get serialised to distinct keys. This diff addresses that bug by using the existing functionality for string unique IDs.
Reviewed By: ezgicicek
Differential Revision: D23133761
fbshipit-source-id: 3cbafb51b
Summary:
As title.
Facebook
Found this case by examining db.
>
D23394545
Time complexity of `_registerTabPreloadables` has **decreased** from `Top` to `O(1)`. Please make sure this is an expected change. You can inspect the trace to understand the complexity increase:
Reviewed By: ezgicicek
Differential Revision: D23448099
fbshipit-source-id: 108fd1ef2
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: Added a model for copy constructor for `std::function`. In most cases, the SIL instruction `std::function::function(&dest, &src)` gives us pointers to `dest` and `src`, hence, we model the copy constructor as a shallow copy. However, in some cases, e.g. `std::function f = lambda_literal`, SIL instruction contains the closure itself `std::function::function(&dest, (operator(), captured_vars)`, hence, we need to make sure we copy the right value.
Reviewed By: ezgicicek
Differential Revision: D23396568
fbshipit-source-id: 0acb8f6bc
Summary: We only added suffix-stripped prefixes of props but this causes FPS if a component declares props that actually contain suffixes since we strip them when we are adding builder calls. The fix is to add both normal and stripped versions.
Reviewed By: skcho, Katalune
Differential Revision: D23375405
fbshipit-source-id: e99cb4dd8
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: There was a mismatch between formals and actuals in `std::function::operator()` because we were not passing the first argument corresponding to the closure.
Reviewed By: ezgicicek
Differential Revision: D23372104
fbshipit-source-id: d0f9b27d6
Summary: When we evaluate lambdas in pulse, we create a closure object with `fake` fields to store captured variables. However, during the function call we were not linking the captured values from the closure object. We address this missing part here.
Reviewed By: jvillard
Differential Revision: D23316750
fbshipit-source-id: 14751aa58
Summary:
Report errors found by running Topl on top of Pulse, when using
--topl-pulse. Topl tests now run on top of Pulse.
Reviewed By: jvillard
Differential Revision: D23030771
fbshipit-source-id: 8770c2902
Summary:
Two issues are fixed
1. For this diff, when the condition `curr_langauge_is Java` is removed in some part of the analysis, some c bufferoverrun tests are broken. This is fixed in this diff by inspecting if we are dealing with a `Size` alias referring to an `objc_internal_collection_array`.
2. Previously, when we are modifying mutable array in a loop, e.g. adding element to the array or removing element from the array, we are unable to give an estimable size of the array after exiting the loop. This is now fixed, and the corresponding FPs are resolved.
Reviewed By: skcho
Differential Revision: D23342350
fbshipit-source-id: 200f5261c
Summary:
The generated code used to contain Prune statements that had boolean
connectives in their conditions. After this commit, all conditions
should have no boolean connective (LNot, LOr, LAnd) at top-level; that
is, prune conditions should be atomic.
The main motivation behind this change is that (a) frontends follow this
convention, and (b) Pulse assumes this convention.
Reviewed By: jvillard
Differential Revision: D23022273
fbshipit-source-id: 1313328e4
Summary:
In the previous diffs, we implement enumerator in order to estimate the cost of for-each loop in ObjC, but when we have FP case when enumerator is used not in for-each loop. For example, the following code has top cost before the fix.
```
void nsarray_enumerator_linear_FP(NSArray* array) {
id obj;
NSInteger sum = 0;
NSEnumerator* enumerator = [array objectEnumerator];
while (obj = [enumerator nextObject]) {
sum += (NSInteger)obj;
}
}
```
Reviewed By: skcho
Differential Revision: D23294895
fbshipit-source-id: 50c7b359f
Summary:
`delete` works exactly like `free` so merge both models together. Also
move the `free(0)` test to nullptr.cpp as it seems more appropriate.
Reviewed By: da319
Differential Revision: D23241297
fbshipit-source-id: 20a32ac54
Summary:
Fix the FP when iterating through constant collection.
facebook
This fix is a hack for now.
Reviewed By: ezgicicek, skcho
Differential Revision: D23241338
fbshipit-source-id: e2e0c05f8
Summary:
As title.
This diff is co-authored by SungKeun Cho and me.
facebook
This diff is co-authored by skcho and me.
Original comments from skcho
"For the record:
1. Rory and I tried to write models for ObjC iterator.
2. We could not use Java's iterator semantics: In Java's, `hasNext` returns the size of collection, rather than a boolean, and which is used as a control variable. On the other hand, in ObjC, it calls only `nextObject`, not calling `hasNext`, and the return value of which is being checked as `null`.
3. We added an artificial field `objc_iterator_offset` to keep the index of the iterator, and the models added in this diff are handling that integer value.
A problem is that `array.objc_iterator_offset` is not included in the control variables, since the condition of the loop is `nextObject() != null` that does not include the iterator offset. We need to make `array.objc_iterator_offset` as a control variable, by changing the part collecting control variables.
"
Reviewed By: ezgicicek, skcho
Differential Revision: D22944278
fbshipit-source-id: 7e71b79c1
Summary: In the frontend captured variables for blocks are added as formal parameters in procdesc at the beginning.
Reviewed By: dulmarod
Differential Revision: D23163619
fbshipit-source-id: 2bcbe9b9c
Summary:
When implementing iterator, we find out that because some semantics of inferbo is Java-specific, we cannot simply use Java's `Collection` model for `NSCollection`.
So this diff writing `NSCollection` model separately.
This diff also extracts the common parts of `NSCollection` and `Collection` into `AbstractCollection` to combine the duplicate the parts.
Reviewed By: ezgicicek
Differential Revision: D22975159
fbshipit-source-id: daed3f99f
Qianyi Shu
Mitya Lyubarskiy
Jules Villard
Sungkeun Cho
Ezgi Çiçek
Nikos Gorogiannis
Daiva Naudziuniene
Dulma Churchill
Radu Grigore