Summary:
Relative paths in jbuilder + `S **` seem to be a losing combo. Spell out the directories instead.
This was obtained via letting jbuilder generate .merlin, then curating it by hand.
Reviewed By: jberdine
Differential Revision: D6159600
fbshipit-source-id: 7d799bb
Summary:
:
Make both buck capture and compilation database handle buck command line arguments and invoke buck query the same way.
Plus allow:
- target patterns `//some/dir:` and `//some/dir/...`. However since `//some/dir:#flavor` and `//some/dir/...#flavor` are not supported, they need to be expanded before adding the infer flavor.
- target aliases (defined in `.buckconfig`)
- shortcuts `//some/dir` rewritten to `//some/dir:dir`
- relative path `some/dir:name` rewritten to `//some/dir:name`
Reviewed By: jvillard
Differential Revision: D5321087
fbshipit-source-id: 48876d4
Summary: These can make the compilation fail, so don't use them unless we really need to.
Reviewed By: mbouaziz
Differential Revision: D6147574
fbshipit-source-id: ab2c3fa
Summary:
If you write
```
boolean readUnderLockOk() {
synchronized (mLock) {
return mField;
}
}
```
it will be turned into
```
lock()
irvar0 = mField
unlock()
return irvar0
```
in the bytecode. Since HIL eliminates reads/writes to temporaries, it will make the above code appear to perform a read of `mField` outside of the lock.
This diff fixes the problem by forcing HIL to perform all pending reads/writes before you exit a critical section.
Reviewed By: jberdine
Differential Revision: D6138749
fbshipit-source-id: e8ad9a0
Summary: In HIL, allow deref'ing a magic address like `0xdeadbeef` for debugging purposes. Previously, we would crash on code like this.
Reviewed By: mbouaziz
Differential Revision: D6143802
fbshipit-source-id: 4151924
Summary:
Linters are now considered a "checker", like backend checkers. This makes, eg,
`--racerd-only` disable the linters, which is more intuitive.
We can now express `-a linters` and `--clang-frontend-action` in terms of these
two new options. For instance, `-a linters --clang-frontend-action lint` is the
same as `--linters-only --no-capture`.
This is another step in the direction of getting rid of `--analyzers`.
Reviewed By: dulmarod
Differential Revision: D6147387
fbshipit-source-id: 53622b2
Summary: A stepping stone to have descriptive issue types for each kind of flow rather that lumping everything into `QUANDARY_TAINT_ERROR`.
Reviewed By: mbouaziz
Differential Revision: D6126690
fbshipit-source-id: a7230c0
Summary: This check is deprecated and will be replaced by a dedicated checker to detect unitialized values.
Reviewed By: mbouaziz
Differential Revision: D6133108
fbshipit-source-id: 1c0e9ac
Summary: Previously, this would incorrectly classify types like `map<std::string, int>` as a buffer
Reviewed By: mbouaziz
Differential Revision: D6125530
fbshipit-source-id: c8564de
Summary:
Before this change, analyses using HIL needed to pass `IdAcessPathMapDomain.empty` to abstract interpreter, and would get back the map as part of the post.
This is a confusing API and was a pain point for Dino in trying to use HIL.
This diff adds a HIL wrapper around the abstract interpreter that hides these details.
It replaces `LowerHIL.makeDefault` as the new "simplest possible way" to use HIL.
Reviewed By: jberdine
Differential Revision: D6125597
fbshipit-source-id: 560856b
Summary:
This is in the spec for clang compilation databases.
Also improves error messages when we fail to parse the compilation database.
closes#771
Reviewed By: dulmarod
Differential Revision: D6123832
fbshipit-source-id: 070f70f
Summary: Saving the list of bugs in a set removes the ordering. Also, there should be no need to remove the duplicated warnings at this level.
Reviewed By: sblackshear
Differential Revision: D6060554
fbshipit-source-id: a78d35d
Summary:
Looked at some problematic summaries and am noticing some common patterns.
Adding some dynamic checks to be run in debug mode in order to make sure my fixes for these patterns are real.
Reviewed By: jeremydubreil
Differential Revision: D5779593
fbshipit-source-id: 9de6497
Summary:
The options passed via `--Xbuck` are usually meant for `buck build` but we also
use them for `buck targets`. It's hard to know which options to take into
account for which Buck subcommand, so just filter out known-incompatible ones.
Reviewed By: dulmarod
Differential Revision: D6123459
fbshipit-source-id: 976b978
Summary:
Install ocamlformat from github as part of `make devsetup`, and use it
for formatting OCaml (and jbuild) code.
Reviewed By: jvillard
Differential Revision: D6092464
fbshipit-source-id: 4ba0845
Summary: Sinks weren't being printed when passthroughs are empty (which, for now, is always). Oops!
Reviewed By: jvillard
Differential Revision: D6110164
fbshipit-source-id: 4488ab0
Summary: This will make it easier to generalize the checker to handling uninitialized struct fields.
Reviewed By: ddino
Differential Revision: D6099484
fbshipit-source-id: b9c534b
Summary:
Buck reads the version on stderr or, very recently, from either stdout or stderr.
This makes infer output the version of stderr when called from Buck or invoked as javac, and on stdout otherwise.
Reviewed By: martinoluca
Differential Revision: D6098392
fbshipit-source-id: 23f1d5a
Summary: This makes `--biabduction-blacklist-path-regex` and others work as expected.
Reviewed By: mbouaziz
Differential Revision: D6088625
fbshipit-source-id: 8f1daa3
Summary:
This is a better default than running the biabduction analysis only, now that
we have several mature checkers.
Reviewed By: jeremydubreil
Differential Revision: D6051186
fbshipit-source-id: 04ac0c6
Summary: In preparation for making `-a checkers` the default (when no analyzer is specified), let's test `-a checkers` by default.
Reviewed By: mbouaziz
Differential Revision: D6051177
fbshipit-source-id: d8ef611
Summary:
Refactor `RegisterCheckers` to give a record type to checkers instead of a tuple type.
Print active checkers with their per-language information.
Improve the manual entries slightly.
Reviewed By: sblackshear
Differential Revision: D6051167
fbshipit-source-id: 90bcb61
Summary:
Whenever we see a use of a lock, infer that the current method can run in a multithreaded context. But only report when there's a write under a lock that can be read or written without synchronization elsewhere.
For now, we only infer this based on the direct usage of a lock; we don't assume a caller runs in a multithreaded context just because its (transitive) callee can.
We can work on that trickier case later, and we can work on smarter inference that takes reads under sync into account. But for now, warning on unprotected writes of reads that occur under sync appears to be too noisy.
Reviewed By: jberdine
Differential Revision: D5918801
fbshipit-source-id: 2450cf2
Summary: This commit adds unsigned symbol for preciser analysis results with less number of uses of min/max operators.
Reviewed By: mbouaziz
Differential Revision: D6040437
fbshipit-source-id: 999ca4c
Summary:
This is useful if some command behaves like one infer knows how to integrate with. For instance:
```
infer --force-integration clang -- clang-3.8 -c examples/hello.c
```
Reviewed By: jeremydubreil, mbouaziz
Differential Revision: D6051589
fbshipit-source-id: dd693b0
Summary:
This allows us to get rid of code that copied source files individually. I
didn't migrate the various flags that could be included as it doesn't look like
that's possible yet (they depend on the context and on some configuration
options).
Reviewed By: jberdine
Differential Revision: D6051825
fbshipit-source-id: c28dd37
Summary:
This will allow most of the checkers, except the bi-abduction, to skip the analysis on the specialized clone of the methods used to handle dynamic dispatch. Doing this, we can run the bi-abduction analysis using:
infer -a checkers --biabduction
without risk of conflicts on the resolution of dynamic dispatch.
Reviewed By: sblackshear
Differential Revision: D6052347
fbshipit-source-id: 0c75bf3
Summary: This removes cases of duplicated warnings when the dynamic dispatch handling specializes a method Infer already reported on.
Reviewed By: sblackshear
Differential Revision: D6060337
fbshipit-source-id: dbefeca
Summary:
It looks like the old code for expanding access paths assumed that `FormalMap.get_formals_indexes` assumed the returned list tuples would be sorted by index, but it's actually sorted by var name.
As a consequence, formals might be expanded into the wrong actuals.
This diff fixes the problem by not relying on `get_formals_indexes`.
Reviewed By: jberdine
Differential Revision: D6056365
fbshipit-source-id: 09f3208
Summary: The order of the elements in the list maters since the function `string_to_analyzer` will return the fist element found in the list. Inverting the `"biabduction"` and `"infer"` entries in the list allows D6051146 to have no functional implications.
Reviewed By: sblackshear
Differential Revision: D6055840
fbshipit-source-id: 6cf5ac2
Summary:
This was a crutch from the days before ownership analysis.
We shouldn't need it anymore, and it was actually causing FP's because we were skipping analysis of `ImmutableList.builder()` and not understanding that the return value is owned.
Reviewed By: jeremydubreil
Differential Revision: D6035631
fbshipit-source-id: afa0ade
Summary:
One day `-a infer` will alias `-a checkers` so for now create another, more
explicit analyzer name that can be used to migrate progressively. For instance,
after this commit what should continue to use `-a biabduction` can change to
that, so that when `-a infer` becomes an alias to `-a checkers` it can keep
working.
Reviewed By: jeremydubreil
Differential Revision: D6051146
fbshipit-source-id: 1ef4c34
Summary:
This generates `--resource-leak-only` automatically, and make the other
checkers' `-only` option work as expected with respect to `--resource-leak` too
(eg, `--resource-leak --biabduction-only` disables resource leak).
Reviewed By: jeremydubreil
Differential Revision: D6051134
fbshipit-source-id: 2d4a2ba
Summary:
1. Mark some Makefile targets as depending on `MAKEFILE_LIST` so they get rebuilt on Makefile changes
2. Do not show boolean options with no documentation in the man pages (like we do for other option types).
3. Default to Lazy dynamic dispatch for the checkers.
4. In the tests, use `--<checker>-only` instead of relying on `--no-default-checkers`
5. `--no-filtering` is redundant if `--debug-exceptions` is passed
Reviewed By: jeremydubreil
Differential Revision: D6030578
fbshipit-source-id: 3320f0a
Summary: We will then be able to merge the tests for the other checkers without affecting these lab tests
Reviewed By: jvillard
Differential Revision: D6039433
fbshipit-source-id: e575ce9
Summary:
Another step toward running the biabduction analysis as a checker.
Depends on D6038210
Reviewed By: jvillard
Differential Revision: D6038682
fbshipit-source-id: fed45bf
Summary:
The previous version of the code was trying to lookup from disk the procedure description of the procedure to analyze, which was in fact already loaded in memory.
This diff fixes one of the issues preventing the bi-abduction to run as a checker when using the lazy dynamic dispatch algorithm.
Reviewed By: sblackshear
Differential Revision: D6038210
fbshipit-source-id: 10a98ee
Summary:
9c7fc65 introduced a large performance regression, this diff eliminates it and a bit more.
Instead of constructing the quotiented access list map in a two-step process of first constructing a map of all accesses and then quotienting it, the quotiented map is constructed directly by using a coarser comparison function on keys. Partitioning the access map O(number of access paths) times, using an apparently expensive partition predicate, seems to be causing trouble based on rough profile data.
Reviewed By: da319
Differential Revision: D6005262
fbshipit-source-id: 077846c
Summary: Stack-allocated variables cannot be raced on in cpp as every thread has its own stack. At the beginning of the analysis we add ownership to the local variables.
Reviewed By: jberdine
Differential Revision: D6020506
fbshipit-source-id: 0a90a97
Summary: Now that we report write-write races involving more than one write, we need to improve the traces accordingly.
Reviewed By: jberdine
Differential Revision: D6026845
fbshipit-source-id: b1366dd
Summary:
This is to avoid getting BUSY from sqlite when the machine is busy (not
necessarily busy because of infer).
Reviewed By: jberdine
Differential Revision: D6020022
fbshipit-source-id: ca0f913
Summary:
Next step to issue deduplication: do not keep safety conditions that are subsumed by others.
Only do it if they do not have infinite bound: replacing `0 < size` by `1 < size` is ok, but replacing it by `+oo < size` is not because it looks much more like a lack of precision.
Reviewed By: skcho
Differential Revision: D5978455
fbshipit-source-id: acc2384
Summary:
A specific type of alias is added for the vector::empty() result and it is used at pruning.
Now, there are two types of aliases:
- "simple" alias: x=y
- "empty" alias: x=v.empty() and y=v.size
So, if x!=0, y is pruned by (y=0). Otherwise, i.e., x==0, y is pruned by (y>=1).
Reviewed By: mbouaziz
Differential Revision: D6004968
fbshipit-source-id: bb8d50d
Summary:
`pp_instr_list` was not tailrec causing a stack overflow on big code.
Also simplified a few things
Reviewed By: jvillard
Differential Revision: D5995451
fbshipit-source-id: 40a4911
Summary: The may alias analysis relation that the thread safety analysis uses is very specific to Java and causes many false alarms for C++ code. This diff disables it when analyzing C++ code. Improving it to avoid false negatives is left for later.
Reviewed By: sblackshear
Differential Revision: D5974182
fbshipit-source-id: 9c7fc65
Summary:
The analyzer currently does not understand the control flow of
Singletons, which leads to false alarms. This diff is an unsound hack
that simply ignores any read or write accesses made when computing the
value of a singleton.
Reviewed By: sblackshear
Differential Revision: D5979639
fbshipit-source-id: 34caecb
Summary:
Model folly::SharedMutex lock and unlock operations, some
apache::thrift::concurrency::ReadWriteMutex operations, some
folly::RWSpinLock operations, and folly::MicroSpinLock operations.
Reviewed By: sblackshear
Differential Revision: D5974225
fbshipit-source-id: 19e2816
Summary:
The biabduction backend can raise exceptions that will be caught when triggered
from within the biabduction backend itself (eg, `analyze_procedure` called from
Symexec as a result of an ondemand analysis, because Symexec will catch these),
but not caught when called as the result of an ondemand analysis emanating from
another analyzer (eg ThreadSafety).
Make the biabduction more self-contained by wrapping the analysis of a
procedure inside a `try/with` with similar properties as the one of Symexec.
Reviewed By: jeremydubreil
Differential Revision: D5986335
fbshipit-source-id: 36a5d32
Summary:
Attempting to translate these will not go well as the declaration still depends
on some template arguments. Added a test that was previously crashing the
frontend.
Also extend the catching of "Unimplemented" and other errors to `translate_one_decl` as it was useful to debug this issue. In particular, reraise all exceptions and log some additional context when doing so.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D5976357
fbshipit-source-id: fca8e38
Summary:
Previously, annotating something ThreadSafe meant "check that it is safe to run all of this procedure's methods in parallel with each other" (including self-parallelization).
This makes sense, but it means that if the user writes no annotations, we do no checking.
I'm moving toward a model of inferring when an access might happen on a thread that can run concurrently with other threads, then automatically checking that it is thread-safe w.r.t to all other accesses to the same memory (on or off the current thread thread).
This will let us report even when there are no `ThreadSafe` annotations.
Any method that is known to run on a new thread (e.g., `Runnable.run`) will be modeled as running on a thread that can run in parallel with other threads, and so will any method that is `synchronized` or acquires a lock.
In this setup, adding `ThreadSafe` to a method just means: "assume that the current method can run in parallel with any thread, including another thread that includes a different invocation of the same method (a self race) unless you see evidence to the contrary" (e.g., calling `assertMainThread` or annotating with `UiThread`).
The key step in this diff is changing the threads domain to abstract *what threads the current thread may run in parallel with* rather than *what the current thread* is. This makes things much simpler.
Reviewed By: jberdine
Differential Revision: D5895242
fbshipit-source-id: 2e23d1e
Summary:
Indicate if read or write is protected, and do not print only the
field but also the object involved in the race.
Reviewed By: sblackshear
Differential Revision: D5974250
fbshipit-source-id: 351a576
Summary:
Expanding traces currently works in the following way:
Given a `TraceElem.Kind` `k` we want to report in `foo`, we look for a callee `C` of `foo` that has a `TraceElem.Kind` equal to `k` in its summary, grab the summary for `C`, then repeat until we bottom out.
This isn't very flexible: it insists on equality between `TraceElem.Kind`'s as the criteria for expanding a trace.
This diff introduces a new `matches` function for deciding when to expand a trace from a caller into a callee.
Clients that don't want strict equality can implement a fuzzier kind of equality inside this function.
I've gone ahead and done this for the trace elemes of thread-safety.
In the near future, equivalent access paths won't always compare equal from caller to callee, so we want to match their suffixes instead.
Reviewed By: jvillard
Differential Revision: D5914118
fbshipit-source-id: 233c603
Summary:
Move Inferbo safety conditions to their own file.
Split the old `Condition.t` to a condition together with a trace.
This will ease having: different kind of condition and several traces for the same condition (see following diff)
Reviewed By: jvillard
Differential Revision: D5942030
fbshipit-source-id: d74a612
Summary: Not using this for now, and it seems good to simplify the complex domain as much as we can.
Reviewed By: jberdine
Differential Revision: D5970233
fbshipit-source-id: a451503
Summary:
This is due to the changes in `facebook-clang-plugins` where objc_object_type_info now has a `field_prefix` set to `ooti_`
See 5f2042abe6 for the changes made to `facebook-clang-plugins`
update-submodule: facebook-clang-plugins
Reviewed By: jvillard
Differential Revision: D5963064
fbshipit-source-id: 9705774
Summary:
Running `infer report foo.specs` would overwrite report.json with an empty json.
Only recompute report.json when running `infer-analyze` or `infer-run`.
Reviewed By: mbouaziz
Differential Revision: D5963500
fbshipit-source-id: e579c6f
Summary:
Use a monotonic time source instead.
Also, sleep between retries in the Serialization code.
Reviewed By: jberdine
Differential Revision: D5941697
fbshipit-source-id: 05efbe1
Summary:
Despite what the sqlite manual says, it looks like it's possible for sqlite not
to remove the -shm and -wal files after a successful termination.
Close the database and clean them up in Buck mode so that they do not perturb
the cache.
Reviewed By: martinoluca
Differential Revision: D5953967
fbshipit-source-id: 9068b01
Summary:
Inject a marker using a global variable in <iostream>, and whitelist it so that
the frontend translates it.
Use the marker in the SIOF checker to tell whether a file includes <iostream>.
If so, start the analysis of its methods assuming that the standard streams are
initialised.
Reviewed By: sblackshear
Differential Revision: D5941343
fbshipit-source-id: 3388d55
Summary:
The previous domain for SIOF was duplicating some work with the generic Trace
domain, and basically was a bit confused and confusing. A sink was a set of
global accesses, and a state contains a set of sinks. Then the checker has to
needlessly jump through hoops to normalize this set of sets of accesses into a
set of accesses.
The new domain has one sink = one access, as suggested by sblackshear. This simplifies
a few things, and makes the dedup logic much easier: just grab the first report
of the list of reports for a function.
We only report on the fake procedures generated to initialise a global, and the
filtering means that we keep only one report per global.
Reviewed By: sblackshear
Differential Revision: D5932138
fbshipit-source-id: acb7285
Summary:
Wrap ANSITerminal so that it doesn't apply control codes unless they can be
interpreted, and so that clients are unable to call ANSITerminal directly.
Reviewed By: mbouaziz
Differential Revision: D5953733
fbshipit-source-id: 6b3602a
Summary:
Bottom bounds do not make sense (what is the meaning of `[_|_; 1]`?), let's get rid of them.
`Bot` was useful for substitution though, with a special meaning, use `bottom_lifted` for that case.
Reviewed By: skcho
Differential Revision: D5941796
fbshipit-source-id: 5778255
Summary: A bottom interval in a safety condition doesn't make sense. Let's not allow it at all.
Reviewed By: skcho
Differential Revision: D5941552
fbshipit-source-id: 6bd2a65
Summary: If we know for sure we won't need to store an attribute in the DB, there's no need to compute its marshalled value.
Reviewed By: jberdine
Differential Revision: D5891050
fbshipit-source-id: cf4534e
Summary:
This adds more structure to the SQL schema backing attributes. With that, we
can transfer the logic for updating attributes in SQLite, instead of doing
optimistic concurrency in the client.
Reviewed By: jberdine
Differential Revision: D5891038
fbshipit-source-id: 6577ba2
Summary:
- use a similar key as for specs in the attributes table
- cache blob computations
- this improves memory usage a lot
Reviewed By: mbouaziz
Differential Revision: D5824177
fbshipit-source-id: c318577
Summary:
Preparing statements allocates memory and is generally best done once and for all.
We need to re-prepare statements when opening a new DB connexion (after each fork).
Reviewed By: mbouaziz
Differential Revision: D5824157
fbshipit-source-id: 4d239ac
Summary: Not sure if useful but seems sensible. It disappears at the top of the stack when we do merging in SQL.
Reviewed By: mbouaziz
Differential Revision: D5824131
fbshipit-source-id: fd64752
Summary: Use `pp` functions until it needs to be turned into a string.
Reviewed By: jvillard
Differential Revision: D5941473
fbshipit-source-id: 87ca9df
Summary:
Use an SQLite database to store proc attributes, instead of files on disk.
Wrap SQLite operations in two layers:
1. `SqliteUtils` provides helper functions to make sure DB operations succeed
2. `KeyValue` provides a functor to expose a simple and type-safe key/value store backed by the SQLite DB.
Reviewed By: jberdine
Differential Revision: D5640053
fbshipit-source-id: 31050e5
Summary:
Its value is unused in Infer and is constantly emitted as None from facebook-clang-plugins, so it was also removed from facebook-clang-plugins (a96c39601f)
update-submodule: facebook-clang-plugins
Reviewed By: jvillard
Differential Revision: D5940900
fbshipit-source-id: e7fd6ae
Summary: Remove functions that are redundant with Base.List functions on sorted lists.
Reviewed By: mbouaziz
Differential Revision: D5931792
fbshipit-source-id: caec210
Summary: The case of closures was not considered for the convertion of SIL instructions into HIL instructions
Reviewed By: sblackshear
Differential Revision: D5929675
fbshipit-source-id: bb6920a
Summary: These folders are left empty when linting, so better not create them.
Reviewed By: jvillard
Differential Revision: D5921418
fbshipit-source-id: f6efc4f
Summary: The tests are slower when running in debug mode, and it creates a lot of html outputs
Reviewed By: sblackshear
Differential Revision: D5916511
fbshipit-source-id: 07c90b7
Summary:
This diff does two things:
# Infer no longer add the contrains that the return value of a skip function is never null. This was leading to false negatives and is not necessary as those return value are treated angelically
# Infer now support `Nonnull` on the return value of skip functions.
Reviewed By: jberdine, sblackshear
Differential Revision: D5840324
fbshipit-source-id: bbd8d82
Summary:
`reraise` was error-prone when one forgot to save the backtrace between where the exception is caught and where it is reraised.
If any exception was raised (even caught) in between, the printed backtrace would be the one of the last exception thrown and it would be very confusing.
This diff kills `reraise` and introduces `reraise_after exn ~f` and `reraise_if exn ~f` to be used right after catching the exception.
Also turned some of them to the common pattern `try_finally ~f ~finally`.
Reviewed By: jvillard
Differential Revision: D5911244
fbshipit-source-id: 9883d1e
Summary: Example of combination between annotating fields with nullable and the biabduction analysis in Objective C
Reviewed By: dulmarod
Differential Revision: D5906016
fbshipit-source-id: b95c6e0
Summary:
The interval bound of the abstract domain is extended.
`[min|max](int, symbol)` => `int [+|-] [min|max](int, symbol)`
As a result, `vector::empty` can be modelled to return a more precise value: `1 - min(1, size)`.
Reviewed By: mbouaziz
Differential Revision: D5899404
fbshipit-source-id: c8c3f49
Summary:
We take it into account to not report bugs inside the available block. This requires a plugin change.
update-submodule: facebook-clang-plugins
Reviewed By: jvillard
Differential Revision: D5891511
fbshipit-source-id: 21a02ad
Summary:
With this change and the previous facebook-clang-plugins change, infer no
longer exhausts the biniou buffer when reading the serialized C++ AST.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D5891081
fbshipit-source-id: cf48eac
Summary:
These get way too big in C++, and we only use the very first word of them, to
tell apart class from struct from union... so sad, very bad.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D5890594
fbshipit-source-id: 49e6284
Summary:
The only language types we have are Java/Clang/Python. The unit of analysis is a source file, and you can't write a source file that mixes two or more of these languages (to the best of my knowledge).
This diff simplifies using the assumption that all procedures in a file are written in the same language.
Reviewed By: jeremydubreil
Differential Revision: D5886942
fbshipit-source-id: 88c3759
Summary:
The only language types we have are Java/Clang/Python. The unit of analysis is a source file, and you can't write a source file that mixes two or more of these languages (to the best of my knowledge).
This diff simplifies using the assumption that all procedures in a file are written in the same language.
Reviewed By: jeremydubreil
Differential Revision: D5886942
fbshipit-source-id: 8555a16
Summary: Only Eradicate uses this, no need to create it for every checker.
Reviewed By: jeremydubreil
Differential Revision: D5886775
fbshipit-source-id: 7242437
Summary:
A Java cluster checker currently defines a "cluster" as all of the procedures in the same class.
But the cluster checker actually knows about all the procedures defined in the same source file.
In some checkers (such as thread-safety), we want to aggregate results across classes in the same file, not just methods in the same class.
This refactoring leaves the behavior the same for now, but will make it easier to do this in the near future.
Reviewed By: jeremydubreil
Differential Revision: D5885896
fbshipit-source-id: 0815fca
Summary:
Calling functions that raise exceptions (even if they get caught) may smudge
the backtraces we get from OCaml. We need to record the original backtrace
*before* calling such fuctions on the path between catching an exception and
reraising it.
Also change the heptuple returned by `Exceptions.recognize_exception` into a
record type, and make that function not raise when classifying exceptions.
Reviewed By: jberdine
Differential Revision: D5882934
fbshipit-source-id: 8e99fe8
Summary: Infer can then detect the resource leak when resources are stored into a container
Reviewed By: sblackshear
Differential Revision: D5887702
fbshipit-source-id: 6106cfb
Summary: The point of the tracing mode is to compute all the possible path leading to an error state. However, within a method, many of those paths are not feasibile in practice. This leads to many false alarms for the resource leak analysis.
Reviewed By: sblackshear
Differential Revision: D5888695
fbshipit-source-id: 2dbc57b
Summary: Model the `remove(...)` and the `clear()` method of `HashMap`.
Reviewed By: sblackshear
Differential Revision: D5887674
fbshipit-source-id: c3f40ee
Summary: Handling the utility functions for asserting that we're on background thread.
Reviewed By: jberdine
Differential Revision: D5863435
fbshipit-source-id: 3ad95b5
Summary:
Previously, we just tracked a boolean representing whether we were possibly on the main thread (true) or definitely not on the main thread (false).
In order to start supporting `Thread.start`, `Runnable.run`, etc., we'll need something more expressive.
This diff introduces a lattice:
```
Any
/ \
Main Background
\ /
Unknown
```
as the new threads domain. The initial value is `Unknown`, and we introduce `Main` in situations where we would have introduced `true` before.
This (mostly) preserves behavior: the main difference is that before code like
```
if (*) {
assertMainThread()
} else {
x.f = ...
}
```
would have recorded that the access to `x.f` was on the main thread, whereas now we'll say that it's on an unknown thread.
Reviewed By: peterogithub
Differential Revision: D5860256
fbshipit-source-id: efee330
Summary:
It's useful to be able to disable de-duplication on the command line with `--no-filtering`.
Gate de-duplication with `Config.filtering` and move the de-duplication tests to a new directory under the build systems tests.
Reviewed By: jeremydubreil
Differential Revision: D5865329
fbshipit-source-id: 5094f5b
Summary:
Since D5381239, infer is careful not to delete directories that do not "look
like" results directories on startup, in case the user passed, eg, `-o /`.
In our repo, lots of results dir are created by build/test of infer, and when
the version of infer changes and the expected contents of results directories
change then it might start refusing to delete the results directories created
with another version of infer.
Add an option to force infer to delete the results directory no matter how
dodgy it looks, and use it in our repo by adding the option in every
.inferconfig.
Reviewed By: mbouaziz
Differential Revision: D5870984
fbshipit-source-id: 09412de
Summary: Will then be easier to understand if some changes in the test results is legit or not.
Reviewed By: sblackshear
Differential Revision: D5863961
fbshipit-source-id: 7eb3f33
Summary: Parmap delivers a better scheduling, which works as a pipeline, as opposed to what existed before, which schedules processes in batches.
Reviewed By: jvillard
Differential Revision: D5678661
fbshipit-source-id: a632c71
Summary:
Suggesting to add `_Nullable` on the fields checked for, or assigned to, `nullptr` will allow the biabduction analysis to report null dereferences that are related to the lifetime of objects.
Depends on D5832147
Reviewed By: sblackshear
Differential Revision: D5836538
fbshipit-source-id: c1b8e48
Summary: The prune nodes where translated as `prune (expr = false)` and `prune ( expr != false)`. This case is a bit tricky to deconstruct in HIL. This diff translates the prune instructions as just `prune !expr` for the true branch and `prune expr` for the false branch.
Reviewed By: dulmarod
Differential Revision: D5832147
fbshipit-source-id: 2c3502d
Summary:
We need to make sure that destructors of virtual base classes are called only once. Similarly to what clang does, we have two destructors for a class: a destructor wrapper and an inner destructor.
Destructor wrapper is called from outside, i.e., when variables go out of scope or when destructors of fields are being called.
Destructor wrappers have calls to inner destructors of all virtual base classes in the inheritance as their bodies.
Inner destructors have destructor bodies and calls to destructor wrappers of fields and inner destructors of non-virtual base classes.
Reviewed By: dulmarod
Differential Revision: D5834555
fbshipit-source-id: 51db238
Summary:
The "placement new" operator `new (e) T` constructs a `T` in the pre-allocated memory address `e`.
We weren't translating the `e` part, which was leading to false positives in the dead store analysis.
Reviewed By: dulmarod
Differential Revision: D5814191
fbshipit-source-id: 05c6fa9
Summary:
With flambda (`-O3`), compilation time is ~5x slower, but the backend is ~25% faster!
To mitigate the atrocious compilation times, introduce a new `opt` build mode in the jbuilder files.
- build in "opt" mode by default from the toplevel (so that install scripts and external users get the fastest infer by default), in "default" mode by default from infer/src (since the latter is only called directly by infer devs, for faster builds)
- `make byte` is as fast as before in any mode
- `make test` will build "opt" by default, which is very slow. Solution for testing (or building the models) locally: `make BUILD_MODE=default test`.
- You can even change the default locally with `export BUILD_MODE=default`.
The benchmarks are to be taken with a sizable pinch of salt because I ran them only once and other stuff could be running in the background. That said, the perf win is consistent across all projects, with 15-20% win in wallclock time and around 25% win in total CPU time, ~9% win in sys time, and ~25% fewer minor allocations, and ~5-10% fewer overall allocations. This is only for the backend; the capture is by and large unaffected (either the same or a tad faster within noise range).
Here are the results running on OpenSSL 1.0.2d on osx (12 cores, 32G RAM)
=== base
infer binary: 26193088 bytes
compile time: 40s
capture:
```lang=text
real 1m7.513s
user 3m11.437s
sys 0m55.236s
```
analysis:
```lang=text
real 5m41.580s
user 61m37.855s
sys 1m12.870s
```
Memory profile:
```lang=json
{
...
"minor_gb": 0.1534719169139862,
"promoted_gb": 0.0038930922746658325,
"major_gb": 0.4546157643198967,
"allocated_gb": 0.6041945889592171,
"minor_collections": 78,
"major_collections": 23,
"compactions": 7,
"top_heap_gb": 0.07388687133789062,
"stack_kb": 0.3984375,
"minor_heap_kb": 8192.0,
...
}
```
=== flambda with stock options (no `-Oclassic`, just the same flags as base)
Exactly the same as base.
=== flambda `-O3`
infer binary: 56870376 bytes (2.17x bigger)
compile time: 191s (4.78x slower)
capture is the same as base:
```lang=text
real 1m9.203s
user 3m12.242s
sys 0m58.905s
```
analysis is ~20% wallclock time faster, ~25% CPU time faster:
```lang=text
real 4m32.656s
user 46m43.987s
sys 1m2.424s
```
memory usage is a bit lower too:
```lang=json
{
...
"minor_gb": 0.11583046615123749, // 75% of previous
"promoted_gb": 0.00363825261592865, // 93% of previous
"major_gb": 0.45415670424699783, // about same
"allocated_gb": 0.5663489177823067, // 94% of previous
"minor_collections": 73,
"major_collections": 22,
"compactions": 7,
"top_heap_gb": 0.07165145874023438,
"stack_kb": 0.3359375,
"minor_heap_kb": 8192.0,
...
}
```
=== flambda `-O2`
Not nearly as exciting as `-O3`, but the compilation cost is still quite high:
infer: 37826856 bytes
compilation of infer: 100s
Capture and analysis timings are mostly the same as base.
Reviewed By: jberdine
Differential Revision: D4867979
fbshipit-source-id: 99230b7
Summary:
This can be a long-running step and it's useful to know how long it took. We
already dump some statistics on stderr after merging is done, this just adds
one more line.
Reviewed By: mbouaziz
Differential Revision: D5833580
fbshipit-source-id: 70e19ab
Summary:
Simple instance of the problem: analyzing the following program times out.
```
#include <tuple>
void foo() {
std::tuple<std::tuple<int>> x;
}
```
Replacing `std::tuple<std::tuple<int>>` by `std::tuple<int>` makes the analysis
terminate.
In the AST, both tuple<tuple<int>> and tuple<int> have the same template
specialization type: "Pack" (which means we're supposed to go look into the
arguments of the template to get their values). This is not information enough
and that's the plugin fault.
On the backend side, this means that two types have the same Typ.Name.t, namely
"std::tuple<_>", so they collide in the tenv. The definition of
tuple<tuple<int>> is the one making it into the tenv. One of the fields of the
corresponding CxxRecord is of type "tuple<int>", which we see as the same
"tuple<_>", which causes the loop.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D5775840
fbshipit-source-id: 0528604
Summary:
Sort the complete set of warnings by everything except procname, then de-duplicate.
This scheme prevents reporting identical error messages on the same line/same file.
This is important for avoiding duplicate reports on multiple instantiations of the same template.
Reviewed By: jberdine
Differential Revision: D5819467
fbshipit-source-id: 984f47f
Summary: The resolution was previously only happening for constructors, but calls to private methods or to `super` are also neither static calls nor virtual calls. In this case, the resolution logic should be the same as for constructors.
Reviewed By: sblackshear
Differential Revision: D5830376
fbshipit-source-id: 9b56f80
Sam Blackshear
Jules Villard
Dulma Churchill
Mehdi Bouaziz
Dino Distefano
Jeremy Dubreil
Anders Papitto
Josh Berdine
Martino Luca
Sungkeun Cho
Daiva Naudziuniene
Kyriakos Nikolaos Gkorogiannis