Summary: We report dead store false positives in template arguments when constexpr is used. To remove the false positives, with the expense of some false negatives, we do not report dead stores on constexpr anymore.
Reviewed By: mbouaziz
Differential Revision: D9608095
fbshipit-source-id: 91b0c71c4
Summary:
Lambdas can capture references to locals of the enclosing method as long as
they are not propagated outside the method. However to keep things simple
always allow them to capture locals of the enclosing method at the price of
some false negatives.
Reviewed By: da319
Differential Revision: D8974434
fbshipit-source-id: 957ae44bd
Summary:
- Was not used by the caller
- Gives smaller summaries
- Will allow adding a intra-proc info, e.g. `node` for reporting (not sure yet)
Reviewed By: skcho
Differential Revision: D9373763
fbshipit-source-id: 322001b53
Summary: The pattern matching could previously be missing some valid cases (in theory).
Reviewed By: mbouaziz, jberdine
Differential Revision: D9491441
fbshipit-source-id: 2bc1fc1aa
Summary:
In SIL, (1) some program variables (e.g., array parameter) are used as pointers to heap addresses and (2) the other program variables (e.g., local array) are used as addresses themselves. So, the values of (1) are retrieved by the `Load` command, while that of (2) are by `Exp.Lvar` expressions directly.
To address them differently, we had managed two maps (`Mem.Stack` and `Mem.Heap`), but which introduced function duplications on abstract memory and increased complexity. This diff merges the two maps, and instead a location set is used for distinguishing two types of abstract locations during analysis.
Reviewed By: mbouaziz
Differential Revision: D9420388
fbshipit-source-id: 13f824850
Summary: It is easier to filter out those reports in `.inferconfig` if we want them that modifying a boolean value ddirectly in the code
Reviewed By: mbouaziz
Differential Revision: D9494082
fbshipit-source-id: 9fb042313
Summary: There no clear alternative to using models of the standard library at this point so we can simplify the code a little bit
Reviewed By: mbouaziz
Differential Revision: D9491062
fbshipit-source-id: 9e5a6eeea
Summary:
It returns unknown values on non-const function calls like on unknown
function calls.
Reviewed By: mbouaziz
Differential Revision: D9478862
fbshipit-source-id: 4b795ec55
Summary:
Not all clang commands are happy with all arguments, but the driver is usually
the place we want to add arguments to.
Reviewed By: martinoluca
Differential Revision: D9421403
fbshipit-source-id: fa6d39a9b
Summary:
After some testing, it looks like getting the pdesc via
`Ondemand.get_proc_desc` will also load models' proc descs from their
summaries, so this code should not be needed.
Reviewed By: jeremydubreil, mbouaziz, martintrojer
Differential Revision: D9197176
fbshipit-source-id: 1b8603bfa
Summary:
`Errlog` will merge similar issues (same severity, name, description) reported at the same location, so let's make sure the locaiton is mandatory.
Issues:
- errors happening in `Ondemand` still use the `State` which makes sense only for biabduction and eradicate
- a case of `NullabilitySuggest` didn't have a location, I did my best to patch it but I'm sure the location could be more precise
Reviewed By: jvillard
Differential Revision: D9332840
fbshipit-source-id: ee7898146
Summary: Keeping pushing arguments higher in the stack, `node_id_key` is not used in calls to `log_warning/error`
Reviewed By: jvillard
Differential Revision: D9332826
fbshipit-source-id: e5c48c686
Summary:
Before we would convert it to string in `Reporting` and pass it to `Errlog` which would use it only to 'log events'.
I guess the reason is that there was a cyclic dependency between `Errlog` and `clang_method_kind` defined in `ProcAttributes`.
This diff:
- moves it to its own module
- defers the conversion to string
Reviewed By: jvillard
Differential Revision: D9332819
fbshipit-source-id: 43a028b61
Summary:
- abstracted the type for a node key
- moved it to its own module with an ugly `compute` to avoid cyclic dependencies...
- renamed `node_id` to `node_id_key` where needed
- moved key computation from `State` to `Procdesc.Node`
Reviewed By: jvillard
Differential Revision: D9332803
fbshipit-source-id: fe1ae8c1c
Summary:
- made arguments of `Errlog.log_issue` mandatory
- pushed some arguments of `log_issue_from_errlog` higher in the stack, the goal is to make sure `State` is only used in analyses that update it (biabduction and eradicate, if I'm correct)
Reviewed By: jvillard
Differential Revision: D9332773
fbshipit-source-id: ce79df21c
Summary: The `procedure` field in the final report should use the non-ambiguous fully qualified name containing the Java package declaration and the list of parameter types.
Reviewed By: mbouaziz
Differential Revision: D9237522
fbshipit-source-id: e9b0ff664
Summary: C++17 introduce guaranteed copy elision which omits constructor calls. In ownership analysis, we depended on these constructor calls to acquire ownership. In particular, when a method returns struct, previously, a constructor was used to acquire ownership. In this diff, we acquire ownership of the returned structs directly.
Reviewed By: mbouaziz
Differential Revision: D9244302
fbshipit-source-id: ae8261b99
Summary:
Useful tips and tricks to debug infer's OCaml code. Also emit a developer
warning when the database is not initialised (since it's only expected to
happen when running infer from the toplevel).
Reviewed By: mbouaziz
Differential Revision: D9295782
fbshipit-source-id: 09b7b9a02
Summary:
To keep up with the times. Changes consist of new features and moving modules
around so shouldn't change anything on our side.
Depends on D9239803
The controller you requested could not be found.: facebook-clang-plugins
Reviewed By: da319
Differential Revision: D9239817
fbshipit-source-id: d02a2076a
Summary:
Use `ignore` instead, as this will warn if the argument is an arrow type,
unlike `let _ = ...`. This makes the code more future-proof: if an argument is
added to a function called in `let _ = f x` then the compiler will complain
instead of silently turning a value into a partial evaluation.
Also got rid of particularly irksome `let _ = <stuff returning unit> in` where I could.
Reviewed By: mbouaziz
Differential Revision: D9217176
fbshipit-source-id: 3be463405
Summary: It moves the functions that constructs array values from BufferOverrunSemantics to ArrayBlk and Val modules.
Reviewed By: mbouaziz
Differential Revision: D9194130
fbshipit-source-id: bf040a01a
Summary:
It removes the sizeof function because most of the cases on static types are addressed in the clang frontend.
Depends on D9193802
Reviewed By: mbouaziz
Differential Revision: D9213876
fbshipit-source-id: 0ce2f3749
Summary:
The internal concept of "kind" should in fact be named "severity" to match the convention used by many other tools, whereas the internal concept of "severity", i.e "HIGH", "MEDIUM" and "LOW" was never used and in any case redundant with the concept of "info", "warning", "error".
This diff maps both the "kind" and "severity" fields to value of the form "advice", "info", "warning", and "error" to be able to progressively migrate the code using the "kind" field.
Reviewed By: mbouaziz, jvillard
Differential Revision: D9187978
fbshipit-source-id: 447d89f51
Summary:
- changes the `Ondemand` callbacks to take the execution environment instead of a `get_proc_desc` function.
- removes all the cases passing `get_proc_desc` as parameter to use `Ondemand.get_proc_desc` instead.
Reviewed By: jvillard
Differential Revision: D9200583
fbshipit-source-id: d16c218b5
Summary:
Some paths are hardcoded in infer as being relative to the current executable,
for instance the directory where to find the models. By copying infertop.bc to
infer/bin like we do for `infer` these relative paths lead to the expected
place, which means models can be loaded in the toplevel like they would be in a
normal infer execution. This is more useful for debugging than previously.
Reviewed By: jeremydubreil, mbouaziz
Differential Revision: D9197142
fbshipit-source-id: 48c4f82fb
Summary:
Using the proc name directly should be equivalent and does not rely on
`Summary` caching summaries for us.
Reviewed By: mbouaziz
Differential Revision: D9196503
fbshipit-source-id: dea67999a
Summary:
We want to kill the cache in `Summary`, and calling `Summary.get` relies on
that cache existing for efficiency. However in this case it's not needed
because we can pass the summary from above instead.
Reviewed By: mbouaziz
Differential Revision: D9195234
fbshipit-source-id: 5b7023242
Summary:
Exposing the in-memory cache seems dangerous. There were only 2 uses anyway:
eradicate and biabduction. I think the biabduction one is safe to remove. The
eradicate one I do not really understand as we return a different summary than
the one we cache... the tests pass though.
Reviewed By: jeremydubreil
Differential Revision: D9150167
fbshipit-source-id: cf30af232
Summary:
When we see `pthread_create(..., ..., foo, ...)`, we want to call the function
`foo` to check that its precondition is met. The initial goal was to get rid
of the uncouth call to `Summary.get` when what we really want is to analyse
`foo` instead of just betting on the fact that it has been analysed already.
Besides switching to `Ondemand.analyze_proc_name`, this also changes the
matching of the function pointer in the arguments of `pthread_create()` to
detect the common case of a constant function name. I also added tests.
Reviewed By: jeremydubreil
Differential Revision: D9195159
fbshipit-source-id: dfec79f14
Summary: This should be functionally equivalent but removes one call to `Summary.get`
Reviewed By: jvillard
Differential Revision: D9153924
fbshipit-source-id: d49789d2f
Summary:
Because of the custom formatting we do on top of `Cmdliner.Manpage` in
`CommandLineOption`, the long `$(b,strings inside bold block)` would sometimes
end up split across several lines and then `Cmdliner` complains about the line
ending before having seen the closing parenthesis `)`.
Instead of allowing these long strings, remove then from the help message as
they don't seem very useful to me. Replace them with just a mention of
`--test-determinator` so all the options related to Test Determinator are easy
to look up.
Also change the relevant options from `CLOpt.mk_string` to `CLOpt.mk_path`
because they look like they are supposed to be paths.
Reviewed By: ddino
Differential Revision: D9133505
fbshipit-source-id: d82acf5bf
Summary: This code is no longer necessary because the bug hash does not depend on the name of the anonymous classes
Reviewed By: mbouaziz
Differential Revision: D9176205
fbshipit-source-id: 9a8e9c9f8
Summary: It uses a SymbolPath map to Symbol in Inferbo's summary instead of an entry memory of callee, which is used for instantiations of the abstract memories on function calls.
Reviewed By: mbouaziz
Differential Revision: D9081631
fbshipit-source-id: 478cda0de
Summary: Errors that include temporary variables are difficult to understand. Do not report stack variable address escape on temporary variables.
Reviewed By: jvillard
Differential Revision: D9117517
fbshipit-source-id: 9ebd75ecc
Summary:
Print the following for each source file to analyse in non-interactive mode:
```
path/to/source_file.c starting
[...]
path/to/source_file.c DONE in <time>
```
This should help diagnose when infer is stuck. It also logs this information to
the log file regardless of the form of the progress bar.
Also add a `--progress-bar-style` option to allow the user to force a
particular rendering: plain (as above), multiline (The Glorious One), or auto
(selection depends on whether infer is connected to a TTY on stdin *and*
stderr).
Reviewed By: mbouaziz
Differential Revision: D9120509
fbshipit-source-id: 4b43b7464
Summary: Treat calls to Thread.sleep as blocking, even when the timeouts are less than the ANR limit.
Reviewed By: da319
Differential Revision: D9027950
fbshipit-source-id: 001409896
Summary:
The current message is confusing when the current class is marked UIThread.
See picture attached to task.
Reviewed By: jeremydubreil
Differential Revision: D8996593
fbshipit-source-id: cf52ee5d6
Summary:
It adds relational domains to Inferbo: octagon of Apron and polyhedra of Elina.
- Each Mem domain value includes one relational value containing relations among symbols. The relational values are modified by the `Prune` and `Store` commands.
- Each abstract value includes three symbols, which represent integer value, array offset, and array size of an abstract value.
The relational domain is deactivated by default. Use the `--bo-relational-domain {oct, poly}` option for the activation, though Inferbo with the relational domains does not work at this point because some modifications of Apron and Elina we made has not been applied to their opam repositories yet.
Reviewed By: jvillard
Differential Revision: D8874102
fbshipit-source-id: 08e5883cb
Summary: This avoid cases where the type definition is not found when trying to lookup the type definition for the class starting from the method name.
Reviewed By: ngorogiannis
Differential Revision: D9027983
fbshipit-source-id: 1add7692f
Summary: Added variant type for statement node to make it cleaner to match a particular statement node.
Reviewed By: mbouaziz
Differential Revision: D8997124
fbshipit-source-id: e19f6eacd
Summary:
Guava subclasses Future in ways that make .get() calls safe from the UI thread.
Treat such methods as skip.
Reviewed By: jeremydubreil
Differential Revision: D9013475
fbshipit-source-id: 38373aa5f
Summary:
Litho uses an idiom as follows. `Component.Builder` has an abstract method `T getThis()` which is supposed to be implemented as `{return this;}`.
I believe the reason it's there is to have covariant return types in subclasses, though this doesn't really matter.
We don't do dynamic dispatch, so instead of summarising `getThis` as having a return ownership of `OwnedIf({ 0 })` we just return `Unowned`.
This is generating many false positives, which are really hard to debug.
Here I'm special-casing any abstract method whose return type is equal to that of it's only argument, to return conditional ownership.
Reviewed By: da319
Differential Revision: D8947992
fbshipit-source-id: 33c7e952d
Summary:
`ANSITerminal` flushes after almost every operation. As a result, there is a
lot of flicker in the task bar. Going through `Format` instead reduces the
flicker.
Also erase end-of-lines when outputting logging stuff to the console. This makes sure the output is not garbled with stuff from the task bar.
Reviewed By: ngorogiannis
Differential Revision: D8931294
fbshipit-source-id: e89c87f84
Summary: Exceptional successors were not meant to be created for return nodes, but they were created if try block had a single return statement.
Reviewed By: jvillard
Differential Revision: D8913371
fbshipit-source-id: 6ac85b21d
Summary: This integration rolls out its own parallelisation using `Parmap` and its own error handling. Make it use `Tasks` instead.
Reviewed By: mbouaziz
Differential Revision: D8807109
fbshipit-source-id: ac09b3791
Summary: `IntLit.to_int` could raise, was not documented until recently and was not named `_exn`. Switch to option type and fix uses.
Reviewed By: jeremydubreil
Differential Revision: D8865525
fbshipit-source-id: f5ec2f221
Summary: `ownership_of_expr` is already doing some of the work in `propagate_return`. Also, split and move into abstract domain, where it belongs.
Reviewed By: jeremydubreil
Differential Revision: D8855257
fbshipit-source-id: 7756552d8
Summary:
Currently, some blocking calls are turning up too many false positives. Adjust severities by fix rate/preexisting numbers.
Also, restrict some calls to exact class, as opposed to superclasses.
Reviewed By: mbouaziz
Differential Revision: D8850599
fbshipit-source-id: 47543d04a
Summary:
It adds relational domains to Inferbo: octagon of Apron and polyhedra of Elina.
- Each `Mem` domain value includes one relational value containing relations among *symbols*. The relational values are modified by the `Prune` and `Store` commands.
- Each abstract value includes three *symbols*, which represent integer value, array offset, and array size of an abstract value.
The relational domain is deactivated by default, so this diff should not make any differences in CI.
Use `--bo-relational-domain {oct, poly}` for the activation, though Inferbo with the relational domains does not work at this point because some modifications of Apron and Elina we made has not been applied to their opam repositories yet.
Reviewed By: mbouaziz, jvillard
Differential Revision: D8478542
fbshipit-source-id: 510ff53
Summary:
When `--reanalyze` is passed, mark the summaries of procedures matching
`--procedures-filter` as needing to be analysed before running the analysis.
This allows one to, for instance, re-run the analysis in debug mode on only
some files or procedures. However, this won't work for the Java Buck
integration since the summaries are hidden away in buck-out.
Reviewed By: mbouaziz
Differential Revision: D8783668
fbshipit-source-id: 9032d83
Summary:
This allows to deduplicate some code related to walking the rows of the results
of a SQLite query. Give more meaningful names to the API while I'm at it.
Reviewed By: mbouaziz
Differential Revision: D8783332
fbshipit-source-id: 4aa6613
Summary: All the rows were wrapped in `Some` but that is not needed anywhere.
Reviewed By: mbouaziz
Differential Revision: D8783310
fbshipit-source-id: b020af3
Summary: Some values in Config.mli were in the wrong section. Also, use ocamldoc formatting for sections.
Reviewed By: mbouaziz
Differential Revision: D8783244
fbshipit-source-id: f9a0729
Summary:
Filtering on the SQLite side was done to be more efficient, but these are debug
options so it should be fine for them to be not very optimised.
Filtering on the OCaml side will allow us to re-use these filtering options for
other purposes, such as re-analysing certain procedures only.
Reviewed By: mbouaziz
Differential Revision: D8767691
fbshipit-source-id: e232660
Daiva Naudziuniene
Mehdi Bouaziz
Sungkeun Cho
Jeremy Dubreil
Martino Luca
Jules Villard
Josh Berdine
Ezgi Çiçek
Nikos Gorogiannis
Dino Distefano
Martin Trojer
Julian Sutherland
Katie Ots
Dulma Churchill