Summary: The issues that are not reported by default are all experimental issues from the biabduction analysis. In that case, it is easier to use a blacklist of errors to filter out so that the issues found by the checkers based on the AI framework can be reported by default without having to add them to the whitelist.
Reviewed By: jvillard
Differential Revision: D5051327
fbshipit-source-id: 2a93b11
Summary:
The code was pretty fragile, it's less fragile now. We should probably just
delete that and start outputting proper class/package info directly in the
report.json instead of reverse-engineering them.
Fixes#640
Reviewed By: jeremydubreil
Differential Revision: D4905973
fbshipit-source-id: 1590067
Summary:
An array has a static or dynamic length (number of elements), but it also has a
stride, determined by the type of the element: `sizeof(element_type)`. We don't
have a good `sizeof()` function available on SIL types, so record that stride
in the array type.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D4969697
fbshipit-source-id: 98e0670
Summary: In particular, the heuristics for propagating taint via unknown code needs to be aware of the frontend's trick of introducing dummy return variables.
Reviewed By: mbouaziz
Differential Revision: D5046345
fbshipit-source-id: da87665
Summary:
HIL had only been tested in Java, and it made some assumptions about what array expressions look like (the LHS is always resolvable to an access path) and assignments (the LHS is always an access path) that aren't true in C.
Fixed the code so we won't crash in this case.
Thanks to jeremydubreil for catching this.
Reviewed By: jeremydubreil
Differential Revision: D5047649
fbshipit-source-id: e8484f4
Summary:
There are two pointer-related operations you can do in C++ but not Java that we need to support in taint analysis:
(1) `*formal_ptr = ...` when `formal_ptr` is a formal that's a pointer type. Java doesn't have raw pointers, so we didn't need to handle this case.
(2) Passing by reference, which Java also doesn't have (everything is pass-by-value).
Reviewed By: mbouaziz
Differential Revision: D5041246
fbshipit-source-id: 4e8f962
Summary:
Introduce `infer-<command>` for each command, except for internal commands
(only `infer-clang` for now) which are not exported. Install these executables
(which are just symlinks to `infer`) on `make install`. The main executable
looks at the name it was invoked with to figure out if it should behave as a
particular command.
Get rid of `InferClang`, `InferAnalyze`, and `InferPrint`. As a bonus, we now
only need to build one executable: `infer`, which should be a few seconds
faster (less link time).
`InferAnalyze` is now `infer-analyze` and `InferPrint` is `infer-print`. To run
`InferClang`, use a symlink named `clang`, `clang++`, etc. to `infer`. There
are such symlinks available in "infer/lib/wrappers/" already.
I also noticed that the scripts in xcodebuild_wrappers/ don't seem useful
anymore, so use wrappers/ instead, as for `make`.
Reviewed By: mbouaziz
Differential Revision: D5036495
fbshipit-source-id: 4a90030
Summary:
`infer clang ...` was not being handled correctly and would error incorrectly.
Small fix before bigger change.
Reviewed By: mbouaziz
Differential Revision: D5036447
fbshipit-source-id: 87fcc40
Summary: It's useful to have all the options in a single man page for discoverability.
Reviewed By: mbouaziz
Differential Revision: D5028007
fbshipit-source-id: 37f8d2e
Summary:
Allows one to dump the manuals in various formats:
- plain: plain text
- pager: default, display the man page in all its glory inside a pager
- groff: the source code of the man page
- auto: pager or plain
Reviewed By: mbouaziz
Differential Revision: D5027636
fbshipit-source-id: 3b97edc
Summary:
Glorious, glorious man pages.
This changes a bunch of things that were hard to break up from the diff, so the
resulting diff is big.
Use `Cmdliner.Manpage` to format man pages (and also format a bit ourselves
since it is so stubborn).
As a bonus, introduce the following subcommands:
```
infer run ... # same as default mode with -- before
infer capture ... # -a capture
infer compile ... # -a compile
infer analyze ... # InferAnalyze
infer report ... # InferPrint
infer diff ... # this one is not new
infer clang ... # InferClang, not that you should use it
```
The man pages can still be improved a lot. Notable missing sections:
`ENVIRONMENT`, stuff about .inferconfig, some example usage, `DESCRIPTION`, ...
Reviewed By: jberdine
Differential Revision: D4921083
fbshipit-source-id: 9602230
Summary:
With this change, running the biabduction analysis with
infer -a infer -- ...
or with:
infer -a checkers --biabduction -- ...
take the same time and give the same list of results.
Reviewed By: sblackshear
Differential Revision: D5026676
fbshipit-source-id: ef23911
Summary: Same as D5026082, but allowing specification in JSON rather than harcoded in Infer.
Reviewed By: jeremydubreil
Differential Revision: D5030042
fbshipit-source-id: 8a6cfee
Summary:
In the case of the Buck integration for Java, the summary of the procedure may be found from the classpath even though the procedure description is not available.
Depends on D5027049
Reviewed By: jvillard
Differential Revision: D5027188
fbshipit-source-id: b1a6095
Summary: The procedure description is available when initializing the analysis summary, so it is simpler to use it than to rely on loading the data from the attributes.
Reviewed By: jvillard
Differential Revision: D5027049
fbshipit-source-id: 92cac5c
Summary: A lot of C++ library functions look like this, so it's important to have.
Reviewed By: mbouaziz
Differential Revision: D5026082
fbshipit-source-id: 6f421b6
Summary: Stops Quandary errors from getting dropped on the floor when it runs alongside the other checkers.
Reviewed By: jeremydubreil
Differential Revision: D5010801
fbshipit-source-id: 2847f61
Summary: Making sure simple passthroughs like the identity function work in C++.
Reviewed By: mbouaziz
Differential Revision: D5024031
fbshipit-source-id: ce48ead
Summary: This actually fixes issues of infinite loop as the function `Specs.set_status` was saving the `Active` status in the summary from the specs table which could differ from the summary passed as argument to the checkers
Reviewed By: sblackshear
Differential Revision: D5025923
fbshipit-source-id: c23a6f9
Summary:
Now,
infer -a infer -- ...
and
infer -a checkers --biabduction -- ...
will return the same list of errors
Reviewed By: sblackshear
Differential Revision: D5023223
fbshipit-source-id: f52ce5d
Summary:
Needed because this is how the Clang frontend translates returns of non-POD, non pointer values (I think)?
Will handle the more general case of pass by reference soon.
Reviewed By: jvillard
Differential Revision: D5017653
fbshipit-source-id: 1fbcea5
Summary:
Ran the build with -w,-32 , delete code, repeat, until a fixpoint of no more warnings is reach.
Unfortunately we cannot fatal on w32 because ppx_compare can generate dead code (eg `compare_t` and only `compare` is used).
Reviewed By: mbouaziz
Differential Revision: D4945800
fbshipit-source-id: c95afb6
Summary:
`[a, b] < [a, _]` and `[_, a] < [b, a]` are most probably false (it comes from size < size)
Mark definitely unsatisfied conditions as B1, others as B2+
Reviewed By: KihongHeo, jvillard
Differential Revision: D4962107
fbshipit-source-id: ba8f469
Summary:
The bufferoverrun checkers can now be run with:
infer -a checkers --bufferoverrun -- ...
Reviewed By: jeremydubreil
Differential Revision: D5010689
fbshipit-source-id: 2eaa396
Summary:
The Siof checkers can now be run with:
infer -a checkers --siof -- ...
and also runs by default using:
infer -a checkers -- ...
Reviewed By: jberdine
Differential Revision: D5009731
fbshipit-source-id: e0e2168
Summary: Pass the classpath infromation from the javac command line option file to allow the dection of errors accros Buck targets
Reviewed By: sblackshear
Differential Revision: D5018329
fbshipit-source-id: cf13198
Summary:
First step to be able to enable and disable the checkers to run in the following form:
> infer -a checkers --checker1 --checker2 --checker3 -- ...
and have a predefined list of checkers that are run by default with:
> infer -a checkers -- ...
Reviewed By: sblackshear
Differential Revision: D5007377
fbshipit-source-id: d7339ef
Summary:
While working on making the AI framework simpler to use, it become hard to change the shared API while keeping these unused checkers compiling, and even harder to keep their functionality since there is no tests for them.
Also, these checkers are not useful as proof of concept since using the AI framework is the preferred way to write new kinds of analysis now.
Reviewed By: sblackshear
Differential Revision: D4999387
fbshipit-source-id: 497284b
Summary:
Bufferoverrun-specific model for std::vector
Requires `--bufferoverrun` command line flag
Reviewed By: akotulski
Differential Revision: D4962136
fbshipit-source-id: f6b5f15
Summary:
This gives the option to run the biabduction analysis together with the other Clang-based checkers with the command:
infer -a checkers --biabduction -- ...
The filtering does not work yet because the filtering for the biabduction analysis matches the analyzer `Infer`, and does not filter much when the analyzer is `Checkers`, which is the case here.
Reviewed By: sblackshear
Differential Revision: D4773834
fbshipit-source-id: 16300cc
Summary:
`Location.dummy` is often used in a situation where we know the source file, but not the line/column.
Use `Location.none` for this instead.
Reviewed By: jeremydubreil
Differential Revision: D4991232
fbshipit-source-id: fc361a4
Summary:
Last step for converting thread-safety and quandary to HIL.
Push the logic for managing the id map and converting the instructions into a functor.
This way, client analyses can simply write HIL transfer functions and call the functor.
Reviewed By: jberdine
Differential Revision: D4989987
fbshipit-source-id: 485169e
Summary:
For now we just want to find bugs, let's do something smarter later (smash heap variables only when needed).
Depends on D4962107
Reviewed By: jvillard
Differential Revision: D4962121
fbshipit-source-id: 1b777a6
Summary:
Don't store redundant information in C++ template Type.Name.t.
New signature:
```
| CppClass (qual_name, template_args)
```
For example, for `std::shared_ptr<int>`, will look like this:
```
| CppClass (["std", "shared_ptr"], Template [int])
```
While it used to be:
```
| CppClass (["std", "shared_ptr<int>"], Template (["std", "shared_ptr"], [int]))
```
Reviewed By: jberdine, mbouaziz
Differential Revision: D4834512
fbshipit-source-id: cb1c570
Summary: The name of the source file was passed around everywhere but can also be accessed from the location associated to every node.
Reviewed By: sblackshear
Differential Revision: D4981848
fbshipit-source-id: 2ee592e
Summary: Now, all the summary access functions in the module `Specs` are of the form: `Specs.summary -> 'a`. This is a step toward making the analysis flow stateless.
Reviewed By: sblackshear
Differential Revision: D4976126
fbshipit-source-id: 28b6da1
Summary: This function is always used in the frontend where summaries don't exist yet
Reviewed By: akotulski
Differential Revision: D4979132
fbshipit-source-id: 8d49c52
Summary:
Before we understood ownership, we needed this to avoid a mountain of Builder-related FP's.
Now that we have fairly sophisticated understanding of ownership, we can kill this hack.
Reviewed By: jaegs
Differential Revision: D4940238
fbshipit-source-id: 8d86e57
Summary: Before, running any of these would crash with `Unsupported infer analyzer with Buck flavors:`
Reviewed By: jberdine
Differential Revision: D4970769
fbshipit-source-id: 76be6d5
Summary:
Add `volatile` and `restrict` type qualifiers. Change `Ast_expressions.create_*_type` functions
to always get optional type quals argument.
update-submodule: facebook-clang-plugins
Reviewed By: jberdine
Differential Revision: D4969634
fbshipit-source-id: 9a63bf7
Summary: This code only runs when Infer is running and is not reached when any other analyzer is used
Reviewed By: sblackshear
Differential Revision: D4973824
fbshipit-source-id: 700e24b
Summary: The Java frontend creates a single `tenv` file per `javac` invocation, but the code loading the `tenv` for a given Java procedure in the backend was not taking advantage of it. Also, with the lazy dynamic dispatch algorithm, the procedure name can be created on-demand and therefore defeat the approach to load the tenv by looking at the call graph to associate existing procedure names to the corresponding serialized tenv file. This diff should also fix this last point.
Reviewed By: sblackshear
Differential Revision: D4969254
fbshipit-source-id: 66ed318
Summary:
Title.
The way types are printed is completely valid, but little weird for some C++ programmers:
`int const` - same as `const int`
`int * const` - pointer is `const`, value under it is not
`int const *` - pointer is not `const`, but the value is
`int const * const` - both pointer and value are const
Reviewed By: jberdine
Differential Revision: D4962180
fbshipit-source-id: dcb02e3
Summary:
Modify the type of `Exp.Sizeof ...` to include the value that the expression
evaluates to according to the compiler, or None if it cannot be known
statically.
Use this information in inferbo.
Mostly unused in the BiAbduction checker for now, although it could be useful
there too.
update-submodule: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D4953634
fbshipit-source-id: be0999d
Summary:
`libstdc++` includes `bits/unique_ptr.h` via `include "unique_ptr.h"` in `bits/locale_conv.h` which infer can't redirect to model headers. This leads to compilation issues.
To work around that, redirect include of `locale_conv.h` to include `unique_ptr` via `include <...>` before normal `locale_conv.h`.
Reviewed By: mbouaziz
Differential Revision: D4962314
fbshipit-source-id: d4b9830
Summary:
Bottom means unreachable, not unknown.
Replace Bottom by Top in:
- unknown procedure return value
- unknown constant
- closure, exceptions
- when there are more formals than actuals
Depends on D4961989
Reviewed By: KihongHeo
Differential Revision: D4962006
fbshipit-source-id: e2a153d
Summary:
- Use Logging for logging
- Add a few extra debug (not ON by default)
- No space before colon
Depends on D4961957
Reviewed By: KihongHeo
Differential Revision: D4961989
fbshipit-source-id: 7c8697d
Summary:
Backend needs to know whether type is const or not. In order to achieve it, frontend needs to know it first.
This diff changes bunch of things:
- update clang plugin to have AST exporter actually export that info most of the time
- change types of functions in clang frontend until it compiles
- replace `type_ptr` with `qual_type` and `tp` with `qt` in names where applicable
- cleanup some things in the process
update-submodule: facebook-clang-plugins
Reviewed By: jvillard
Differential Revision: D4938567
fbshipit-source-id: 716b3ef
Summary: Sometimes reports need traces to be fully understood, but sometimes reporting where the exception takes place can save time to developers.
Reviewed By: jvillard
Differential Revision: D4914037
fbshipit-source-id: 039ab63
Summary: The purpose of the annotation reachability analysis is to report when a method annotated with `X` never calls, directly or indirectly, another method annotated with `Y`. However, there can be different call stacks following different execution paths from `X` to `Y`. Reporting more than one call stack ending with the same annotated procedure does not bring more signal to the end user. So the purpose of this diff is to avoid those duplicated reports and report at most one annotation reachability issue per end of call stack.
Reviewed By: sblackshear
Differential Revision: D4942765
fbshipit-source-id: 46325a7
Summary: The analysis logic was split between the treatment of the instructions and the definition of the domain, making the code more complicated that it should. This diff moves more of the logic into the domain definition and change to variable names to more descriptive ones
Reviewed By: sblackshear
Differential Revision: D4936414
fbshipit-source-id: ff59de7
Summary:
- Bottom-lift abstract memory domain to express unreachable node
- Two cases to make a node unreachable
+ constant: when an evaluation result of condition expression is
bottom or false, e.g., "prune(0)".
+ alias: when the same structure e is compared to itself with "<",
">", and "!=", e.g., "prune(e < e)".
- Add test for the new prune (prune_constant.c, prune_alias.c)
- Debug the semantics of comparison
Reviewed By: mbouaziz
Differential Revision: D4938055
fbshipit-source-id: d0fadf0
Summary: This will be needed to generate man pages with an accurate date.
Reviewed By: jberdine, mbouaziz
Differential Revision: D4937498
fbshipit-source-id: b5ebd31
Summary:
Went through the trouble of killing these warnings, make sure they do not creep
up again in the future.
Reviewed By: mbouaziz
Differential Revision: D4937493
fbshipit-source-id: 0e2eda4
Summary:
We won't be needing to know the terminal size when we move the help to
manpages. Hopefully if we need C stubs again in the future we can refer back to
this diff to revive them.
Reviewed By: jberdine
Differential Revision: D4937491
fbshipit-source-id: 99139fa
Summary: This is necessary to create a `report` subcommand (see later diffs).
Reviewed By: jberdine, dulmarod
Differential Revision: D4937488
fbshipit-source-id: 3fec0b5
Summary:
As an interprocedural checker, SIOF should not run unless explicitly required.
Make it a new type of analyzer like other similar checkers.
Reviewed By: mbouaziz
Differential Revision: D4937820
fbshipit-source-id: a9e2d38
Summary: There was unncecessary abstraction here - instead of looking up sil type in the map, simply store it as part of variant payload
Reviewed By: jberdine
Differential Revision: D4938114
fbshipit-source-id: 9fb8dd4
Summary: `bufferoverrun` is not part of `checkers` yet so bufferoverrun tests require their own Makefile
Reviewed By: jvillard
Differential Revision: D4937558
fbshipit-source-id: 20380f1
Summary:
Whenever running CTL in debug mode, you can now stop the execution and attach a step-by-step debugger by placing a breakpoint in the source code.
A breakpoint is the comment containing `INFER_BREAKPOINT`, placed on the line of interest.
You can attach the debugger to your breakpoint by running Infer with the following command
```
infer --linters-def-file my_linter.al --linters-developer-mode -- clang -c file.m
```
Reviewed By: dulmarod
Differential Revision: D4843358
fbshipit-source-id: acebde4
Summary: Handling special case of DeclStmt with VarDecl: emit the VarDecl node then emit the statements in DeclStmt as children of VarDecl. This is because despite being equal, the statements inside VarDecl and those inside DeclStmt belong to different instances, hence they fail the phys_equal check that should colour them
Reviewed By: dulmarod
Differential Revision: D4850681
fbshipit-source-id: b9683dc
Summary:
We lost InferClang++ a long time ago... It was installed properly on `make
install` but not during normal compilation!
Reviewed By: akotulski
Differential Revision: D4905947
fbshipit-source-id: 56375c2
Summary:
The error was complaining about a missing `etree` package but that's actually
part of the `lxml` package and that wasn't clear from the output.
See for instance https://github.com/facebook/infer/issues/635
Reviewed By: martinoluca
Differential Revision: D4905953
fbshipit-source-id: 28b26af
Summary: Sawja assigns them on multiple control-flow paths, so they're not SSA.
Reviewed By: peterogithub
Differential Revision: D4896745
fbshipit-source-id: c805216
Summary:
There are false positives in the current analysis due to the
use of conjunction in the treatment of threaded. Changing conjunction to disjunction
removes these false positives. Some new false negatives arise, but all the old tests pass.
This is a stopgap towards a better solution being planned.
Reviewed By: sblackshear
Differential Revision: D4883280
fbshipit-source-id: c2a7e6e
Summary:
These were showing up as allocation huge amounts of memory on some analysis
profiles from OpenSSL using Spacetime. Doesn't hurt to make them allocate less.
Reviewed By: jeremydubreil
Differential Revision: D4884520
fbshipit-source-id: e79b815
Summary:
Running `infer --help` with spacetime showed that `Str` was a hog. And indeed perf is 74% better now:
```
before:
bash -c 'for i in $(seq 100); do infer --help > /dev/null 2>/dev/null; done' 28.02s user 0.62s system 98% cpu 29.205 total
after:
bash -c 'for i in $(seq 100); do infer --help > /dev/null 2>/dev/null; done' 6.34s user 0.64s system 92% cpu 7.557 total
```
Running on many files one after the other is now 64% faster too:
```
$ cd infer/tests/codetoanalyze/c/errors
$ # before
$ rm -fr infer-out && time bash -c 'for src in */*.c; do infer -a capture --continue -- clang -c $src; done'
bash -c 7.77s user 0.66s system 97% cpu 8.647 total
$ # after
$ rm -fr infer-out && time bash -c 'for src in */*.c; do infer -a capture --continue -- clang -c $src; done'
bash -c 2.35s user 0.56s system 93% cpu 3.119 total
$ time infer -a capture -- clang -c */*.c
infer -a capture -- clang -c */*.c 0.54s user 0.20s system 99% cpu 0.737 total
```
Reviewed By: mbouaziz
Differential Revision: D4875803
fbshipit-source-id: cfcfa69
Summary:
OCaml 4.04.0 new warnings raised a few valid points!
Fixing warning 57 in two ways:
- best way: introduce an auxiliary function to avoid code duplication
- not-so-best way: introduce code duplication. I did that when the branches body are small. Typically the number of bound variables in the pattern is high, so an auxiliary function would need to take many arguments and the whole thing will not be readable (we'd still duplicate the arguments we pass to the function for instance).
Reviewed By: jberdine
Differential Revision: D4851006
fbshipit-source-id: fbf1867
Summary:
Limit the use of `SourceFile.invalid` (renamed from `SourceFile.empty`) as much
as possible. In particular, do not generate bogus procnames for external global
variables: their translation unit was set to the invalid source file, now we
distinguish between extern/non-extern global variables more explicitly.
`SourceFile.invalid` is still used in too many places to actually remove it, often as a dummy initial value that never gets used, but sometimes as an actual value... Worse, we cannot fail on all operations on `SourceFile.Invalid` yet: the `SourceFile.to_string` method is used in too many places where it could get `SourceFile.Invalid` as argument. It's easy to see where it's used by making it raise in the code, then running the tests. This results in spaghetti backtraces that are hard to trace back to a root cause.
Reviewed By: akotulski, jeremydubreil
Differential Revision: D4860019
fbshipit-source-id: 45be040
Summary:
Try to read .inferconfig in the current directory, then in .., then in ../..,
etc. This can be overriden with the `INFERCONFIG` environment variable.
This removes the need for two-phase parsing, so clean up that code too.
Paths in .inferconfig are interpreted relative to where .inferconfig is located.
This does not apply to other path-sensitive things like regexpes... this is not
a show stopper because regexpes can account for the fact that infer may be
called from different project roots.
Make sure we fail when .inferconfig exists but cannot be read.
Reviewed By: jberdine
Differential Revision: D4843142
fbshipit-source-id: 340a63f
Summary: The flakiness is exercised by upgrading the OCaml compiler to 4.04.0.
Reviewed By: jberdine
Differential Revision: D4859904
fbshipit-source-id: 0dd7ff6
Summary:
If we couldn't project the callee access path into the caller during summary application, we still added the corresponding trace to the caller state.
This was wasteful; it just bloats the caller with state it will never look at.
Fixed it by making `get_caller_ap_node` return `None` when the state won't be visible in the caller.
Reviewed By: jeremydubreil
Differential Revision: D4727937
fbshipit-source-id: 87665e9
Summary: This is required to upgrade OCaml as our ancient Reason is not available on 4.04.0.
Reviewed By: yunxing
Differential Revision: D4851582
fbshipit-source-id: 994a9a8
Summary:
It's distracting to see the debug HTML for the preanalysis when you're trying to debug something else.
Also, it breaks the nice bi-abduction debug feature of marking the visited nodes as green.
Reviewed By: akotulski
Differential Revision: D4858578
fbshipit-source-id: 8e77976
Summary: This should make the reports much easier to understand. We can generalize to reporting a stack trace for all of the writes in the future if we wish.
Reviewed By: peterogithub
Differential Revision: D4845641
fbshipit-source-id: 589fdbc
Summary: Prereq for reporting a call stack for both the read and write in a read/write race.
Reviewed By: peterogithub
Differential Revision: D4845603
fbshipit-source-id: ebfeb9b
Summary:
We shouldn't encourage contributors to run only a subset of the tests with
`make test`, but it's more helpful to complain clearly should they try to do
so.
Now `make test` will ensure that both the clang and the Java analyzers are
enabled, otherwise it will refuse to run and suggest to run `make config_tests`
instead.
It can still be useful to be able to run only the Java or Clang tests. The diff
also fixes the cases where that previously failed.
Fixes#634
Reviewed By: jberdine
Differential Revision: D4826515
fbshipit-source-id: 4b76029
Summary:
This debugger stops the execution of CTL during the evaluation of formulas, then shows useful info about the current state:
- Which formula is being evaluated
- In which part of the AST is such formula being evaluated
- Line number of the source file
- Whether a CTL-Transition will take place or not
- Node-ID that can be related to the dotty file containing the evaluation graph (the one in infer-out/lint_dotty/)
The AST is shown with coloured nodes, where each colour has the following meaning:
- [Default terminal colour], Bold: The node is being evaluated
- Green, Bold: The formula returned true on the current node
- Red, Bold: The formula returned false on the current node
Reviewed By: ddino
Differential Revision: D4834451
fbshipit-source-id: c9aa970
Summary:
Make it possible to write one model which will be used by all template instantiations.
There is one big missing piece: infer never tries to do template instantiation by itself. With current code, it's possible to use generic models
as long as header contains `__infer_generic_model` annotation (see the test as an example).
This is not viable to modify all headers with this annotation hence infer will try to do template instantiation for generic models in later diffs.
Reviewed By: jberdine
Differential Revision: D4826365
fbshipit-source-id: 2233e42
Summary:
Initial version of naming, required for generic models. It's simply non mangled name stripped from any template arguments.
This makes it impossible to have two generic models with
1. different template arguments
2. different overloads (function with same name, but different types of arguments)
Reviewed By: jberdine
Differential Revision: D4826358
fbshipit-source-id: 42ac763
Summary: If two public methods touch the same state and only one is marked `ThreadSafe`, it's reasonable to report unsafe accesses on both of them.
Reviewed By: peterogithub
Differential Revision: D4785038
fbshipit-source-id: 5a80da4
Summary: `__attribute__((annotate("")))` is better way of passing information to the frontend
Reviewed By: jberdine
Differential Revision: D4818805
fbshipit-source-id: 6e8add2
Summary:
- Do not print OCaml backtrace unless in developer mode.
- Print javac output when it fails.
- Refactor Javac.ml so that rerunning the command doesn't duplicate code.
The output of the clang and Javac integration is good, the output for mvn is
ok, the output for Buck is still fairly "meh".
Reviewed By: jberdine
Differential Revision: D4818722
fbshipit-source-id: 1973b93
Summary: This will avoid the redefine this Map and Set module as pretty printable when used to create abstract domains.
Reviewed By: sblackshear
Differential Revision: D4811849
fbshipit-source-id: e2f6763
Summary:
*Unless* the unprotected write runs on the main thread and the read doesn't.
Otherwise, we'll already report on the unprotected write, and we don't want to duplicate.
Reviewed By: peterogithub
Differential Revision: D4798357
fbshipit-source-id: 5de06a0
Summary:
Currently --per-procedure-parallelism defaults to a chunk size of 1
procedure, which has a high overhead. Add a command line option to
control it, and raise the default value.
Reviewed By: jvillard
Differential Revision: D4794692
fbshipit-source-id: 7715a40
Summary:
Otherwise, we can get an exception when calling `Fieldname.java_get_field`.
Thanks to ngorogiannis for reporting.
Reviewed By: jeremydubreil
Differential Revision: D4805197
fbshipit-source-id: 3141bb1
Summary: This representation is more natural and results in less `List.rev` calls.
Reviewed By: jberdine
Differential Revision: D4762629
fbshipit-source-id: 481cbe4
Summary:
This is step further simplify the code to avoid cases where the summary of the procedure being analyzed can exist in two different versions:
# one version is the summary passed as parameter to every checker
# the other is a copy of the summary in the in-memory specs table
This diff implements:
# the analysis always run through the `Ondemand` module (was already the case before)
# the summary of the procedure being analyzed is created at the beginning of the on-demand analysis call
# all the checkers run in sequence, update their respective part of the payload and log errors to the error table
# the summary is store at the end of the on-demand analysis call
Reviewed By: sblackshear
Differential Revision: D4787414
fbshipit-source-id: 2d115c9
Summary:
This checker was always running by default but was apparently never reporting.
This checkers can always be run using:
infer -a checkers --checkers-repeated-calls -- ...
Reviewed By: sblackshear
Differential Revision: D4782472
fbshipit-source-id: 5ec77f4
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
Sam Blackshear
Jeremy Dubreil
Jules Villard
Mehdi Bouaziz
Dino Distefano
Dulma Churchill
Martino Luca
Ryan Rhee
Andrzej Kotulski
Peter O'Hearn
Sungkeun Cho
Kihong Heo
Josh Berdine
htxs
Paco Estevez Garcia
Cristiano Calcagno
Kihong Heo
Matthew Haughton
Daniel Wagner-Hall