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
Summary:
The reporting phases iterates over each procedure summary and print all the issues from each procedure.
That's nice because we don't have to build a big list of the issues in-memory, but it's not so nice if you want to ouput the reports in a certain order or de-duplicate them.
This diff builds the in-memory list and outputs the issues afterward. By itself, this isn't very useful. But in the near future it will allow us to:
- Group all of the issues from the same file (finally!!!)
- Get rid of duplicate issues on multiple instantiations of the same C++ template
- Probably other cool stuff too
Reviewed By: jeremydubreil, mbouaziz
Differential Revision: D5816646
fbshipit-source-id: 799bcd0
Summary:
The clang frontend uses `assert false` for unimplemented features that should
abort method translations, as well as for genuine internal errors.
Distinguishing between the two, we can fail hard on the latter and not the
former.
1. This introduces a new exception `Unimplemented` that is used instead of `assert false` where appropriate.
2. Changed some other cases into `die ...` (when there's an error message to display)
3. Wherever a path in the code that we assumed to be unreachable was observed reachable, we now raise `IncorrectAssumption`. These should be fixed, but the fixes are not obvious.
Reviewed By: sblackshear
Differential Revision: D5784384
fbshipit-source-id: 61b55af
Summary:
In the most recent version of clang plugin, lambda captures were moved from `LambdaExpr` to `CXXRecordDecl`. Updating infer to reflect this change.
update-submodule: facebook-clang-plugins
Reviewed By: jvillard
Differential Revision: D5823034
fbshipit-source-id: dd5fc45
Summary: This will run infer with `--no-keep-going` by default in our tests.
Reviewed By: sblackshear
Differential Revision: D5814237
fbshipit-source-id: c1e1a4e
Summary:
"Running as clang" was its own infer subcommand. That's not terribly good
because it makes it hard to specify another subcommand, and in particular it
broke the `compile` subcommand in some integrations because "running as clang"
would always do capture. The cmake tests required to run with `--keep-going`
because of this.
Instead of having its own fake subcommand, simply add a new boolean in the
config for "infer runs as clang", as we do for javac already (used in the mvn
integration).
Also make logging of the environment better.
Reviewed By: jberdine
Differential Revision: D5813986
fbshipit-source-id: 72b96cd
Summary: This is to prevent test failures to happen whenchanging the code a little.
Reviewed By: sblackshear
Differential Revision: D5815349
fbshipit-source-id: 8516102
Summary: Destroying local variables that are out of scope after `continue`.
Reviewed By: jberdine
Differential Revision: D5804120
fbshipit-source-id: 638cff5
Summary: I often find myself needing a generic `Pp.seq` where I can specify the separator.
Reviewed By: jeremydubreil
Differential Revision: D5803915
fbshipit-source-id: fb8d30d
Summary: That match branch is Java-only but could be reached with a C++ method, causing a crash.
Reviewed By: jberdine, mbouaziz
Differential Revision: D5814041
fbshipit-source-id: 6b1b501
Summary: It's useful to be able to configure both max depth and max width
Reviewed By: jeremydubreil
Differential Revision: D5801567
fbshipit-source-id: 0138cd7
Summary:
Races on the internal implementation of data structures defined in
system headers are currently not detected, since the memory accesses
are in procedures that are not analyzed.
This diff adds models for a few std::map operations that indicate if
they read or write the underlying representation.
Reviewed By: da319
Differential Revision: D5804293
fbshipit-source-id: 55ff28c
Summary: Try to preserve the original backtrace. Introduce `reraise` in the global namespace.
Reviewed By: jberdine
Differential Revision: D5804121
fbshipit-source-id: 0947a47
Summary:
This makes it much easier to read infer logs coming from these subprocesses
that use a sub-results dir.
Reviewed By: jberdine
Differential Revision: D5777783
fbshipit-source-id: f074536
Summary: Destroying local variables that are out of scope after `break`.
Reviewed By: jberdine
Differential Revision: D5764647
fbshipit-source-id: a7e06ae
Summary: The successor node of `continue` was not correct inside the `do while`.
Reviewed By: sblackshear
Differential Revision: D5769578
fbshipit-source-id: d7b0843
Summary:
Read the documentation and it doesn't seem like these functions are guaranteed to choose the same value in different runs.
I hypothesize that these may be the source of flakiness in the thread-safety tests/smoke tests.
Reviewed By: jvillard
Differential Revision: D5794384
fbshipit-source-id: 02b7a96
Summary: With this diff, the analysis trace will jump to the definition of the skipped methods when the location is known. This is especially useful when the analysis is relying on the method annotations.
Reviewed By: sblackshear
Differential Revision: D5783428
fbshipit-source-id: 561b739
Summary: The code was previously inconsistent as passing the option was using an empty list of instructions to translate the code but still using the original JBIR representation to connect the nodes.
Reviewed By: sblackshear
Differential Revision: D5795903
fbshipit-source-id: 88c0e64
Summary:
The behaviour of infer was observed to be different in 4.04.2 vs (4.05.0 or
4.04.2+flambda). Investigating further, the behaviour is also different in byte
vs native versions of infer under 4.04.2. Looking at the Changelog of 4.05.0
(thanks mbouaziz), there are fixes for inconsistent behaviours between byte
and native relative to evaluation order.
Lots of debugging later, this 1 line patch is born. Also use `List.concat_map`
instead of re-implementing it.
Reviewed By: jberdine
Differential Revision: D5793809
fbshipit-source-id: 374fb4c
Summary: With Logging.exit you have more control of the code that invokes exit, for example when forking and running certain functions that may in turn invoke exit, and you want to handle the execution flow differently - like invoking certain callbacks before exiting, or not exiting at all.
Reviewed By: jvillard
Differential Revision: D5746914
fbshipit-source-id: 596fba1
Summary: This will prevent trivial errors like misspelling node names, e.g. when those are typed as part of the operators `IN-NODE <node-name>` or `HOLDS-IN-NODE <node-name>`
Reviewed By: dulmarod
Differential Revision: D5680411
fbshipit-source-id: e241c1c
Summary: This should be equivalent as far as bug finding is concerned, but will allow the analysis traces to jump into the skipped methods.
Reviewed By: sblackshear
Differential Revision: D5778245
fbshipit-source-id: 3fd061f
Summary:
We supported globals as sources before, but we did so by allowing ClangTrace etc. to match against any access path in the footprint of the trace.
This is very powerful/flexible, but it's ultimately not a good idea because it leads to traces that are hard to read.
This is because a footprint source doesn't have any information about its provenance: we might know that the value came from a global, but we don't know where the read occurred.
The mechanism for handling procedure calls as sources already knows how to solve this problem.
This diff implements globals as sources as a special case of procedure call sources instead.
This will give us much nicer traces with full provenance of the read from the global.
Reviewed By: mbouaziz
Differential Revision: D5772299
fbshipit-source-id: 491ae81
Summary: This is almost equivalent to the previous one except in the case where new and old are both undefined: before, we would just pick "old", but now we pick the biggest according to their source files. I think the previous behaviour was a bug because it was more non-deterministic.
Reviewed By: jberdine
Differential Revision: D5649481
fbshipit-source-id: aeb527d
Summary: It is not clear to me what the removed code was for in the first place. Basically, it was replacing the pure part of propositions in a semantically equivalent way, e.g. replacing `a = b /\ Attribute(a)` to `a = b /\ Attribute(b)`, and `a = b /\ Attribute(b)` to `a = b /\ Attribute(a)`.
Reviewed By: jberdine
Differential Revision: D5657366
fbshipit-source-id: 93cd9e0
Summary: The list of fields of a Java object in SIL is the list of fields declared in the class plus all the fields declared in the the super classes. It turns out that we were missing the fields declared in the implemented interfaces.
Reviewed By: sblackshear
Differential Revision: D5720386
fbshipit-source-id: d65c9de
Summary:
Calling `Exn.backtrace` doesn't give us the current backtrace, only the one of
the latest exception raised. Change the logic of toplevel exception catching to
print the right backtrace.
Also, do not use `Format` to print from `Config` as `Logging` may have messed
with it already.
Finally, throw in some colours!
Reviewed By: jberdine
Differential Revision: D5764825
fbshipit-source-id: cd51688
Summary: Add procedure where error is triggered (if available) + bad data to messages.
Reviewed By: jvillard
Differential Revision: D5756398
fbshipit-source-id: a16f7cf
Summary:
When a lambda has an `auto` parameter, the inferred type of the parameter because part of the name.
Our heuristic for identifying lambda was checking if the lambda's name was exactly `operator()`, which won't catch this case.
Reviewed By: jvillard
Differential Revision: D5753323
fbshipit-source-id: 85ff75a
Summary:
Not translating these properly was causing false positives for the dead store analysis in cases like
```
int i = 0;
return [j = i]() { return j; }();
```
Reviewed By: da319
Differential Revision: D5731562
fbshipit-source-id: ae79ac8
Summary:
Keep track of generated files better and make sure we nuke them on `make clean`.
Also get rid of a few unused variables in infer/src/Makefile.
Reviewed By: sblackshear
Differential Revision: D5754553
fbshipit-source-id: b5fca41
Summary: We inject destructor calls of base classes inside destructor bodies after the destructor calls of fields.
Reviewed By: dulmarod
Differential Revision: D5745499
fbshipit-source-id: 90745ec
Summary: Refactoring destructor translation to take type pointer instead of qual type as a parameter, as we only have type pointers for base classes.
Reviewed By: dulmarod
Differential Revision: D5745490
fbshipit-source-id: 121f492
Summary: We used to crash whenever we hit these. The simple translation implemented here is not particularly inspiring, but it is better than crashing.
Reviewed By: jvillard
Differential Revision: D5702095
fbshipit-source-id: 3795d43
Summary: This appears to be interchangeable with `Config.debug`, so let's kill it.
Reviewed By: jvillard
Differential Revision: D5742685
fbshipit-source-id: d390b6b
Summary: This adds an option to only translate the body of a method when the file matches the give pattern. This is especially intended to be use for generated files.
Reviewed By: jvillard
Differential Revision: D5729120
fbshipit-source-id: 1e28469
Summary: With this, we can now get now get inter-procedural issues involving native methods.
Reviewed By: sblackshear
Differential Revision: D5730638
fbshipit-source-id: 3bdbdbd
Summary: Atoms of the form `identifier = footprint var` naturally occurs with the angelic analysis mode. So it is not clear to me why we should drop those.
Reviewed By: sblackshear
Differential Revision: D5654754
fbshipit-source-id: 9dd2eb5
Summary:
- failwith police: no more `failwith`. Instead, use `Logging.die`.
- Introduce the `SimpleLogging` module for dying from modules where `Logging`
cannot be used (usually because that would create a cyclic dependency).
- always log backtraces, and show backtraces on the console except for usage errors
- Also point out in the log file where the toplevel executions of infer happen
Reviewed By: jeremydubreil
Differential Revision: D5726362
fbshipit-source-id: d7a01fc
Summary: This makes the traces more readable when involving skipped functions.
Reviewed By: sblackshear
Differential Revision: D5731683
fbshipit-source-id: 49d363b
Summary: This new tests outlines that Infer does not detect inter-target issues involving native methods.
Reviewed By: sblackshear
Differential Revision: D5720873
fbshipit-source-id: cce8193
Summary: This should not affect the analysis results so there is no need to raise an exception here. We can always add this unsigned shift operator to SIL if we want the analysis to take them into account
Reviewed By: sblackshear
Differential Revision: D5703792
fbshipit-source-id: 739891f
Summary:
This approach was requiring the `InferArray` class to always be part of the classpath, and the only benefit was to preserve the length of the arrays, when known, on calls to `clone()` method. However, adding it to the models would create circular dependencies between the models, the builtins and the tests.
The code is now simpler and we can more aggressively fail when classes that are supposed to be found from the classpath are not found.
Reviewed By: sblackshear
Differential Revision: D5703173
fbshipit-source-id: 3e6cea5
Summary:
I'm working on parameterizing access trees with a config that will limit max depth + perhaps width if needed.
This is a stepping stone to enforcing max depth.
Reviewed By: jvillard
Differential Revision: D5693453
fbshipit-source-id: c15b0ee
Summary:
Rather than printing the footprint using its actual representation (an access trie with bool nodes), print it as a set of access paths.
This makes it easier to read sources in Quandary specs/debug Quandary.
Reviewed By: jeremydubreil
Differential Revision: D5682630
fbshipit-source-id: ac55b7f
Summary:
Add `is_empty` to `AbstractDomain.WithBottom` sig and use the empty checks for nicer printing of access trees: don't print empty nodes/traces.
This should make it easier to debug Quandary; it's pretty hard to stare at an access tree and see what's going on right now.
Reviewed By: jberdine
Differential Revision: D5682248
fbshipit-source-id: 56d2a9d
Summary:
This gets rid of the horrible hack of asking jbuilder to build
_build/{default,test}/.ppx/ppx_compare/ppx.exe prior to starting several
jbuilds in parallel. We now refrain from starting several jbuilds in parallel.
Also, I finally understood that order-only deps do nothing for phony targets,
and not much else for non-phony ones. I hate make even more.
Reviewed By: jberdine
Differential Revision: D5678699
fbshipit-source-id: 52179e8
Summary:
This simplifies the jbuild files: no need to list these files explicitly
anymore, nor to exclude them explicitly from the main `InferModules` library
(due to their different compilation flags).
Isolate common parts into jbuild.common do `cat`-based code inclusion into
jbuild files to factorize code.
Reviewed By: jberdine
Differential Revision: D5678328
fbshipit-source-id: 6d7d925
Summary: In case of syntax errors in AL files, stdout will contain a JSON list with all files affected by the errors, including info like filename and line number.
Reviewed By: dulmarod, jvillard
Differential Revision: D5640272
fbshipit-source-id: 569b16d
Summary:
A function can both be a sink and propagate source info, but we currently ignore the summary for any function that is also a sink.
This will cause us to under-report for (e.g.) `src1 = source(); src2 = strcpy(dest, src1); exec(src2)`.
This is both a potential buffer overflow and a potential shell injection, but we won't report the second issue.
Reviewed By: jberdine
Differential Revision: D5676167
fbshipit-source-id: 232ab2f
Summary:
We now represent the footprint with an access trie, so this code is no longer required.
This lets us simplify things a bit
Reviewed By: jberdine
Differential Revision: D5664484
fbshipit-source-id: c35edf2
Summary:
In looking at summaries that Quandary took a long time to compute, one thing I notice frequently is redundancy in the footprint sources (e.g., I might see `Footprint(x), Footprint(x.f), Footprint(x*)`).
`sudo perf top` indicates that joining big sets of sources is a major performance bottleneck, and a large number of footprint sources is surely a big part of this (since we expect the number of non-footprint sources to be small).
This diff addresses the redundancy issue by using a more complex representation for a set of sources. The "known" sources are still in a set, but the footprint sources are now represented as a set of access paths (via an access trie).
The access path trie is a minimal representation of a set of access paths, so it would represent the example above as a simple `x*`.
This should make join/widen/<= faster and improve performance
Reviewed By: jberdine
Differential Revision: D5663980
fbshipit-source-id: 9fb66f8
Summary:
The previous widening operator added stars to the *end* of paths that existed in `next` but not `prev`. This is not enough to ensure termination in the case where the trie is growing both deeper and wider at the same time.
The newly added test demonstrates this issue. In the code, there's an ever-growing path of the form `tmp.prev.next.prev.next...` that wasn't summarized by the previous widening operator. The new widening is much more aggressive: it replaces *any* node present in `next` but not `prev` with a `*` (rather than trying to tack a star onto the end). This fixes the issue.
This issue was causing divergence on tricky doubly-linked list code in prod.
Reviewed By: jeremydubreil
Differential Revision: D5665719
fbshipit-source-id: 1310a92
Summary:
Saw these two types of errors before (but they're hard to reproduce locally) when building the models:
- `ERROR: Zip.Error("/mnt/btrfs/trunk-git-infer-739-1503054473/infer/bin/../lib/java/models.jar", "", "end of central directory not found, not a ZIP file")`. I think this means infer reads a partially-written models jar. We shouldn't try to load this in models mode.
- `install` would complain that the destination already exists. I think this can only happen if there's a race and the file gets created between when install first checks and when it tries to write to it.
This made me realise that the some of the models are computed in C and C++ mode
and we pick one computed spec arbitrarily. That sounds a bit dodgy but at least
now we do so in a non-racy way.
Reviewed By: jeremydubreil
Differential Revision: D5658389
fbshipit-source-id: 8077279
Summary: Also, stop trying to delete directories that do not exist: "sources" and "filelists".
Reviewed By: mbouaziz
Differential Revision: D5658089
fbshipit-source-id: e1cdb13
Summary:
This is a check for when an unavailable class is being allocated.
This diff also adds a check for the context to remove false positives: If the class is not available but the method calls are wrapped in a check whether the class is available, then don't report.
Reviewed By: jvillard
Differential Revision: D5631191
fbshipit-source-id: 2082dfe
Summary: Other parts of the code where using the checks in the AndroidFramework module. It is better to have those things in one place.
Reviewed By: sblackshear
Differential Revision: D5654247
fbshipit-source-id: 2a783e7
Summary:
Calling exit at the end of a proc does not create unreachable code, prior to this commit inferbo reports that it does.
We extend collect_instrs to detect when we're at the end of a procedure in C and not report on unreachable code if we call a procedure there.
Reviewed By: mbouaziz
Differential Revision: D5623637
fbshipit-source-id: 0d5f326
Summary: This check is not possible in Java as it natirally happens in the totally legit case of the `try ... finally`.
Reviewed By: sblackshear
Differential Revision: D5568802
fbshipit-source-id: 24ca074
Summary:
Instead of a whitelist and blacklist and default issue types and default
blacklist and filtering, consider a simpler semantics where
1. checkers can be individually turned on or off on the command line
2. most checkers are on by default
3. `--no-filtering` turns all issue types on, but they can then be turned off again by further arguments
This provides a more flexible CLI and is similar to other options in the infer
CLI, where "global" behaviour is generally avoided.
Dynamically created checkers (eg, AL linters) cause some complications in the
implementation but I think the semantics is still clear.
Also change the name of the option to mention "issue types" instead of
"checks", since the latter can be confused with "checkers".
Reviewed By: jberdine
Differential Revision: D5583238
fbshipit-source-id: 21de476
Summary:
Every module declared but not used in the same source file is warned about
currently. Disable the noisy warning.
For instance, before this diff (and after "M-x merlin-restar-process"):
1. open AbstractInterpreter.ml
2. merlin shows a warning for `module Make` inside emacs
Reviewed By: jberdine
Differential Revision: D5621383
fbshipit-source-id: c175e5d
Summary:
Seeing rrors in "/foo/bar/src/base/infer.ml" is less distracting than seeing
errors in "/foo/bar/src/./base/infer.ml".
Reviewed By: jberdine
Differential Revision: D5621060
fbshipit-source-id: 55ee069
Summary:
The issue was as follows:
- ppx.exe depended on all the ocaml source files
- ppx.exe is an implicit prerequisit of all the (non-automatically-generated) build objects in jbuilder
- as a result, when modifying a source file, we would recompile ppx.exe, thus all the source files
ppx.exe only needs to depend on all the generated source files, so that
jbuilder can find them all.
Reviewed By: jberdine
Differential Revision: D5621007
fbshipit-source-id: 92c5b9c
Summary:
The `-index-store-path` argument does not exist in Clang 5.0 and it gets discarded without any error message. The problem is that its argument, a folder, is not discarded, and Clang considers it as a source file. This leads to the following errors:
- `cannot specify -o when generating multiple output files` (this happens if a `-o` argument is passed)
- `error reading '<PATH>'` (this can be observed when running the "normalized" version of the clang command, generated via the -### flag)
This weird case can be observed when `-index-store-path` is passed in a sequence like the following: `-x c -index-store-path <PATH> -c`.
With this change, we remove the `index-store-path` option, and its argument, from the original clang command.
Reviewed By: jvillard
Differential Revision: D5601808
fbshipit-source-id: 4200308
Summary:
This gives additional information to users. For instance:
```
--biabduction
Activates: the separation logic based bi-abduction analysis using
the checkers framework (Conversely: --no-biabduction)
This option is relevant to infer-analyze(1).
```
Reviewed By: sblackshear
Differential Revision: D5583197
fbshipit-source-id: 2960b90
Summary:
We previously lumped ownership predicates in with all other predicates. That limited us to a flat ownership domain.
This diff separates out the ownership predicates so we can have a richer lattice of predicates with each access path.
This lets us be more precise; for example, we can now show that
```
needToOwnBothParams(Obj o1, Obj o2) {
Obj alias;
if (*) { alias = o1; } else { alias = o2; }
alias.f = ... // both o1 and o2 need to be owned for this to be safe
}
void ownBothParamsOk() {
needToOwnBothParams(new Obj(), new Obj()); // ok, would have complained before
}
```
is safe.
Reviewed By: jberdine
Differential Revision: D5589898
fbshipit-source-id: 9606a46
Summary:
This makes it easier to test a single checker.
Also refactor the code to make it harder to mess up the list of default/all checkers.
Reviewed By: sblackshear
Differential Revision: D5583209
fbshipit-source-id: 7c919b2
Summary:
This makes the CLI more complete: before, it was often impossible to "go back"
once some options were passed, namely options produced by `mk_*_{list,opt}`.
Now these automatically create an accompanying `--<long>-reset` option that
resets the config variable to its default value.
Also unify our naming of `~meta` arguments:
- no more spaces in them (except one instance where it's a whole sentence)
- use `+foo` if `foo` can be specified multiple times
Reviewed By: dulmarod
Differential Revision: D5583187
fbshipit-source-id: a8c2567
Summary:
Use jbuilder to build infer instead of ocamlbuild. This is mainly to get faster builds:
```
times in 10ms, ±differences measured in speedups, 4 cores
| | ocb total | jb | ±total | ocb user | jb | ±user | ocb cpu | jb | ±cpu | ocb sys | jb | ±sys |
|-----------------------------------+-----------+------+--------+----------+------+-------+---------+-----+------+---------+------+------|
| byte from scratch | 6428 | 2456 | 2.62 | 7743 | 6662 | 1.16 | 138 | 331 | 2.40 | 1184 | 1477 | 0.80 |
| native from scratch | 9841 | 4289 | 2.29 | 9530 | 8834 | 1.08 | 110 | 245 | 2.23 | 1373 | 1712 | 0.80 |
| byte after native | 29578 | 1602 | 18.46 | 4514 | 4640 | 0.97 | 170 | 325 | 1.91 | 543 | 576 | 0.94 |
| change infer.ml byte | 344 | 282 | 1.22 | 292 | 215 | 1.36 | 96 | 99 | 1.03 | 040 | 066 | 0.61 |
| change infer.ml native | 837 | 223 | 3.75 | 789 | 174 | 4.53 | 98 | 99 | 1.01 | 036 | 47 | 0.77 |
| change Config.ml byte | 451 | 339 | 1.33 | 382 | 336 | 1.14 | 97 | 122 | 1.26 | 056 | 80 | 0.70 |
| change Config.ml native | 4024 | 1760 | 2.29 | 4585 | 4225 | 1.09 | 127 | 276 | 2.17 | 559 | 644 | 0.87 |
| change cFrontend_config.ml byte | 348 | 643 | 0.54 | 297 | 330 | 0.90 | 96 | 67 | 0.70 | 038 | 102 | 0.37 |
| change cFrontend_config.ml native | 1480 | 584 | 2.53 | 1435 | 906 | 1.58 | 106 | 185 | 1.75 | 136 | 178 | 0.76 |
#+TBLFM: $4=$2/$3;f2::$7=$5/$6;f2::$10=$9/$8;f2::$13=$11/$12;f2
50 cores
| | ocb total | jb | ±total | ocb user | jb | ±user | ocb cpu | jb | ±cpu | ocb sys | jb | ±sys |
|---------------------+-----------+------+--------+----------+------+-------+---------+----+------+---------+------+------|
| byte from scratch | 9114 | 2061 | 4.42 | 9334 | 5133 | 1.82 | | | 0/0 | 2566 | 1726 | 1.49 |
| native from scratch | 13481 | 3967 | 3.40 | 12291 | 7608 | 1.62 | | | 0/0 | 3003 | 2100 | 1.43 |
| byte after native | 3467 | 1476 | 2.35 | 5067 | 3912 | 1.30 | | | 0/0 | 971 | 801 | 1.21 |
#+TBLFM: $4=$2/$3;f2::$7=$5/$6;f2::$10=$9/$8;f2::$13=$11/$12;f2
```
Menu:
1. Write a jbuild file, autogenerated from jbuild.in because we need to fill in
some information at build-time (really, at configure time, but TODO), such as
whether or not clang is enabled.
2. Nuke lots of stuff from infer/src/Makefile that is now in the jbuild file
3. The jbuild file lives in infer/src/ so it can see all the sources. If we put it somewhere else, eg, infer/, then `jbuilder` scans too many files (all irrelevant) and takes 2.5s to start instead of .8s. Adding irrelevant directories to jbuild-ignore does not help.
4. jbuilder does not support subdirectories, so resort to listing all the
source files in the generated jbuild (only source directories need to be
manually listed in jbuild.in though). Still, the generated .merlin is wrong
and makes merlin find source files in _build, so manually tune it to get
good merlin support. We also lose some of merlin for unit tests as it
cannot see their build artefacts anymore.
5. checkCopyright gets its own jbuild because it's standalone. Also, remove
some deprecation warnings in checkCopyright due to the new version of Core from
a while ago.
6. Drop less-used Makefile features (they had regressed anyway) such as
building individual modules. Also, building mod_dep.pdf now takes all the
source files available so they better build (before, it would only take the
source files from the config, eg with or without clang) (that's pretty minor).
7. The toplevel is now built as a custom toplevel because that was easier. It
should soon be even easier: https://github.com/janestreet/jbuilder/issues/210
8. Move BUILTINS.mli to BUILTINS.ml because jbuilder is not happy about
interface files without implementations.
In particular, I did not try to migrate too much of the Makefile logic to jbuilder,
more can be done in the future.
Reviewed By: jberdine
Differential Revision: D5573661
fbshipit-source-id: 4ca6d8f
Summary:
The only path to the `MergeCaptured.slink` function is when we should merge, so
it doesn't make sense to check `Config.merge` again. In the case of `infer run
--flavors -- buck ...`, this would create regular symlinks instead of the much
faster multilinks.
Reviewed By: jberdine
Differential Revision: D5574020
fbshipit-source-id: df710ca
Summary:
Fix for the #669. Recursion problem has been reproduced (see the new doctest in the latest commit) and eliminated. I guess the new version will work faster too (no more unnecessary slicing).
Closes https://github.com/facebook/infer/pull/697
Reviewed By: sblackshear
Differential Revision: D5507925
Pulled By: jvillard
fbshipit-source-id: 21fa07b
Summary:
This is a needed step in the direction of making prenalysis functional: it will return a view of the CFG rather than mutating the CFG.
ProcCfg already works by providing a view on the underyling CFG, but the bi-abduction can't leverage this because it uses the "raw" CFG.
This diff does a partial swap of the raw CFG for an exceptional ProcCfg. The goal is to make sure the bi-abduction never calls `Procdesc.get_instrs`; it should use the `ProcCfg` wrapper instead.
That way, preanalyses that add instructions (like the liveness prenalysis) will work.
There's still some calls to `Procdesc.get_succs` etc., but we can remove those in a future diff.
They're not on the critical path because the current preanalyses only add instructions, not nodes or edges.
Reviewed By: jeremydubreil
Differential Revision: D5556387
fbshipit-source-id: 4ffda00
Summary:
Previous version was hard to understand because it was doing many things within same code. New version has different code for Arrays, Structs and others.
There is some copy-paste, but it's easier to follow code (open to suggestions though)
Reviewed By: dulmarod
Differential Revision: D5547999
fbshipit-source-id: 77ecb24
Summary: It wasn't using code from `std::vector::empty` which recently was improved. Instead of inlining `std::vector::empty`, call it to know whether vector is empty or not.
Reviewed By: jvillard
Differential Revision: D5573379
fbshipit-source-id: e024a42
Summary: Useful for identifying user-controlled array accesses that could lead to buffer overflows
Reviewed By: mbouaziz
Differential Revision: D5520985
fbshipit-source-id: 92984f6
Summary:
When a sink name is specified in `.inferconfig` or in OCaml, it might conflict with a function of the same name that has a different number of args.
We shouldn't try to create a sink in this case, and we definitely shouldn't crash.
Reviewed By: jeremydubreil
Differential Revision: D5561216
fbshipit-source-id: fa1859b
Summary: This is unused, as far as I can tell. If we want to revive it, we can do it in AL or as a simple checker; it certainly doesn't require the full might of bi-abduction.
Reviewed By: jeremydubreil
Differential Revision: D5556325
fbshipit-source-id: e3895c2
Summary:
Replace `inferTraceBugs` with `infer-explore` with a similar CLI. Some options changed:
- --max-level -> --max-nesting, and "max" is the default value instead of a possible value
- --no-source -> --no-source-preview
Reviewed By: mbouaziz
Differential Revision: D5526651
fbshipit-source-id: 8383f37
Summary:
In some cases we normalize expressions to check some facts about them. In these
cases, trying to keep as much information as possible in the expression, such
as the fact it comes from a `sizeof()` expression, is not needed. Doing
destructive normalization allows us to replace `sizeof()` by its
statically-known value.
closes#706
Reviewed By: mbouaziz
Differential Revision: D5536685
fbshipit-source-id: cc3d731
Summary: A temporary workaround until we can understand why this happens and fix it.
Reviewed By: jeremydubreil
Differential Revision: D5559838
fbshipit-source-id: dc86eb9
Summary: That was too noisy. Propagate `--quiet` to the Python reporting hook so as to still emit bugs.txt and so on.
Reviewed By: martinoluca
Differential Revision: D5501106
fbshipit-source-id: 63b6451
Summary:
This allows the user to specify a different command to build the current version of the project vs to build the previous version of the project. Here's an example:
```
infer diff --gen-previous-build-command-script "echo clang -c hello.c" ... -- clang -c hello2.c
```
By default the two build commands are the same. If the script is to be used for both the current and the previous versions, then it's on the user to run it once first, eg:
```
infer diff --gen-previous-build-command-script "./myscript.sh previous" ... -- $(./myscript.sh current)
```
Reviewed By: martinoluca
Differential Revision: D5500989
fbshipit-source-id: 7374b44
Summary: Working on making CFG immutable by killing refs to functions that mutate them. This is an easy one because it doesn't do anything :).
Reviewed By: jeremydubreil, mbouaziz
Differential Revision: D5551521
fbshipit-source-id: bec76a9
Summary:
Record the list of access paths (if any) used in the index expression for each array access.
This will make it possible to use array accesses as sinks in Quandary
Reviewed By: jeremydubreil
Differential Revision: D5531356
fbshipit-source-id: 8204909
Sam Blackshear
Jules Villard
Dulma Churchill
Jeremy Dubreil
Anders Papitto
Josh Berdine
Martino Luca
Mehdi Bouaziz
Dino Distefano
Sungkeun Cho
Daiva Naudziuniene
Kyriakos Nikolaos Gkorogiannis
Roy Li
Greg Nisbet
Jia Chen
Meng Zhang
Andrzej Kotulski
Alexander Slesarev