Summary: This diff prunes array sizes in Java by adding the size alias on the `get_array_length` function calls.
Reviewed By: ezgicicek
Differential Revision: D16983501
fbshipit-source-id: a924af09d
Summary:
Rename some AL source files so they mention AL explicitly instead of
"cFrontend" which could be confused with the clang frontend itself.
Reviewed By: ezgicicek
Differential Revision: D16962539
fbshipit-source-id: 29237cd1c
Summary: AL makes for close to a third of the source files in clang/. Put the code in its own folder for clarity.
Reviewed By: ezgicicek
Differential Revision: D16962438
fbshipit-source-id: 3373e69b9
Summary:
This diff avoids that an integer value is pruned to the bottom by
comparing to a pointer.
For example, before this diff,
assume((int*)x == p);
assume((int*)x != p);
where x is an integer, x is pruned to the bottom in both of the assume
cases. So, there were some, unintentional and false, unreachable
code.
Depends on D16960199
Reviewed By: ezgicicek
Differential Revision: D16964735
fbshipit-source-id: 90a3c8c80
Summary:
It changes the order of StdBasicString and StdVector for easier
reviewing of the following diff.
Reviewed By: ezgicicek
Differential Revision: D16963153
fbshipit-source-id: 50325e4e1
Summary:
The access path format forced some weird patterns on this code, simplify
using the access expression structure.
Reviewed By: ezgicicek
Differential Revision: D16960660
fbshipit-source-id: e8faf619e
Summary:
It prunes the size of collections when the size function is called in the condition expression. The diff extended the alias domain to understand temporary variables of SIL from Java.
Depends on D16761461
Reviewed By: ezgicicek
Differential Revision: D16761611
fbshipit-source-id: 849c5c71c
Summary:
This adds logging of the number of nodes in the reverse analysis call graph, and the number of these nodes that are invalidated by incremental analysis.
This data will show the precision of incremental analysis.
Reviewed By: ngorogiannis
Differential Revision: D16939101
fbshipit-source-id: 1e465f1a6
Summary:
It revises Java's cast model to keep type in the location when it has a field.
The type information is useful especially when generating ondemand values of Collection elements.
Depends on D16807299
Reviewed By: ezgicicek
Differential Revision: D16807378
fbshipit-source-id: 636e54429
Summary:
It doesn't make sense to use incremental analysis without specifying changed files. This is a possible source of future bugs.
This commit causes infer to die if incremental analysis is used without changed files.
---
Previously:
I think this code is currently a bit brittle because the CI shadow builds sometimes use `--incremental-analysis` because they are called with the same command as the diff analysis.
I am worried that in the future the shadow builds could be broken by this, although everything looks like it works right now. This diff would prevent breaking smoke builds in future because the shadow builds don't set changed-files (afaik).
However, possibly this is not the right place to fix the problem. It might be better to change the CI, I'm not sure.
Reviewed By: mityal
Differential Revision: D16829192
fbshipit-source-id: 5ee1ce9d0
Summary:
Use whatever information we can to decide whether to use C or Java
syntax when outputting an access expression, now that we store them as
such.
Also, make cluster callbacks explicitly set the language, as this was not done before and led to some confusion (Clang being set when analysing a Java file).
Reviewed By: skcho
Differential Revision: D16884160
fbshipit-source-id: 40adf9f35
Summary:
Access paths are too coarse to properly address C/C++ instructions, and lead to false positives and negatives. Begin the process of porting the underlying domains to access expressions, in a results-preserving way. This roughly consists in:
- Adding missing functions in `AccessExpression` to mirror those in `AccessPath`.
- Replacing `AccessExpression` for `AccessPath` and removing conversions from the former to the latter except in:
- Printing functions, to ensure formatting issues won't change tests/CI.
- Reporting/deduplication still happens through access path conversion, as we need an analogue of `ModuloThis` for `AccessExpression`.
- In selected places, ignore any access type not present in `AccessPath` (ie. dereference/take address of).
Reviewed By: jberdine
Differential Revision: D16856721
fbshipit-source-id: 5e3a88b75
Summary: Ideally, we should be able to handle them like pruning if statements but for now, let's add the test.
Reviewed By: skcho
Differential Revision: D16938842
fbshipit-source-id: 04fae9559
Summary:
It uses inline record for Loc.Field
Depends on D16807279
Reviewed By: ezgicicek
Differential Revision: D16807299
fbshipit-source-id: 45eab34a4
Summary: Checker configs were defined as tuples which are amenable to problems with wrong ordering. Let's make convert them to a record type to prevent such issues.
Reviewed By: jvillard
Differential Revision: D16936737
fbshipit-source-id: 32aad6e97
Summary: Javalib is not happy about some class signature, let's try to silence it temporarily.
Reviewed By: ngorogiannis
Differential Revision: D16917273
fbshipit-source-id: 39ce4adee
Summary:
Change the logic of the annotation reachability checker in the following
ways:
1. Sanitizers take priority over sinks, i.e. a procedure that is both a
sink and a sanitizer is not a sink. This changes the existing tests
that seemed to assume the opposite. However I think that way is more
useful and goes better with the fact that sanitizers are specified as
"overrides".
2. When applying a summary, check again that we are not in a sanitizer
for the corresponding sink.
Without (2) this there was a subtle bug when several rules were
specified. For example, if `sink_wrapper()` wraps `sink()` for a rule
`R` then the summary of `sink_wrapper()` will be: `R-sink : call to sink()`.
Then, suppose `sanitizer()` calls `sink_wrapper()` and `sanitizer()` is
a sanitizer for `R` but not for another rule `R'`. The previous code
would add the call to `sink()` to the summary of `sanitizer()` because
it's not a sanitizer for `R'`, even though `sink()` is not a sink for
`R'`!
The current code will re-apply the rules correctly so that sinks are
matched only against the right sanitizers.
Reviewed By: skcho
Differential Revision: D16895577
fbshipit-source-id: 266cc4940
Summary:
- run the tests! they weren't hooked up to the main Makefile :/
- add some html debug messages
- formatting
Reviewed By: skcho
Differential Revision: D16895578
fbshipit-source-id: e96d737cc
Summary:
I added this logging in D16730426, to try and debug incremental analysis.
I don't need the logging anymore, so I'm taking it out. I don't this it's very useful for users.
Reviewed By: ezgicicek
Differential Revision: D16904498
fbshipit-source-id: 88b0f1cb5
Summary:
Since it is non-sense to get ranges of boolean values, this diff
ignores control values that only contain boolean symbols.
Depends on D16804802
Reviewed By: ezgicicek
Differential Revision: D16804808
fbshipit-source-id: ccb25db4d
Summary: The pattern "check if an access has already been reported, otherwise see if it is a violation, report it, then add it to the set of reported accesses" is too much copy pasta. Push that into the reporting functions.
Reviewed By: ezgicicek
Differential Revision: D16859208
fbshipit-source-id: 5370efd41
Summary: I'm not entirely sure why this function was written in such a horrendous way :)
Reviewed By: skcho
Differential Revision: D16858396
fbshipit-source-id: d998e17f3
Summary: Functions with empty body have unit cost, not zero. The unit cost comes from the start node.
Reviewed By: skcho
Differential Revision: D16855642
fbshipit-source-id: 6b5181faf
Summary:
Before this diff it returned `[0,size-1]`, but which became bottom
when size was given by 0. As a result, it made the both branches of
`if(iterator.hasNext())` unreachable. Similarly, if the size was 1,
it only visited the false branch of the if condition because the
condition value was `[0,0]` at that time.
This diff changes it to return `[0,size]`, so that
* the false branch is reachable when the size is 0
* the both branches are reachable when the size is 1
Reviewed By: ezgicicek
Differential Revision: D16803000
fbshipit-source-id: f8772be27
Summary: We want to keep big O notation as simple as possible in cost analysis reports (especially in diff time). Therefore, let's not show constants/min/max in big O notations even though the resulting asymptotic bound might be inaccurate. Developers can click on the trace and see the actual cost.
Reviewed By: skcho
Differential Revision: D16731351
fbshipit-source-id: 2e16f7eca
Summary: In order to test changes to bigO notation, let's record them in test results.
Reviewed By: skcho
Differential Revision: D16763972
fbshipit-source-id: c1376909b
Summary:
It renames a function to make it clear what it does.
Depends on D16761451
Reviewed By: ezgicicek
Differential Revision: D16761461
fbshipit-source-id: b989cc274
Summary: We do not need to keep the elements type of vector in the field.
Reviewed By: ezgicicek
Differential Revision: D16761451
fbshipit-source-id: 6d5384662
Summary: Add a sanity check that looks up the `INFERVERSION` environment variable and, if set, checks that the current binary matches that version.
Reviewed By: skcho
Differential Revision: D16761575
fbshipit-source-id: 9d5c32220
Summary:
Incremental analysis isn't behaving as expected in production, and I think the deletion and re-creation of the results directory is why
Adding logging to test this theory
Reviewed By: ngorogiannis
Differential Revision: D16730426
fbshipit-source-id: 0a670cabf
Summary:
Correct the models of ArrayList initialization. Basically, there are two ways to initialize:
- by setting an initial capacity, which creates an empty list
- by passing another collection as an argument
Before, we had only modeled the second case which was resulting in wrong memory model for the first case. This diff fixes that.
Reviewed By: skcho
Differential Revision: D16711055
fbshipit-source-id: e82faf191
Summary: Remove duplicate `s` when printing the time taken by the analysis
Reviewed By: ngorogiannis
Differential Revision: D16710277
fbshipit-source-id: 3ba7f6693
Summary:
It adds a vector model of `data` method.
Depends on D16687280
Reviewed By: ezgicicek
Differential Revision: D16689400
fbshipit-source-id: 156016b3c
Summary:
It adds a model of vector::push_back
Depends on D16687225
Reviewed By: ezgicicek
Differential Revision: D16687269
fbshipit-source-id: 9d2a73fca
Summary:
It enables pruning of vector's size when the return value of the function call of `vector::size` is pruned.
Depends on D16687167
Reviewed By: ezgicicek
Differential Revision: D16687225
fbshipit-source-id: 793a21b3a
Summary:
Add logging for the number of procedures whose summaries are invalidated by incremental analysis
This will help verify that incremental analysis is working as expected in production
Reviewed By: ngorogiannis
Differential Revision: D16686911
fbshipit-source-id: 53c89c3bb
Summary:
It generates vector value ondemand when it is given as a parameter.
Depends on D16645589
Reviewed By: ezgicicek
Differential Revision: D16645624
fbshipit-source-id: 7498c8ab2
Summary:
This diff makes it sure that Inferbo does nothing on relational domain
at function calls when the command line option for them is not given.
Reviewed By: ezgicicek
Differential Revision: D16647903
fbshipit-source-id: 74ef251fe
Summary: Test that cost analysis works with incremental analysis enabled
Reviewed By: ezgicicek
Differential Revision: D16620101
fbshipit-source-id: b41403954
Summary:
The models are only for biabduction so try to make that clearer in the
code and documentation.
Reviewed By: skcho
Differential Revision: D16603147
fbshipit-source-id: 4a2be53de
Summary:
It's not being worked on and is not in a state where it works.
It would probably better to write this as a script of some kind or else
resurrect this subcommand in a form where it behaves more like a script,
ie fork/execs infer analyses instead of having them be function calls
(but then it might as well *be* a script as it would likely be more
flexible).
In any case...
youarealreadydead
Reviewed By: ezgicicek
Differential Revision: D16602417
fbshipit-source-id: d0d129539
Summary:
These have proved to be too fragile to maintain as they would often break
compilation of user code. They have been off by default for more than a year
now (D7350715).
Removing the include models shows a more accurate picture of what infer results
look like in production. As such, lots of tests have changed, mostly
biabduction but also in inferbo. SIOF was using include-based models too but
now libc++ is better and iostreams are implemented in a way that SIOF
understands (instead of being magical creatures) so nothing changed there.
Reviewed By: skcho
Differential Revision: D16602171
fbshipit-source-id: ce38f045b
Summary:
Write a test for the invalidation of changed procedures
Reverse analysis graph for this test: https://fburl.com/graphviz/ybidpidq
The procedures marked as changed are `a` and `d`, and this causes `a,b,c,d,e,main` to be invalidated as expected
Reviewed By: jvillard
Differential Revision: D16579526
fbshipit-source-id: cbec304ce
Summary:
Add test `incremental_analysis_remove_file` to the toplevel makefile so that it is called by `make test` etc
Also swapped the src_before and src_after files so the test checks file removal instead of addition.
Reviewed By: jvillard
Differential Revision: D16562340
fbshipit-source-id: 79bab5f66
Summary: Models of Java's Collection mistakenly assumed that there was an argument for empty set whereas `Collections.emptySet()` doesn't have any actuals. This diff fixes that an also removes the type argument from the corresponding model definition.
Reviewed By: skcho
Differential Revision: D16582314
fbshipit-source-id: d4304dc60
Summary: Sometimes programmers use integer underflow to get a maximum number of that type. This diff assumes that integer underflows from the syntactical form `(unsigned 0) - constant` is intended by the programmer, and suppresses the alarms of which.
Reviewed By: ezgicicek
Differential Revision: D16560639
fbshipit-source-id: 206f30dbc
Summary:
Count the following:
- how many procedures were *actually* analyzed (i.e. some checkers ran
on them)
- how many times an analysis result was retrieved from the local cache
and how many times it was missed
Reviewed By: skcho
Differential Revision: D16561867
fbshipit-source-id: 8c43ce13c
Summary:
Instead of `let incr_foo () = global_stats.foo <- global_stats.foo + 1` where you have to
check that you copy/pasted the right stuff and substituted `foo`
everywhere, write `let incr_foo () = incr Fiels.summary_foo` where
there's less room for errors.
Reviewed By: artempyanykh
Differential Revision: D16561868
fbshipit-source-id: 77ea09bef
Summary:
Before this diff, it gave up pruning of linear bound by minmax bound.
For example, `overapprox_min (x+c1, c2+min(d1,y))` was `x+c1`.
However, we can get a bit more preciser value as follows.
```
overapprox_min (x+c1, c2+min(d1,y))
<= min (x+c1, c2+d1)
= c1+min(c2+d1-c1, x)
```
Reviewed By: ezgicicek
Differential Revision: D16543837
fbshipit-source-id: 8fdbce097
Summary:
- make most behaviours independent of the java version so that either works fine without user intervention
- modify regexp used to parse `javac` output to work for all versions
- no need to be sure we are in Java 11 to match java 11-only method name in quandary
- for the rest, provide a command-line flag to specify the java version manually in case it differs from the version that infer was built against
- this only affects the Maven integration for now
To do all that, also change the configure script to record the version of java instead of just a boolean for whether it's >= 10.
Reviewed By: ezgicicek
Differential Revision: D16493988
fbshipit-source-id: 622e91b25
Summary:
The default values of config options can sometimes depend on build-time
configuration values. This makes checking that the manuals "remain the same"
trickier as the manuals can be different depending on the platform. This
removes *all* default values from the checked-in manuals. We could be more
fine-grained and scrub only the values that are susceptible to change but for
now this is probably good enough.
This is done by implementing new options `--help-scrubbed` and
`--help-scrubbed-full` and using these in our tests instead of `--help` and
`--help-full` (which remain unaffected).
Also don't wrap the default values in `$(i,...)` anymore because the defaults
can trigger line breaks and then the man page is ill-formatted because that
format is stupid.
Reviewed By: mityal
Differential Revision: D16543779
fbshipit-source-id: bc929ff8c
Summary:
This diff prevents that the latest prune value is overwritten as top
from callees.
Reviewed By: jvillard
Differential Revision: D16540391
fbshipit-source-id: bdd5b42ed
Summary:
This diff improves the precision of the mod operator.
For example, result of x % c (when x>=0 and c>0) is
(before) [0, c-1]
(after) [0, min(c-1,x)]
Reviewed By: ezgicicek
Differential Revision: D16518578
fbshipit-source-id: a68660ee7
Summary: This diff tries to do weak update for the abstract locations by pointer arithmetic, e.g. `p[n]` or `p+n`, even if the type of `p` is declared as a simple pointer, not an array.
Reviewed By: ezgicicek
Differential Revision: D16458367
fbshipit-source-id: 3b4cdd7e4
Summary:
A test that records the expected output of:
- reverse analysis call graph
- introduced/pre-existing/fixed issues
- cost analysis results
Currently only the call graph is non-empty.
Reviewed By: PhoebeMay
Differential Revision: D16495470
fbshipit-source-id: f186d73d2
Summary:
The `represents_multiple_values` flag was adopted to decide whether updating abstract value strongly or weakly. However, the flag was included in the `Val` domain, which is strange, because it is a property of abstract locations, rather than abstract values. This makes the behavior of memory update function depend on the abstract value to update, making its code complicated.
This diff detach the `represents_multiple_values` flag from the `Value` domain, thus the memory update does not depend on the abstract value. Since this is a refactoring, I believe the diff should not make many semantic changes.
Reviewed By: ezgicicek
Differential Revision: D16441734
fbshipit-source-id: 4c10779d7
Summary: The reverse analysis call-graph is logged if `--debug-level-analysis` > 0, so that its value can be inspected for tests
Reviewed By: jvillard
Differential Revision: D16440567
fbshipit-source-id: 1ec6af1f3
Summary:
Pulse didn't treat local variables going out of scope as invalidating the corresponding address in memory. This diff fixes that by
- marking all local variables that exits the scope with the attribute `AddressOfStackVariable`
- before we write the summary for the proc, we make sure to invalidate all such addresses local to the procedure as `Invalid.` If such an address is read, then we would raise a use-after-lifetime issue.
Reviewed By: jvillard
Differential Revision: D16458355
fbshipit-source-id: 3686524cb
Summary: This implements incremental diff analysis by deleting only the summaries that need to be re-analyzed, keeping all summaries corresponding to procedures that have not been changed (or had a callee change).
Reviewed By: jvillard
Differential Revision: D16358474
fbshipit-source-id: 660a704a0
Summary: Incremental analysis relies on analysis results in the results directory, so don't delete this directory if `--incremental-analysis` is used
Reviewed By: jvillard
Differential Revision: D16458113
fbshipit-source-id: bf7c63cb3
Summary:
The same logic for reading the env var and defaulting to the local
results dir was duplicated in a bunch of places.
Reviewed By: artempyanykh
Differential Revision: D16458976
fbshipit-source-id: 41f1a4f9c
Summary:
There was a little bit of code duplication around `analyze_proc` to deal with
the fact that we may be starting from either a proc name or a proc desc. Create
a new `callee` type that represents this more explicitly. This allows not
loading the `proc_desc` eagerly when we don't need it, although that doesn't
seem to impact perf measurably.
Reviewed By: ezgicicek
Differential Revision: D16442221
fbshipit-source-id: 8e8ebbd6b
Summary:
TL;DR: Until this patch, if you ran infer on MacOS Mojave you most
likely would get an error related to missing header files. Now infer
tries to automatically locate current MacOS SDK path thus providing a
better experience for first time users.
Consider helloworld.c
```
#include <stdio.h>
int main()
{
return 0;
}
```
Invoking the analysis `infer -- cc -c helloworld.c` fails with
facebook-clang-plugins/.../include/c++/v1/stdio.h:108:15: fatal error: 'stdio.h' file not found
The reason for this is twofold:
1. infer uses its own clang, not Apple's one (thus custom paths are
not properly setup).
2. Apple stopped copying standard headers from SDK to /usr/include.
Reviewed By: jvillard
Differential Revision: D16377866
fbshipit-source-id: c336ad64f
Summary: This test wasn't building correctly or being called by the toplevel makefile
Reviewed By: jvillard
Differential Revision: D16458386
fbshipit-source-id: 48a0c2f36
Summary:
:
In previous commits we introduced deriving capabilities in Backend stats.
Now we can rewrite the code so that usage of all fields is enforced at compile time.
Reviewed By: jvillard
Differential Revision: D16458130
fbshipit-source-id: aef751440
Summary:
As discussed in D16358474, the options `--reanalyze` and `--incremental-analysis` are not compatible
This diff warns about the compatibility problem in the documentation
Reviewed By: jvillard
Differential Revision: D16440482
fbshipit-source-id: ab841ace6
Summary:
Minor improvements to a bunch of stuff
- mostly, always log backend stats and time spent analysing, regardless of `-j 1`
- output stats more like they appear in the record
- split `InferAnalyze.main` to be more readable (hopefully)
Reviewed By: mityal
Differential Revision: D16440586
fbshipit-source-id: 6f91f53cd
Summary:
It's a bit more annoying to `incr` but is more uniform with the other
`mutable` field.
Reviewed By: ngorogiannis
Differential Revision: D16359027
fbshipit-source-id: 817cd94a0
Summary:
Modify the scheduler to collect results from children at the end of the
parallel execution. Use this to collect backend stats and log their
aggregated sum.
Reviewed By: ezgicicek
Differential Revision: D16358867
fbshipit-source-id: 775792ef7
Summary:
Use it to trace summary stats. It will be used more/better in future
diffs that aggregates stats across parallel workers.
Reviewed By: ezgicicek
Differential Revision: D16358868
fbshipit-source-id: 764614153
Summary:
Summary.ml defines both a bunch of types and how to use them and a
mechanism to save and store summaries on disk while maintaining a
complex in-memory cache of what's on disk. Make the distinction clear.
Reviewed By: ngorogiannis
Differential Revision: D16358869
fbshipit-source-id: 9d4c6cb77
Summary:
It downgrades issues of void pointer to L5, because of its impreciseness. This is not
ideal but Inferbo cannot analyze arrays pointed by void pointers precisely at the moment.
Reviewed By: jvillard
Differential Revision: D16379911
fbshipit-source-id: f2c016aba
Summary:
Fixes#1126
Different checks contain some ad hoc places that look at this param, but there is no systematic way to suppress this.
The centralized place that is filtering results is `reporting.ml`.
Note that this diff does not remove other usages, because they do more than mere filtering results.
Reviewed By: jvillard
Differential Revision: D16339655
fbshipit-source-id: afabdc97a
Summary:
The analysis call graph is the call graph from the perspective of the analyses run in infer
This commit creates the reverse analysis call graph to be used for incremental diff analysis
Reviewed By: ezgicicek
Differential Revision: D16335938
fbshipit-source-id: 0cbab3298
Summary: The reverse call graph will be constructed by adding edges one-by-one, so expose functionality in CallGraph to add a single edge to the graph
Reviewed By: jvillard
Differential Revision: D16285016
fbshipit-source-id: 553fe1ecf
Summary:
Add a function to delete a summary from disk and caches
This is needed so that summaries corresponding to invalidated procedures can be removed (as part of incremental diff analysis)
Reviewed By: ngorogiannis
Differential Revision: D16332752
fbshipit-source-id: 7d3c7a121
Summary:
The genrule-capture integration with Java relies on a buck config flag `infer.infer_bin=<path to infer>` (see test changes in `DEFS` below).
In a CI environment where the infer binary is checked out under a random directory, this means that the buck genrule is keyed by a random string (the path to infer), and this defeats caching.
Switch to the following contract: the genrule target does not expect a config flag at all. Instead it runs whichever `infer` binary is in the path. To make sure the binary is the same one with the originator, the capture integration runs buck under a modified `PATH` where the originator `infer` is sure to be the first matching entry.
NB cache invalidation is still OK because we rely on `infer.version` buck config flag, which will be hashed into the rulekey.
Reviewed By: jvillard
Differential Revision: D16332696
fbshipit-source-id: 2975d5c26
Sungkeun Cho
Jules Villard
Nikos Gorogiannis
Phoebe Nichols
Ezgi Çiçek
Martin Trojer
Artem Pianykh
Mitya Lyubarskiy
David Pichardie