Summary:
Adds a new type and branching for a missing path of execution.
closes#575
Reviewed By: jvillard
Differential Revision: D4738681
fbshipit-source-id: f72344c
Summary:
Add support for Makefiles to the copyright linter. Makefiles are a bit
different than shell because they should start with the copyright notice
straight away (whereas shell starts with the #! stuff).
Reviewed By: mbouaziz
Differential Revision: D4786620
fbshipit-source-id: 504dc23
Summary:
Nuking the specs then building the models was not a great idea: the models do
not look at specs but only at some dummy marker files, eg
infer/lib/specs/c_models, so they don't necessarily realize that they need to
be rebuilt when the specs have been nuked!
One easy workaround would be to also delete the marker files, but then we would
*always* rebuild the models when building infer. Not good.
The solution here is to nuke the specs and marker files only when the clang
dependencies change, then rebuild all the models.
Reviewed By: jberdine, jeremydubreil
Differential Revision: D4781424
fbshipit-source-id: 2d2606e
Summary:
We shouldn't install byte-executables unless the user types `make byte`.
Othewise everything gets very slow and the cause is not obvious.
Reviewed By: jeremydubreil
Differential Revision: D4779135
fbshipit-source-id: 757bfa2
Summary:
It can be useful when debugging infer or the Makefiles themselves to see what
`make` is doing. Instead of editing Makefiles to remove `@` now you can `make
VERBOSE=1`.
This is just `git ls-files | grep -e Makefile -e '.*\.make' | xargs sed -e 's/^\t@/\t$(QUIET)/' -i`, and adding the definition of `QUIET` to Makefile.config.
Reviewed By: jeremydubreil
Differential Revision: D4779115
fbshipit-source-id: e6e4642
Summary:
No new functionality here; mostly `FN_` tests documenting our current limitations.
Will start chipping away at the false negatives in follow-up diffs.
Reviewed By: peterogithub
Differential Revision: D4780013
fbshipit-source-id: 7a0c821
Summary: Bringing the logic back to where it was before the big refactoring of the reporting logic.
Reviewed By: peterogithub
Differential Revision: D4774541
fbshipit-source-id: afeaaf8
Summary: We only need one "global" view of all the summaries in a file.
Reviewed By: peterogithub
Differential Revision: D4773646
fbshipit-source-id: 29e5316
Summary:
Move all of the reporting on top of the aggregation functionality.
This lets us delete lots of code
Reviewed By: peterogithub
Differential Revision: D4772223
fbshipit-source-id: 47cc51a
Summary:
This was the one type of races we were not yet reporting (besides ones that use the wrong synchronization :)).
Wrote new utility function to aggregate all accesses by the memory they access.
This makes it easy to say which accesses we should report and what their conflicts are.
Eventually, we can simplify the reporting of other kinds of unsafe accesses using this structure.
Reviewed By: peterogithub
Differential Revision: D4770542
fbshipit-source-id: 96d948e
Summary:
We can simplify the code now that the procedure callback are always executed through Ondemand. The procedure callback is still registered for Ondemand analysis by the time we run the cluster callbacks. This allows to run allows to run `Summary.read_summary`, which may run the analysis on-demand, while collecting the summaries for reporting errors.
This allows further simplifications of the Ondemand API.
Reviewed By: sblackshear
Differential Revision: D4764251
fbshipit-source-id: d0bdda4
Summary: This was annoying as "jump to next error" was otherwise always jumping to this warning about shadowing `|>`
Reviewed By: sblackshear
Differential Revision: D4767571
fbshipit-source-id: 932145c
Summary:
For collections whose type does not express that the collection is thread-safe (e.g., `Collections.syncrhonizedMap` and friends).
If you annotate a field holding one of these collections, we won't warn when you mutate the collection.
Reviewed By: jeremydubreil
Differential Revision: D4763565
fbshipit-source-id: 58b487a
Summary: Writing the summaries to disk now happens automatically during the on-demand analysis calls. So, individual checkers do not need to worry anymore about when should the analysis summaries be written to disk for the interprocedural analysis to do the right thing.
Reviewed By: sblackshear
Differential Revision: D4764337
fbshipit-source-id: 63870db
Summary: It seems that we were not really using the `Bottom` part of the domain as a pair of (empty call map, empty tracking var map) was already acting as bottom.
Reviewed By: sblackshear
Differential Revision: D4759757
fbshipit-source-id: 53dedfe
Summary:
Don't pass names as strings in clang frontend. Instead use QualifiedCppName which preserves
each identifier of qualified name.
Done by
1. change return type of `Cast_utils.get_qualified_name` to return `QualifiedCppName.t`
2. change types in `Typ.Name.t` and `Typ.Procname.t` to use qualified names where applicable
3. Keep changing the code until it compiles
Reviewed By: jberdine
Differential Revision: D4754242
fbshipit-source-id: 9d723cb
Summary: This call is redundant and is already done in `AbstractInterpreter`
Reviewed By: sblackshear
Differential Revision: D4754251
fbshipit-source-id: af2d11e
Summary:
Most of our tests work by comparing the output of some command to a baseline
expected output. It's often needed to update that baseline.
Previously, that was crudely done by attempting to move every foo.exp.test file
to foo.exp. This does not work terribly well, in particular because
foo.exp.test might be stale.
Instead, add a `replace` target to every test that knows how to update the
baseline. This allows custom behaviours too, eg in the differential tests.
Most of the tests include base.make or differential.make, so add a replace target
there. A few tests are completely custom, add a replace target to them too.
Reviewed By: jeremydubreil
Differential Revision: D4754279
fbshipit-source-id: ec34273
Summary:
This issue was spotted in the wild. There may be more of those, unfortunately it's hard to predict
More general problem is that types in infer frontend diverge from clang's types for DerivedToBase cast.
Then, infer uses types from clang anyway and that confuses backend. Getting it always right is very hard
Reviewed By: jvillard
Differential Revision: D4754081
fbshipit-source-id: 5fb7069
Summary:
If I read off the main thread and write on the main we
could have a race. (Writes off main are already reported.)
Reviewed By: sblackshear
Differential Revision: D4746138
fbshipit-source-id: 8b6e9c5
Summary:
Improve type of `Fieldname.t` in `Clang` variant - make it store qualified classname and method name.
Based on those changes, fix matching in `Errdesc` to use `QualifiedCppName.Match` instead of string comparisons
Reviewed By: jberdine
Differential Revision: D4746735
fbshipit-source-id: 6f52413
Summary:
Split Fieldname.t into `Java` and `Clang`. Each of them have different naming conventions and this way it's easier to differentiate between them.
Make `Java` variant store string instead of mangled since mangled part was always empty
Changes to `Clang` variant are coming in the next diff
Reviewed By: jeremydubreil
Differential Revision: D4746708
fbshipit-source-id: c5858a8
Summary:
Reorganize by using a top-level iteration over the access map and using a helper function for updating the caller accesses.
The new code is shorter and much more readable.
Reviewed By: peterogithub
Differential Revision: D4740657
fbshipit-source-id: 8e18cd5
Summary: Add `QualifiedCppName.t` and some functions to manipulate it. More places will start using this type (such as `Procnames` or `Typ.Name`) in later diff
Reviewed By: jberdine
Differential Revision: D4738991
fbshipit-source-id: 8f20dd6
Summary:
All tests were redirecting `stderr` into duplicates.txt which made it much harder to see other error messages in stderr (such as uncaught exceptions).
To mitigate it, write duplicates to separate file and don't redirect `stderr` to another file.
Reviewed By: jvillard
Differential Revision: D4728938
fbshipit-source-id: 8ad2fc8
Summary: There was a lot of indirection going on in `Typ.Name` type definition. Inline all those indirections into single variant type
Reviewed By: jberdine
Differential Revision: D4737644
fbshipit-source-id: c5e181b
Summary: Now that all the checkers are now run in a way that will prevent conflicts between them, we can make this change that was breaking the analysis.
Reviewed By: jvillard
Differential Revision: D4621953
fbshipit-source-id: f17c729
Summary:
One limitation of Eradicate is that certain nullability patterns are not expressible using simply the `Nullable` annotation.
One such pattern is using the knowledge that a function returns null when passed null, but returns an object otherwise.
The annotation `PropagatesNullable` is a variant of `Nullable` applied to parameters when their value propagates to the return value.
A method annotated
```
B m(PropagatesNullable A x) { return x == null ? x : B(x); }
```
indicates that `m` returns null if `x` is null, or an object of class `B` if the argument is not null.
Examples with multiple parameters are in the test cases.
This diff builds some infrastructure for annotation transformers: the example above represents the identity function on nullability annotations.
Reviewed By: jvillard
Differential Revision: D4705938
fbshipit-source-id: 9f6194e
Summary:
Before, `trace_of_pname` only grabbed unprotected writes from the summary, so the traces ending in an unprotected read were truncated.
We now look at reads too when appropriate.
Reviewed By: peterogithub
Differential Revision: D4719740
fbshipit-source-id: 28f6e63
Summary: That tuple has 3 elements already, there may be 4th element coming.
Reviewed By: mbouaziz
Differential Revision: D4721342
fbshipit-source-id: cba44ef
Summary:
This removes the dependency of libffi, and gives better guarantees at
compile-time. A bit overkill to retrieve the width of the terminal but what do
you know...
Reviewed By: jberdine
Differential Revision: D4700003
fbshipit-source-id: 036989b
Summary:
Two issues in the previous code:
1. We'd just tack on `#compilation-database` so if there already are `#flavors`
in a Buck target then we get a malformed `#flavors#compilation-database`
instead of `#flavors,compilation-database`. There's already code to deal
with that for other flavors (`#infer-capture-all`), so extend and reuse that.
2. The code didn't work if there are spaces in target names (sigh). Harden the
code a bit so that it works. Unfortunately Buck doesn't escape spaces at all
in its `buck targets --show-output` output, so the parsing still relies on
there being no spaces in flavors to work correctly. Sample output:
```
$ buck targets --show-full-output //clang_compilation_database:Hel\ lo#compilation-database
Not using buckd because watchman isn't installed.
[+] PROCESSING BUCK FILES...0.1s [100%] 🐳 New buck daemon
//clang_compilation_database:Hel lo#compilation-database /home/jul/infer/infer/tests/build_systems/codetoanalyze/buck-out/gen/clang_compilation_database/__Hel lo#compilation-database.json
```
Also, some code in codetoanalyze/clang-compilation-database/ didn't compile, so
fix that.
Reviewed By: dulmarod
Differential Revision: D4714338
fbshipit-source-id: b8ae324
Summary:
These are mentioned on fbinfer.com so IMHO should be displayed when running infer -h
Closes https://github.com/facebook/infer/pull/617
Differential Revision: D4697682
Pulled By: jvillard
fbshipit-source-id: 3d7dd7b
Summary:
It is definitely useful to collect information about how long the analysis of every procedure takes. It allows to detect and focus on outliers when trying to improve performance. However, this kind of information could be collected using a standard logging mechanism and does not need to be stored within the analysis artifacts.
I intend to add some form of similar logging in the context of #16348004 once we can get every analysis procedure analyzed through the `Ondemand` module. In this case, it would be easy to have a single place to log how does the analysis of a procedure take.
Reviewed By: jberdine
Differential Revision: D4636755
fbshipit-source-id: 01f3bca
Summary: Now, running `infer -a checkers -- ...` will also run the ThreadSafety checker
Reviewed By: sblackshear
Differential Revision: D4691330
fbshipit-source-id: 04fc781
Summary: Fail early when there is no registered callbacks to run the analysis of a procedure on-demand
Reviewed By: sblackshear
Differential Revision: D4573726
fbshipit-source-id: a8ee74b
Summary: Run all the checkers one after each other, which allows the Infer AI framework to run several checkers together, including the possibility for them to collaborate.
Reviewed By: sblackshear
Differential Revision: D4621838
fbshipit-source-id: e264d67
Summary:
This option is not needed anymore as it was introduced to counter an uncovered
perf issue with creating human readable reports. The perf issue has been
addressed.
Instead of this option, one can use `infer --report-hook /bin/true ...` to
disable reporting. However, right now the Buck integration doesn't honor it so
this would need to be fixed to be a true equivalent of `--disable-bug-list`.
Reviewed By: jberdine
Differential Revision: D4712877
fbshipit-source-id: a09304f
Summary: This is dead code, and depends on libffi which we could get rid of otherwise.
Reviewed By: jberdine
Differential Revision: D4705645
fbshipit-source-id: efd28ef
Summary:
This makes sure that one can run `./build-infer.sh` then `make`. Otherwise it's
not always clear what one should do to recompile infer, eg when `make` will
work and when `./build-infer.sh` should be used instead, in particular when the
user doesn't have opam configured for her terminal.
Reviewed By: jberdine
Differential Revision: D4698159
fbshipit-source-id: 5df8059
Summary:
Changes every checker to take a summary as parameter and return the updated summary to the next checker. Since several operations, like `Reporting.log_*` are modifying the summary in memory by loading them from the in-memory cache of summaries, we currently need to rely on `Specs.get_summary_unsafe` to return the updated version of the summary.
This diff allows to change the API of `Reporting` to take a summary as input and progressively remove all the calls `Specs.get_summary_unsafe` independently from adding the possibility to run several checkers at the same time. The final objective to have every checker just passing around the summary of the procedure being analyzed, and having the in-memory cache only use to store the summaries of the callees.
Reviewed By: sblackshear
Differential Revision: D4649252
fbshipit-source-id: 98f7ca7
Summary:
We were including hex of empty string if mangled name was not empty (so for all C++ functions).
Instead, include hex of a source file only if it's not empty
Reviewed By: mbouaziz
Differential Revision: D4705388
fbshipit-source-id: 55b6587
Summary:
Procnames files are now reversed qualifier lists with `#` as separator (instead of `::` which needs to be escaped in bash).
Because of the mechanism that is used to obtain qualifiers, it also affects naming for ObjC classes.
Examples:
```
std::unique_ptr<int>::get -> get#unique_ptr<int>#std#__MANGLED,...__ // C++ method
folly::split -> split#folly#__MANGLED,..._ // function within namespace
NSNumber numberWithBool: -> numberWithBool:#NSNumber#class // ObjC method
```
Reviewed By: jvillard
Differential Revision: D4689701
fbshipit-source-id: c3acfc6
Summary:
We want to stop supporting '-multiletter' options altogether in the future as
they are not standard.
For boolean switches, we used to have '-short' for true and '-nshort' for
false, but that second form is necessarily > 1 letters, which doesn't work in
this scheme. Replace these by '-s' for true and '-S' for false.
Reviewed By: jberdine
Differential Revision: D4674079
fbshipit-source-id: 95bacfe
Summary:
Add a new command-line option `--per-procedure-parallelism`, to change the granularity of parallelism of the analysis from file to procedure.
This is intended for `--reactive` mode where e.g. a single file is changed and the analysis currently uses just one core.
When the option is used, the Makefile mechanism is replaced by using forking instead.
The parent process does as little allocation as possible, to avoid taxing the kernel.
Caveats:
- Not active in Java, (issues with camlzip).
- Not active in checkers, yet.
Example use:
```
infer --reactive --changed-files-index index.txt --per-procedure-parallelism -- analyze
```
Reviewed By: jberdine
Differential Revision: D4634884
fbshipit-source-id: e358c18
Summary:
There is no point in attaching plugin to `clang -E` calls since they don't produce any AST.
Same applies to `-M`, `-MM`, etc. (but not `-MD` nor `-MMD`).
Reviewed By: jvillard
Differential Revision: D4681618
fbshipit-source-id: 7a76add
Summary:
There was a race in serialization code where a reader could try to deserialize a partially overwritten value.
Here we use a `file.lock` to synchronize the writers, while the readers are not synchronized.
The atomic `rename` is used to move `file.lock` to `file` and make it available to readers.
Since `rename` is atomic, readers should see possibly stale, but not corrupted, data.
Reviewed By: sblackshear
Differential Revision: D4689664
fbshipit-source-id: dc5b546
Summary:
Read template arguments from the AST and if argument is a type, translate it into Sil type. Then, save this information into procname.
There is one ugly change - linters now may populate tenv (it's not saved anytwhere though)
Reviewed By: dulmarod
Differential Revision: D4635508
fbshipit-source-id: e806ca7
Summary:
When both an unprotected write and a read/write race emanate from the same line,
undoubtedly because of interprocedurality, strip the read/write report (for now).
Perhaps report the info in more succinct form later, but keep to one report/line.
Reviewed By: sblackshear
Differential Revision: D4685102
fbshipit-source-id: 291cf20
Summary:
There was a bug where we allowed ourselves to project local variables from the callee summary into an access path in the caller.
We should only be able to project callee variables that are in the footprint.
Reviewed By: jeremydubreil
Differential Revision: D4684868
fbshipit-source-id: 53a2b9d
Summary: This cleans up the domain/transfer functions, and it also means that we can now track reads that occur under synchronization.
Reviewed By: peterogithub
Differential Revision: D4674243
fbshipit-source-id: 8e13656
Summary: I noticed we don't have `T` or `S` to denote timeouts in debug mode anymore. Today I saw it's still in `--stats` mode. Bring this feature back to `--debug` as well.
Reviewed By: cristianoc
Differential Revision: D4681669
fbshipit-source-id: 16ef19b
Summary:
When parsing .inferconfig and the env var, allow all modes of infer. This
allows people to provide default values for options in .inferconfig. This
requires subcommands' options to be all distinct, but that's already the case.
Reviewed By: jberdine
Differential Revision: D4674672
fbshipit-source-id: 992b454
Summary:
Updated version of the plugin exports some missing `VarDecls`. To make sure it doesn't break again,
add a test that didn't work before.
Reviewed By: dulmarod
Differential Revision: D4674123
fbshipit-source-id: 0c1677a
Summary:
Provide proper constructor functions for all Typenames following `Typename.Java` module.
Always use those constructor functions.
Reviewed By: jeremydubreil
Differential Revision: D4673943
fbshipit-source-id: 81625c2
Summary: This allows to run the analysis of every procedure on-demand separately from the cluster callbacks
Reviewed By: sblackshear
Differential Revision: D4664936
fbshipit-source-id: d218328
Summary: On Java Buck projects, InferPrint was loading all the specs files from all the jars in the classpath. This was affecting the performance a lot when the analysis was reporting a lot of issues.
Reviewed By: cristianoc
Differential Revision: D4673226
fbshipit-source-id: 6927836
Summary:
It used to be string which:
1. Doesn't have enough information for parametric models
2. Doesn't have good type
Changing this blows up in clang frontend, but I think it's for the better
Reviewed By: jberdine
Differential Revision: D4667633
fbshipit-source-id: 9f61bf1
Summary:
Eradicate detects circular field initializations (e.g. a field initialized with itself)
by checking in the typestate at the end of the constructor whether the origin
of the field is a field name in the current class.
This has the problem that the following initialization pattern is not recognized as correct:
C(C x) { this.field = x.field }
To fix the issue, the origin information for field accesses x.f is extended
with the origin information of the inner object x.
Circularities are detected if the origin of x is "this".
Reviewed By: jberdine
Differential Revision: D4672472
fbshipit-source-id: 9277bdd
Summary:
Turns out that we were special-casing the `Infer Driver` case for no particular
reason and that was preventing `infer --diff --help` from working as expected.
Reviewed By: martinoluca
Differential Revision: D4666988
fbshipit-source-id: 0868d4b
Summary: `CContext.curr_class` contained information about a class for a method as a string, while it's better to use pointer for this.
Reviewed By: jvillard
Differential Revision: D4666613
fbshipit-source-id: 1c0735b
Summary: Previously, we wouldn't report races where the write was under synchronization.
Reviewed By: peterogithub
Differential Revision: D4658850
fbshipit-source-id: e9f4c41
Summary: I encountered cases where the class name part of the method name was passed as `(None, "package.Class")` instead of `("package", "Class")` and therefore incorrectly failing some inequality checks
Reviewed By: sblackshear
Differential Revision: D4662617
fbshipit-source-id: 98ee3e3
Summary:
Given two analysis results, it's now possible to compare them with the following command:
infer --diff --report-current reportA.json --report-previous reportB.json --file-renamings file_renamings.json
this command will then generate 3 files in `infer-out/differential/{introduced, fixed, preexisting}.json`, whose meaning is the following:
- `introduced.json` has all issues in `current` that are not in `previous`
- `fixed.json` has all issues in `previous` that are not in `current`
- `preexisting.json` has all issues that are in both `current` and `previous`
The json files generated can then be used to categorise results coming from incremental analyses of a codebase.
Reviewed By: jvillard
Differential Revision: D4482517
fbshipit-source-id: 1f7df3e
Summary: Make backend aware of some template instantiation arguments for template classes.
Reviewed By: jberdine
Differential Revision: D4421338
fbshipit-source-id: f7d72b4
Summary:
In order to be able to report races like
```
synchronized write() {
this.f = ...
}
read() {
return this.f;
}
```
, we need to track writes that happen inside of synchronization as well as writes that happen outside of synchronization.
This diff takes a step toward making that possible by defining an "AccessDomain" mapping a precondition for the safety of a write ( {Safe, SafeIf i, Unsafe} =~ {true, owned(i), false} ) to a set of writes that are safe if the precondition will hold.
We're not actually tracking safe writes yet, but this domain will make it easy to do so.
This also lets us kill the conditional writes/unconditional writes combo, which was a bit clumsy
Reviewed By: peterogithub
Differential Revision: D4620153
fbshipit-source-id: 2d9c5ef
Summary:
Make sure `inferTraceBugs` works with non-ascii characters. Harden the code a
bit more on the way.
Fixes#592
Reviewed By: mbouaziz
Differential Revision: D4659016
fbshipit-source-id: 79f7a80
Summary: The implementation of `touch_start_file` was not updating the timestamp when the file exists.
Reviewed By: jvillard
Differential Revision: D4657708
fbshipit-source-id: 0a88ebc
Summary: I accidentally save a summary with the wrong procedure name, which was affecting the analysis in some weird way. This makes this case no longer possible
Reviewed By: cristianoc
Differential Revision: D4654002
fbshipit-source-id: 9fcbe4e
Summary: This function was actually doing the same as `Idenv.create`.
Reviewed By: cristianoc
Differential Revision: D4654241
fbshipit-source-id: 87c098b
Summary:
This is helpful to make sure tests are up to date wrt the models.
Also made the Java deps depend on the models.jar instead of the model sources
as that's what the tests will be using. In particular, updating the sources of
the models will not update the results of a test unless someone rebuilds
models.jar, so rerunning the tests when the models haven't been rebuilt is
useless.
Reviewed By: akotulski
Differential Revision: D4635129
fbshipit-source-id: 75b4ab6
Dino Distefano
Paco Estevez Garcia
Jules Villard
Andrzej Kotulski
Sam Blackshear
Jeremy Dubreil
Peter O'Hearn
Dulma Churchill
Mehdi Bouaziz
Martino Luca
Cristiano Calcagno
Kihong Heo
Matthew Haughton
Daniel Wagner-Hall
Ben Hudson