Summary:
A minor but persistent annoyance: named argument of a non-ambiguous type
of almost the same name. No.
Reviewed By: skcho
Differential Revision: D24991673
fbshipit-source-id: c806f9cec
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:
The translation of `switch` cases needs to insert nodes around the
translation of each `case` sub-statement, so we need to force node
creation in these sub-statements so the nodes around it can be connected
to the translation of the sub-statements.
Also added more logging I found useful when debugging that.
Reviewed By: da319
Differential Revision: D24991455
fbshipit-source-id: d3a622142
Summary:
The current source parser is based on ocamllex only.
In order to track field declaration locations, we propose a
new parser using ocamllex/menhir. This is a more ambitious
project that closely follows the official Java syntax.
Reviewed By: jvillard
Differential Revision: D24858280
fbshipit-source-id: 22d6766e5
Summary:
In all other places, we index params from 0, but accidentally recorded
the wrong number in json. It was because of the confusion between index
and user-visible param position that we show for the user message.
This diff fixes it: now we use 0-based indices internally (but of course still
report 1-based ones in the error message).
Reviewed By: artempyanykh
Differential Revision: D24916878
fbshipit-source-id: 45532c5ff
Summary:
Sometimes there are annotations that don't correspond to the user facing
code.
Previously we would fail, now process them gracefully.
Reviewed By: artempyanykh
Differential Revision: D24890895
fbshipit-source-id: e64a866ec
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: Model `folly::Optional::get_pointer` which returns an address to a value if exists or `nullptr` if empty.
Reviewed By: jvillard
Differential Revision: D24935677
fbshipit-source-id: 9d990fe07
Summary:
We deliberately stopped as soon as an error was detected when applying a
function call. This is not good as other pre/posts of the function may
apply cleanly, which would allow us to cover more behaviours of the
code.
Went on a bit of a refactoring tangeant while fixing this, to clarify
the `Ok None`/`Ok Some _`/`Error _` datatype returned by PulseInterproc.
Now we report errors as soon as we find them during function calls but
continue accumulating specs afterwards.
Reviewed By: da319
Differential Revision: D24888768
fbshipit-source-id: d5f2c29d7
Summary:
In the next diff we need to know when a destructor is needed for sure
before calling some of these auxiliary methods.
Reviewed By: da319
Differential Revision: D24832078
fbshipit-source-id: 6d4e17de2
Summary:
Communicate new facts from the arithmetic domain to the memory domain to
detect contradictions between the two.
Reviewed By: jberdine
Differential Revision: D24832079
fbshipit-source-id: 2caf8e9af
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: In cpp, lambda's operator() name includes line and column numbers which were not ignore in proc name when computing bug hash.
Reviewed By: ngorogiannis
Differential Revision: D24890545
fbshipit-source-id: 95e6735f3
Summary:
As per title, plus de-quadratic-ify substitution of actuals into formals.
Also, fix a bug in treatment of callee summaries where the caller lock state was updated first and then used to process accesses in the callee (so should only take into account the original caller state).
Reviewed By: jvillard
Differential Revision: D24857922
fbshipit-source-id: 07ce6999c
Summary:
Otherwise this gets serialized in an unconventional way; `nullable` serializes it as a
standard `null` or string value.
Reviewed By: artempyanykh
Differential Revision: D24827593
fbshipit-source-id: a5a9afc80
Summary: As per title, plus minor improvements in interfaces and a couple of FIXMEs.
Reviewed By: skcho
Differential Revision: D24836125
fbshipit-source-id: f7a4dc196
Summary:
The starvation domain keeps a domain element per distinct pair of lock object and source location. This was used to counteract the imprecision of implicit Quandary-style traces. Starvation has used explicit traces for a long time now, so keeping all these elements is expensive (in fact, in some cases exponential) and of no value. Now, lock object identity is the only distinguishing feature of a domain element.
Also, fix some pretty printing for debugging purposes.
Reviewed By: jvillard
Differential Revision: D24829306
fbshipit-source-id: 22e12f9c1
Summary:
This diff fixes `degree_with_term` to ignore function pointer symbols. `degree_with_term` does
* calculate the degree
* simplify the polynomial only for printing them to users
thus, there is no problem to ignore the function pointer symbols always, ie which does not affect semantics or summary values.
Reviewed By: ezgicicek
Differential Revision: D24596479
fbshipit-source-id: 1e29d2de0
Summary: We recently introduced a more precise model for constructing an optional from a value by making a shallow copy. However, this introduced Use After Delete false positives. For now, we go back to a less precise model by creating a fresh value. A proper model would be to either make a deep copy or call the copy constructor for a value. We will address this in the following diff.
Reviewed By: jvillard
Differential Revision: D24826749
fbshipit-source-id: 3e5e4edeb
Summary: Refactor `folly::Optional` models to make them easier to reuse for `std::optional`
Reviewed By: jvillard
Differential Revision: D24760053
fbshipit-source-id: f665e84c8
Summary:
- log trans_state for each instruction
- create boxes to indent logs
- hunt down "@." that would prematurely close the boxes
- improve messages
Reviewed By: ngorogiannis
Differential Revision: D24794798
fbshipit-source-id: 80d51a8c5
Summary:
They are indirectly encoded in "procedure" field already, but:
1/ To extract name and params one needs to parse the procedure on the
client side
2/ We already have class, package, and field: method and its params is a
consistent change
Reviewed By: artempyanykh
Differential Revision: D24731064
fbshipit-source-id: fae478e7e
Summary:
If the issue one of:
- Field Not Nullable
- Field Not Initialized
- Field Overannotated,
we record field_name to .json result.
NoTE: Design choice for representation. For Field Not Initialized and Field Overannotated
this is always internal (relative to the class) field, but for Field Not
Nullable it can be either internal or external. We could have a
structured output, or always output full name. I preferred to output
short name for convenience of the main usacase I am anticipating.
NOTE: not to be confused with the case where the field is nullable but
we e.g. try to dereference it. This is indirectly related to the issue
(can be several such fields for starters) and if we one day output it,
it will be provided in a separate way (similarly to how we output
nullable_methods).
Reviewed By: artempyanykh
Differential Revision: D24730320
fbshipit-source-id: c995ec221
Summary:
We never tested params dependent on things (tested only things dependend
on params).
Reviewed By: artempyanykh
Differential Revision: D24726858
fbshipit-source-id: a0861cfc3
Summary:
Since we report issues types without a prefix (e.g. ERADICATE_) and
with spaces we should also allow for prefix-less issue types in
SuppressLint, so both should work
- SuppressLint("eradicate-field-not-initialized")
- SuppressLint("Field Not Initialized")
Reviewed By: jvillard
Differential Revision: D24760341
fbshipit-source-id: 1590cf6d0
Summary:
There is a feature in Nullsafe that is interfering with "annotation
graph" feature. Because of this we would not detect provisional
violations for misuses of params of equals() (They will be recorded
as user facing rather than provisional issues).
This diff turns this feature off for annotation graph mode.
Reviewed By: artempyanykh
Differential Revision: D24726655
fbshipit-source-id: 4b7577667
Summary:
Virtual "this" invisible param exists in the annotated signature, but
does not exist in some other places, which causes a lot of annoyance in
different places.
This diff does not intend to solve all this, but makes one step forward.
1/ AnnotatedNullability now explicitly distincts normal params and
VirtualThis.
2/ AnnotatedSignature accounts for this via: a) not having redundant
fake annotation points b) having corrent param indices (those were off
by 1 in non-virtul methods).
Reviewed By: artempyanykh
Differential Revision: D24726480
fbshipit-source-id: fdb8bb0fb
Summary: This is a complex enough feature so iterating on it in a safe manner will be useful.
Reviewed By: artempyanykh
Differential Revision: D24725406
fbshipit-source-id: 81b247143
Summary: `folly::Optional::value()` returns a reference, hence an error was shown when the actual value was being accessed. Since `value()` throws an exception in case of `folly::none`, we want to show the error message at the call site of `value()`. We do this by dereferencing the result of `value()` in the model.
Reviewed By: jvillard
Differential Revision: D24702875
fbshipit-source-id: ca9f30349
Summary:
This is needed for `infer reportdiff` to report issues that are in files
in the same workspace but were captured under a different project root,
but it's also legit to do so in general as "under project root" really
means "this file belongs to the project under analysis".
Reviewed By: martintrojer
Differential Revision: D24755592
fbshipit-source-id: fa8fab127
Summary: In cpp, lambda's `operator()` name includes line and column numbers which were not ignore when computing bug hash.
Reviewed By: jberdine
Differential Revision: D24649125
fbshipit-source-id: 7a235fd3e
Summary:
The problem in Reporting.ml:log_issue_from_summary is that it merely
checks the presence of `SuppressLint` annotation on method's body to
decide whether to log or not the issue. This means that regardless of
issue types specified in `SuppressLint`, all issues on such method will
get blocked.
Here we fix that.
Reviewed By: ngorogiannis, mityal
Differential Revision: D24726604
fbshipit-source-id: c9cae3833
Summary:
This diff glues the previous work together.
The ClassLevelAnalysis finds list of provisional violation, builds the
graph based on them, and outputs this graph as a separate issue.
Reviewed By: artempyanykh
Differential Revision: D24682802
fbshipit-source-id: 8174da91a
Summary:
This change is what makes the annotation graph a _graph_.
Now we can detect places when adding one annotation causes an issue that can be
fixed by adding another annotation.
Reviewed By: artempyanykh
Differential Revision: D24650979
fbshipit-source-id: b8452b822
Summary:
In the previous diff we introduced
AnnotatedNullability.is_annotatable_as_nullable method.
Lets now use it in all places where we issue ProvisionallyNullable
annotation.
This will lead to more precise nullability graph (without attempts to
annotate primitives).
Reviewed By: artempyanykh
Differential Revision: D24650951
fbshipit-source-id: 8ea0bb97d
Summary:
This change is on par with the logic that we already have for methods
and method params.
In this logic, we explicitly distinct class under analysis & the
external class when fetching AnnotatedNullability (we needed this to support
various Nullsafe modes as well).
Here we use the same approach for fields.
NOTE: this is not intended to work with nested classes just yet.
Reviewed By: artempyanykh
Differential Revision: D24650934
fbshipit-source-id: f555bcc3f
Summary:
:
New flag (equivalent to --clang-biniou-file) to pass in AST to
frontend.
Passing in json files has the big advantage of emitting line numbers
on frontend AST errors.
Reviewed By: jvillard
Differential Revision: D23814358
fbshipit-source-id: 0ad0452ff
Summary: Cleanup `Typ` by moving all constant types to `StdTyp`. Also remove `Typ.typ` as it's just `Typ.t` now.
Reviewed By: jberdine
Differential Revision: D24620397
fbshipit-source-id: 4764f87ef
Summary:
Now we have all pieces of information needed to build the annotation
graph (to be finished as a follow up).
This diff utilizes the same approach that is done when we calculate node
promotions:
1/ AssignmentRule and DereferenceRule know that some issues are related
to provisional annotations and expose this information.
2/ ClassLevelAnalysis collects all the issues, extract violations from
them, and outputs them.
Next step will be changing the output to the meta-issue printing the
annotation graph in json.
Reviewed By: artempyanykh
Differential Revision: D24621410
fbshipit-source-id: c240da3aa
Summary:
In the previous diffs, we introduced
AnnotatedNullability.ProvisionallyNullable. This is a symmetric change
introducing the same for Nullability.t
Note how ProvisionallyNullable is made `is_nonnullish`. This means that
if we run nullsafe in --nullsafe-annotation-graph mode, the following
should start happening:
1/ A lot of new issues will be recorded - every time a local method or
non-null param is dereferenced etc.
2/ But all of those new issues will be filtered by corresponding Rules
(e.g. AssignmentRule). So those issues will not manifest to user-visible
errors. However, now we are capable to see and analyse those hidden
issues in ClassLevelAnalysis. This is exactly what we will be doing in
follow up diffs.
Note that the place of ProvisionallyNullable in the hierarchy is
somewhat arbitraily. It is important to make it less pri then Nullable -
but when it comes to comparing with other "nonnullish" modes - we can
tweak the behavior later on.
Reviewed By: artempyanykh
Differential Revision: D24621411
fbshipit-source-id: 3b99e5e55
Summary:
The fact that we did not preserve all joins (and used only the first
one) was a shorcut since the origin is used only for reporting, and
reporting the info about left joinee is enough for practical purposes.
Hovewer, to support annotation graph, we need all joinees.
This diff is a no-op since we use only the first one currently.
Reviewed By: artempyanykh
Differential Revision: D24621412
fbshipit-source-id: 8fe1174e7
Summary:
This was perhaps needed at some point, but now there is no need in this.
See the next diff that refactors InferredNullability so we store all
joinee results there.
Reviewed By: artempyanykh
Differential Revision: D24621413
fbshipit-source-id: d6f406b87
Summary:
This is the basic building blocks for the future annotation graph. This
diff introduces the first abstraction to be iterated on.
Reviewed By: artempyanykh
Differential Revision: D24621414
fbshipit-source-id: 19acdf216
Summary:
Before we were creating a fresh internal value when we were constructing `folly::Optional`. This diff models `folly::Optional` constructor more precisely by copying the given value.
There was also a missing dereference in the model of `value_or`
Reviewed By: jvillard
Differential Revision: D24621016
fbshipit-source-id: c86d3c157
Summary:
The global type environment is loaded in all analysis workers, so it is best to minimise its memory footprint. `MaxSharing` does a great job at this but
- it is very slow
- it may be involved in observed segfaults, since it uses potentially unsafe operations
This diff replaces `MaxSharing` with a post-facto poor-man's hashcons pass through the parts involved in a java type environment. It is not as efficient as `MaxSharing` at reducing footprint, but it's much faster.
Reviewed By: jberdine
Differential Revision: D24590400
fbshipit-source-id: c37100325
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:
The autorelease pool size checker increases the size when an non-arc-compiled function calls
arc-compiled function. However, if the callee is declared in arc-compiled code and its body is not
actually captured or compiled, we should not increase the autorelease pool size.
Reviewed By: ezgicicek
Differential Revision: D24569661
fbshipit-source-id: 2fd707096
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: Model `folly::Optional::value_or(default)` to return value if not-empty and `default` if empty.
Reviewed By: jvillard
Differential Revision: D24539456
fbshipit-source-id: cc9e176cc
Summary: Can be useful, especially to dump all the summaries as json.
Reviewed By: skcho
Differential Revision: D24504253
fbshipit-source-id: 845e7d657
Summary:
Emit the crucial parts of Pulse summaries as json to enable
post-processing by external tools. Stop somewhat arbitrarily at some
datatypes that are just emitted as "opaque" values.
For example:
```
$ infer debug --procedures --procedures-summary-json --select 0
[[["pulse",[["ContinueProgram",{"post":{"heap":[["v3",[[["Dereference"],["v4","_"]]]],["v7",[[["Dereference"],["v3","_"]]]]],"stack":[[["ProgramVar",{"plain":"return","mangled":null}],["v7","_"]]],"attrs":"_"},"pre":{"heap":[],"stack":[],"attrs":"_"},"skipped_calls":"_","path_condition":"_"}],["ContinueProgram",{"post":{"heap":[["v3",[[["Dereference"],["v4","_"]]]],["v8",[[["Dereference"],["v3","_"]]]]],"stack":[[["ProgramVar",{"plain":"return","mangled":null}],["v8","_"]]],"attrs":"_"},"pre":{"heap":[],"stack":[],"attrs":"_"},"skipped_calls":"_","path_condition":"_"}]]]]]
```
Reviewed By: ezgicicek
Differential Revision: D24503387
fbshipit-source-id: 9bd08e93b
Summary:
Output summaries in json format, so that other tools can exploit the
results of infer without having to be written inside infer itself.
For now the json for a summary is just one line saying "opaque" :)
Set up the infra to generate (yo)json automatically using
ppx_yojson_conv. See it in action in the next diff.
Reviewed By: ezgicicek
Differential Revision: D24503343
fbshipit-source-id: e24a2fff3
Summary:
- output the "menu" of the interactive mode on stderr instead of stdout
so that we can pipe the results, eg
`infer debug --procedures --procedures-summary | cat`
This will be more useful when we add an option to output json, as
otherwise the menu pollutes the json.
- Allow "--select" to work for infer-debug too:
`infer debug --procedures --procedures-summary --select 0`
Reviewed By: da319
Differential Revision: D24503301
fbshipit-source-id: d7fb4b713
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:
Before this diff we would just propagate the callee abstract state,
which doesn't make sense in the caller. We could just remove the state
from AbortProgram altogether as Pulse itself doesn't use it, but for now
let's at least make sure it's accurate.
Also needed for upcoming hackathon that will start from Pulse error
specs to try to produce tests :)
Reviewed By: ezgicicek
Differential Revision: D24448073
fbshipit-source-id: 9100b3f79
Summary:
Detect when changed files paths are trying to escape the project root
and try to guess their relative project root (which has to be a parent
of the current one).
This is perhaps a bit too hacky but it works for the case we need it to.
Reviewed By: martintrojer
Differential Revision: D24425427
fbshipit-source-id: 018651740
Summary:
Sometimes you need several project roots (eww), this makes paths make
sense even in that case.
Reviewed By: martintrojer
Differential Revision: D24336244
fbshipit-source-id: f087d533a
Summary:
Don't wait until pre-analysis has completed before updating the task
bar with the current procedure being analysed.
Reviewed By: skcho
Differential Revision: D24418609
fbshipit-source-id: afedaf687
Summary:
Move from Map to SafeInvertedMap:
- joining two branches where only one branch had the variable set to a
given closure or type should *not* keep that information around: now
we correctly get Top instead
- the "Safe" part is an optimisation that doesn't store Top values in
the map, which is important as most values are not closures so we
don't care about storing the fact that we don't know anything about
them
Reviewed By: ngorogiannis
Differential Revision: D24418560
fbshipit-source-id: 0ac701502
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:
Another step in the refactoring of the starvation domain:
- Main purpose is to mediate access to the set of critical pairs in a summary through a fold function (`fold_critical_pairs_of_summary`) and not through direct field access to that set. This will allow eliding storage of critical pairs entirely and dynamically generating those when folding.
- Remove optional arguments as much as possible, as this led to unused arguments not being caught.
- Helper functions distributed more logically among modules.
Reviewed By: skcho
Differential Revision: D24275399
fbshipit-source-id: d23123a48
Summary: As part of a refactor, push `thread` from the enclosing type (`CriticalPairElement`) into `Event.t`.
Reviewed By: jvillard
Differential Revision: D24161709
fbshipit-source-id: bd812f3fd
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:
`NonBlocking` methods have starvation errors silenced (but not deadlock ones). This is implemented by summarising as usual and then filtering out such events when the summary is finalised, if the method is annotated as such.
It's better to not record the events in the first place.
Reviewed By: ezgicicek
Differential Revision: D24237465
fbshipit-source-id: 1b24a26f0
Summary: This will be needed in the next diff so that we can find all classes that conform to `NSCopying` protocol.
Reviewed By: skcho
Differential Revision: D24216549
fbshipit-source-id: 297b527a6
Summary:
- rename the checker "Uninitialized Variable" to "Uninitialized Value"
as this is the name of the issue type
- delete timestamp XML comment from the man pages to avoid future git
churn when updating the website
- counting is hard
Reviewed By: martintrojer
Differential Revision: D24219165
fbshipit-source-id: cf3057373
Summary:
I wanted to change the default to "callgraph" but that created issues in
our tests, introducing flaky behaviours and even a failure due to trying
to run the pre-analysis multiple times (not 100% sure it was related).
Instead, document the various options and put the option in the analysis
manual so users can choose.
Reviewed By: martintrojer
Differential Revision: D24193751
fbshipit-source-id: 4b7c33a79
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 is a preparation to extend the polynomial domain. What I will do in the following diff is
to extend the polynomial domain to include a symbol representing "cost of function pointer call".
Then, the symbol will be instantiated to another polynomial cost at call sites.
The polynomial domain is defined as a sum of
* non negative insteger
* map from `bound` to polynomail domain
```
type poly = {const: NonNegativeInt.t; terms: poly M.t}
```
The problem is all `bound`s are adderessed as integer values, rather than polynomial, thus it
does not fit to include the symbol for the "cost of function pointer call".
In this diff, it introduces a `Key` module for the map `M`, the type of `Key.t` is the same to the previous key of `M`. The actual extension of the key type will be in the following diff.
Reviewed By: ezgicicek
Differential Revision: D23991401
fbshipit-source-id: cec64347f
Summary: Subsequent diff will push information down into `Event.t` so as preparation, turn all variant values into records.
Reviewed By: jvillard
Differential Revision: D24115201
fbshipit-source-id: d2126dd49
Summary:
`std::lock(x,y,z)` simultaneously acquires locks `x,y,z` in a deadlock free manner (essentially an unspecified fixed order).
Starvation currently deals with it by exploiting properties of the state domain. It's a map from locks to number of times the lock is held, so the count of many locks can be increased at the same time without recording any particular lock order.
In upcoming diffs the domain will be refactored into a tree of nested lock acquisitions (for other reasons) and that domain necessarily records lock order. The obvious way of doing this correctly is to allow `std::lock` as an atomic even (ie, without trying to break it into multiple acquisitions).
This diff does exactly that, by changing the `Event.LockAcquire` variant to take a list of locks.
Reviewed By: ezgicicek
Differential Revision: D24052304
fbshipit-source-id: 410c812d7
Summary:
This diff avoids some usages of functors when they are applied only once in `polynomials.ml`. While
functor is in general helpful to abstract data and make code cleaner, it makes us hard to understand
code, eg `merlin-locate` does not give an actual definition point of terms when it is from a module
parameter.
Reviewed By: ngorogiannis
Differential Revision: D23991164
fbshipit-source-id: c61289e84
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:
This makes sure we call `AbductiveDomain.summary_of_post` exactly once
per post-condition. Notice in particular in the diff:
- in Pulse.ml we remove a now-certified-useless "is_unsat_expensive"
call
- in PulseOperations.ml we add a previously-missing call to
`summary_of_post` (it's needed to remove local variables from the
symbolic state + normalize)
The price to pay is ugly type annotations and down-casting peppered in a
few places, in reasonable number.
Reviewed By: da319
Differential Revision: D24078564
fbshipit-source-id: 3102cacf0
Summary:
Take another page from the Incorrectness Logic book and refrain from reporting issues on paths unless we know for sure that this path will be taken.
Previously, we would report on paths that are merely *not impossible*. This goes very far in the other direction, so it's possible we'll want to go back to some sort of middle ground. Or maybe not. See the changes in the tests to get a sense of what we're missing.
Reviewed By: ezgicicek
Differential Revision: D24014719
fbshipit-source-id: d451faf02
Summary: We add a naive model for `forEach` idiom for Java's Iterable and Maps. The model is naive because it doesn't take the cost of the lambda into account. This will be fixed later.
Reviewed By: da319
Differential Revision: D23868203
fbshipit-source-id: 37d169c6f
Summary: Gradle produces a number of compilation units which are currently captured sequentially. This diff parallelizes this step.
Reviewed By: jvillard
Differential Revision: D23930978
fbshipit-source-id: d71c22ba3
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 can be used by additional tooling for further analysis (e.g.
codemods, autofixes, etc).
Reviewed By: ngorogiannis
Differential Revision: D23987694
fbshipit-source-id: b9fa343ac
Summary: When using the database write server process, the parent has to wait for it to fully start up before continuing. Instead of dying after a fixed amount of time, never terminate (external timeouts can take care of that) and use exponential backoff to avoid spamming.
Reviewed By: jvillard
Differential Revision: D23989959
fbshipit-source-id: ff5232c14
Summary:
Testing procedure for java source parser
- we can run directly the parser without compiling and analysing the source file
- we add a test file
Reviewed By: ngorogiannis
Differential Revision: D23705199
fbshipit-source-id: 2103c1681
Summary: The code that parses `/proc/cpuinfo` expects IDs to be sequential. This is not always the case, so keep track of which IDs appear instead of keeping the maximum, when counting.
Reviewed By: jvillard
Differential Revision: D23987776
fbshipit-source-id: 98d267560
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: Subtle false positives and negatives in Hil make Sil preferable. This diff gets rid of the CFG-emulation of Hil, while still using Hil expressions.
Reviewed By: da319
Differential Revision: D23815026
fbshipit-source-id: 731a6d299
Summary: Without curly braces, it is declaration of globals, not fields.
Reviewed By: ezgicicek
Differential Revision: D23866604
fbshipit-source-id: dd685c8d6
Summary:
The previous diffs recorded it for the case when the unvetted value is
dereferenced or otherwise used wrongly. This case finishes the work,
recording the needed signature for the remaining case (when the
offending third party has a non-nullable param with nullable passed
inside)
Reviewed By: ngorogiannis
Differential Revision: D23706679
fbshipit-source-id: e6f641223
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:
This diff simply changes the type of autorelease size models as `CostUtils.model` instead of
`unit`. In the following diff, it will add a model that has more complicated semantics that does not return the unit cost.
Reviewed By: ezgicicek
Differential Revision: D23785159
fbshipit-source-id: 300808574
Summary:
`WithBlockParameters` is generated by a pre-analysis to express concrete block parameters. However,
before this diff, the block parameters only have names, which is insufficient to find their
summaries. This diff change `WithBlockParameters` to have `Block.t` that includes the parameters of
the block, so we can find blocks' summaries.
Reviewed By: ezgicicek
Differential Revision: D23785148
fbshipit-source-id: 9034f4f8d
Summary: Also, fix some leftover meaningless logging, enable all cost issue types on diff testing so that we can see the unreachable cost issues
Reviewed By: skcho
Differential Revision: D23784563
fbshipit-source-id: 1b4a2b582
Summary:
Nullifying these leads to observable side-effects, like in the added
test.
Reviewed By: da319
Differential Revision: D23759756
fbshipit-source-id: 559a6486b
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:
We in fact build NullsafeIssue.t by parts, filling needed details until
all the info is provided.
This is nicely expressed via partial functions.
See the next diff that uses this to add additional information to the created
`NullsafeIssue.t`
Reviewed By: skcho
Differential Revision: D23706674
fbshipit-source-id: ca5fac704
Jules Villard
Ezgi Çiçek
Joshua Selbo
David Pichardie
Mitya Lyubarskiy
Daiva Naudziuniene
Nikos Gorogiannis
Sungkeun Cho
Artem Pianykh
Josh Berdine
Martin Trojer