Summary:
Access paths are too coarse to properly address C/C++ instructions, and lead to false positives and negatives. Begin the process of porting the underlying domains to access expressions, in a results-preserving way. This roughly consists in:
- Adding missing functions in `AccessExpression` to mirror those in `AccessPath`.
- Replacing `AccessExpression` for `AccessPath` and removing conversions from the former to the latter except in:
- Printing functions, to ensure formatting issues won't change tests/CI.
- Reporting/deduplication still happens through access path conversion, as we need an analogue of `ModuloThis` for `AccessExpression`.
- In selected places, ignore any access type not present in `AccessPath` (ie. dereference/take address of).
Reviewed By: jberdine
Differential Revision: D16856721
fbshipit-source-id: 5e3a88b75
Summary:
The models are only for biabduction so try to make that clearer in the
code and documentation.
Reviewed By: skcho
Differential Revision: D16603147
fbshipit-source-id: 4a2be53de
Summary: Sometimes programmers use integer underflow to get a maximum number of that type. This diff assumes that integer underflows from the syntactical form `(unsigned 0) - constant` is intended by the programmer, and suppresses the alarms of which.
Reviewed By: ezgicicek
Differential Revision: D16560639
fbshipit-source-id: 206f30dbc
Summary:
newer is better, right?
All the code changes in infer are because of core being bumped to v0.12.
Reviewed By: jberdine
Differential Revision: D16223183
fbshipit-source-id: f3c339966
Summary:
[apologies for the unreviewable diff...]
Get rid of HIL expressions in pulse. This finishes the HIL -> SIL
migration. The first step made pulse start from SIL instructions but
would translate most accesses to HIL to re-use most of the existing
pulse code. This diff gets rid of the intermediate translation of SIL
expressions to HIL expressions.
Big changes:
1. `PulseOperations` mostly rewritten, driven by using `Exp.t` instead of `HilExp.AccessExpression.t` for everything.
2. Stop trying to reverse-engineer what addresses mean in terms of
access paths from program variables. Rely on the trace pointing at
the right places in the code to be enough. This is because it wasn't
that useful (and could even be misleading when wrong) but could be
prohibitively expensive in degenerate cases (eg nodes with tens of
thousands of successive array accesses...)
3. `PulseAbductiveDomain.apply_post` now returns the computed return
value instead of recording it itself.
4. Change of vocabulary: `materialize` -> `eval`, `crumb` -> `event`
5. Function calls arguments are now evaluated prior to doing anything
else, which saves everything else from having to (remember to) do
that. In particular, this changes how models look quite a bit.
Reviewed By: mbouaziz
Differential Revision: D15986373
fbshipit-source-id: 1d79935de
Summary:
Reduces the size of the `tenv` by sharing values as most as possible, in an untyped - but supposedly safe - way, by using black magic on objects.
Can be reused for other things later.
Reviewed By: ngorogiannis
Differential Revision: D15855870
fbshipit-source-id: 169a4b86b
Summary:
Using `Marshal.to_string` to create SQLite values used in comparisons is brittle as there is no guarantee that it will return the same value for structurally equal values.
When adding sharing, this will definitely break.
From the SQLite queries I found, only `SourceFile` and `Procname` are used in comparisons.
I haven't tested performance.
It shouldn't change anything for `SourceFile` as there is no possible sharing.
It shouldn't change much for `Procname` as they are pretty small anyway.
Reviewed By: ngorogiannis
Differential Revision: D15923122
fbshipit-source-id: ce4af1fe3
Summary: Inject destructor calls to destroy a temporary when its lifetime ends.
Reviewed By: mbouaziz
Differential Revision: D15674209
fbshipit-source-id: 0f783a906
Summary:
Now that HIL doesn't help us anymore we need to reconstruct its mapping
"SIL logical var -> program access path". We already have everything we
need in pulse: it suffices to walk the current memory graph starting
from program variables until we find the value of the temporary we are
interested in.
This diff also builds some type machinery to make sure all accesses are
explained.
Reviewed By: mbouaziz
Differential Revision: D15824959
fbshipit-source-id: 722c81b39
Summary:
It turns out HIL gets in the way of a precise heap analysis. For
instance, instead of:
```
n$0 = *&x.f
_ = delete(&x)
*&y = n$0
```
HIL tries hard to forget about intermediate variables and shows instead
```
_ = delete(&x)
*&y = *&x.f
```
Oops, that's a use-after-delete, whereas the original code was safe.
While it's easy to write SIL programs that are completely unsound for
HIL, they are not generated very often from the frontends. In fact, the
problem became apparent only when making the clang frontend translate
C++ temporaries destructors, which produces the situation above
routinely.
This diff makes the minimal amount of change to make Pulse build and
produce equivalent results (minus HIL bugs) starting from SIL instead of
HIL. The reporting sucks for now because we need to translate SIL
temporaries back into program access paths. This is done in the next
diff.
Reviewed By: mbouaziz
Differential Revision: D15824961
fbshipit-source-id: 8e4e2a3ed
Summary: Preanalysis is performed at the frontend now. Hence, we don't need to repeatedly check/set when/if it is performed.
Reviewed By: mbouaziz
Differential Revision: D15863175
fbshipit-source-id: f9c6b7ae1
Summary:
One "interesting" feature of the approach of merging the captured targets in Java, is that we union their type environments, as opposed to store partial tenvs together with each source file, which is the case for Clang.
This means
- the final global type environment is potentially huge because it contains all the types in all targets.
- all analysis workers start by loading that tenv in memory, meaning we consume `|size of tenv| x #cpus` memory, which can tip the balance towards OOMs
This diff attempts to economise on global tenv size. This is done by increasing sharing which is then preserved by marshalling. It's done in a brute force way, with hashtables for each struct component, and is not fully effective due to the recursion amongst types and types names, as well types appearing inside other constructs such as procnames.
This is done when calling `Tenv.store` so that
- the computation can be parallelised somewhat (capture is parallel, merging is not)
- buck caching will benefit from smaller tenvs.
This saves about 24% of total memory devoted to the type environment.
Reviewed By: mbouaziz
Differential Revision: D15840054
fbshipit-source-id: 6f03be1a4
Summary:
This started as an attempt to understand how to modify the frontend to
inject destructors for C++ temporaries (see next diffs).
This diff rewrites the existing logic for computing the list of
variables that should be destroyed at the end of each statement, either
because it's the end of their syntactic scope or because control flow
branches outside of their syntactic scope.
The frontend translates a function from the last instructions to the
first, but scope computation needs to be done in the other direction, so
it's done in a separate pass *before* the main translation happens. That
first pass creates a map from statements in the AST to the list of
variables that should be destroyed at the end of these statements. This
is still the case now.
Before, that map would be computed in a bit of a weird way: scopes are
naturally a stack but instead of that the structure maintained was a
flat list + a counter to know where the current scope ended in that
list.
In this diff, redo the computation maintaining a stack of scopes
instead, which is a bit cleaner. Also treat more instructions as
introducing a new scope, eg if, for, ...
Reviewed By: mbouaziz
Differential Revision: D15674208
fbshipit-source-id: c92429e82
Summary:
Somewhat trivial: add a string to "Destruction" nodes to indicate why
they were created. Rename the main `instruction_aux` function into
`instruction_translate` (see next diff for why).
Reviewed By: mbouaziz
Differential Revision: D15674211
fbshipit-source-id: 8a7eda72c
Summary: This allows to match `foo<int_&>` and many other horrible names.
Reviewed By: mbouaziz
Differential Revision: D15825403
fbshipit-source-id: c892033aa
Summary:
This is a simple checker that identifies inefficient uses of `keySet` iterator where (not only the key but also) the value is accessed via `get(key)`. It is more efficient to use `entrySet` iterator which already returns both key-value pairs. This optimization would get rid of many extra lookups which can be expensive.
We simply traverse the CFG starting from the loop head upwards and pick up the map that is iterated over. Then, we check in the loop nodes if there is a call to `get(...)` over this map. If, so we report.
Reviewed By: ngorogiannis
Differential Revision: D15737779
fbshipit-source-id: 702465b4e
Summary:
The synthetic methods from `topl.Property` are now nonempty: they
simulate a nondeterministic automaton.
Reviewed By: jvillard
Differential Revision: D15668471
fbshipit-source-id: 050408283
Summary:
Instrument SIL according to TOPL properties. Roughly, the
instrumentation is a set of calls into procedures that simulate a
nondeterministic automaton. For now, those procedures are NOP dummies.
Reviewed By: jvillard
Differential Revision: D15063942
fbshipit-source-id: d22c2f6fa
Summary:
This messes with the deduplication heuristic when templated function
names show up in the error messages, since the heuristic demands that
the error messages are the same.
Reviewed By: mbouaziz
Differential Revision: D15374333
fbshipit-source-id: 70232d254
Summary:
Improve the error messages, change is more or less documented in the
code.
Reviewed By: mbouaziz
Differential Revision: D15374334
fbshipit-source-id: f1dd54180
Summary: No reason to use custom function name and not implement `Hashable`.
Reviewed By: mbouaziz
Differential Revision: D15097603
fbshipit-source-id: 7303fc15e
Summary:
TOPL properties are essentially automata, which will be modeled as a set
of procedures. The code-to-analyze makes calls into these procedures,
thereby driving the automaton. In this commit, these calls do not do
anything. The point is to prepare the hook-up mechanism.
Reviewed By: jvillard
Differential Revision: D14819650
fbshipit-source-id: d95ecdb3d
Summary:
A long-standing easter egg from infer error messages is the "object
`null` could be null and is dereferenced at line ...". I tried to fix
this but the part that generates the first "null" in the message and the
part that generates the second one are very far apart and it's hard to
see how to make the second part aware of the first in a clean way.
Instead, hack around it by detecting if the string representing the
value is literally `null` and in that case chop `could be null ` from
the error messages...
Reviewed By: jeremydubreil
Differential Revision: D14972324
fbshipit-source-id: ccc48ce6b
Summary:
We get messages like " object returned by `getArguments()` at line 101."
instead of " object returned by `getArguments()` could be null and is
dereferenced at line 101.". Tracking it down, it happens for
nullable-looking values, but I don't know why.
It seems that something regressed but I couldn't track it down.
So, just generate the error message in the same way as for non-nullable
objects in this case to fix the non-sensical message.
Reviewed By: jeremydubreil
Differential Revision: D14972325
fbshipit-source-id: 2a97501cc
Summary:
Feedback from peterogithub:
- mention which access path is being invalidated and accessed in the message
- mention the line at which it was invalidated (the line at which it's accessed is already the line at which we report)
- traces for stack variable/C++ temporary address escapes
- delete double implementation of the same functionality in
`PulseTrace`: `location_of_action_start` is the same as
`outer_location_of_action`...
Reviewed By: jberdine
Differential Revision: D14800294
fbshipit-source-id: 3d9ab9b3d
Summary:
The previous message formatting had regressed and produced non-sensical messages.
More importantly, remove template parameters from error messages to
trigger the heuristic in `InferPrint` that deduplicates errors that are
on the same line with the same error type and message. Without this we
get hundreds of reports that correspond to as many instantiations of the
same code.
Reviewed By: ngorogiannis
Differential Revision: D14747979
fbshipit-source-id: 3c4aad2b1
Summary:
Instead of emitting an ad-hoc builtin on variable declaration emit a new
metadata instruction. This allows us to remove the code matching on that
ad-hoc builtin that had to be inserted in several checkers.
Inferbo & pulse used that information meaningfully and had to undergo
some minor changes to cope with the new metada instruction.
Reviewed By: ezgicicek
Differential Revision: D14833100
fbshipit-source-id: 9b3009d22
Summary:
Bundle all non-semantic-bearing instructions into a `Metadata _`
instruction in SIL.
- On a documentation level this makes clearer the distinction between
instructions that encode the semantics of the program and those that are
just hints for the various backend analysis.
- This makes it easier to add more of these auxiliary instructions in
the future. For example, the next diff introduces a new `Skip` auxiliary
instruction to replace the hacky `ExitScope([], Location.dummy)`.
- It also makes it easier to surface all current and future such
auxiliary instructions to HIL as the datatype for these syntactic hints
can be shared between SIL and HIL. This diff brings `Nullify` and
`Abstract` to HIL for free.
Reviewed By: ngorogiannis
Differential Revision: D14827674
fbshipit-source-id: f68fe2110
Summary: In SIL, sometimes a return value is assigned to `__return_param`.
Reviewed By: ezgicicek, mbouaziz
Differential Revision: D14538590
fbshipit-source-id: dfbb74dc2
Summary:
This will be used in the future to determine what to do with destructors
in pulse.
Reviewed By: mbouaziz
Differential Revision: D14324759
fbshipit-source-id: bc3c34471
Summary:
This seems generally useful. Force people to do it in the future even if
they want to avoid having to update the frontend tests.
Reviewed By: mbouaziz
Differential Revision: D14324758
fbshipit-source-id: cdef3f72a
Summary:
You can only take the address of variables, field accesses, and array
accesses, the rest doesn't make sense.
Reviewed By: mbouaziz
Differential Revision: D14258484
fbshipit-source-id: 8ddcfe810
Summary:
Add an option to specify some classes that we really want to warn about
with the liveness checker, even when they appear used because of the
implicit destructor call inserted by the compiler.
Reviewed By: mbouaziz
Differential Revision: D13991129
fbshipit-source-id: 7fafdba84
Summary: It extends the abstract location for C string length, i.e., the first index of the null character in character array.
Reviewed By: mbouaziz
Differential Revision: D13634241
fbshipit-source-id: d2727d5f5
Summary: Publish solutions to the lab, and a Docker file and image to get started more quickly with infer hacking.
Reviewed By: mbouaziz
Differential Revision: D13648847
fbshipit-source-id: daf48ad03
Summary:
In ObjC there are no access modifiers. The strongest alternative is to put methods in the implementation but omit them from the interface declaration.
Put exported ObjC methods in their own field in the class structure and use that in RacerD to decide whether to report on the method.
Reviewed By: mbouaziz
Differential Revision: D13597504
fbshipit-source-id: c4a3d2705
Summary:
- `Printer.NodesHtml.start_node` prints the instructions rather than doing it in the callee
- use color class for `<LISTING>` rather than wrapping them in `<span>` (also fixes a wrong nesting between `<LISTING>` and `<span>`)
- `Summary.pp_html` is always `Black`
- New line before `<hr>` and `<LISTING>`
- `Io_infer.Html.create` takes a `SourceFile.t` rather than a `path_kind`
- typo
Reviewed By: jvillard
Differential Revision: D13572247
fbshipit-source-id: 65f57df25
Summary:
Record per-location traces. Actually, that doesn't quite make sense as a
location can be accessed in many ways, so associate a trace to each
*edge* in the memory graph. For instance, when doing `x->f = *y`, we
want to take the history of the `<val of y> --*--> ..` edge, add "assigned
at location blah" to it and store this extended history to the edge
`<val of x> --f--> ..`.
Use this machinery to print nicer traces in `infer explore` and better
error messages too (include the last assignment, like biabduction
messages).
Reviewed By: da319
Differential Revision: D13518668
fbshipit-source-id: 0a62fb55f
Summary:
This will be useful to make the analysis more precise. In particular, it
allows a disjunctive version of pulse to deal will deleting vector
elements in a loop: without this, deleting an array element in one
iteration will make the analysis think that the next array element is
invalid too since they are all the same. By keep track of the index, we
can detect when we are sure that two elements are the same and only
report in that case.
Reviewed By: ngorogiannis
Differential Revision: D13431374
fbshipit-source-id: dae82deeb
Summary:
Some code calls `this->~Obj()` then proceeds to use fields in the current
object, which previously we would report as invalid uses. Assume people know
what they are doing and ignore destructor calls to `this`.
Reviewed By: mbouaziz
Differential Revision: D13401145
fbshipit-source-id: f6b0fb6ec
Summary:
`AccessExpression.t` represents array accesses as `ArrayOffset of t * Typ.t * t
list`, i.e. the index is represented by a list of access expressions. This is
not precise enough when indices cannot be represented as such. In fact, in
general any `HilExp.t` can be an array index but this type was an approximation
that was good enough for existing checkers based on HIL.
This diff changes the type of access expressions to be parametric in the type
of array offsets, and uses this to record `HilExp.t` into them when translating
from SIL to HIL.
To accomodate the option of not caring about array offsets
(`include_array_indexes=false`), the type of array offsets is an option type.
Reviewed By: mbouaziz
Differential Revision: D13360944
fbshipit-source-id: b01442459
Summary:
`AccessExpression.t` and `HilExp.t` are about to become mutually
recursive, this will help distinguish the actual changes from the moving
of code around.
This deletes the file left around in the previous commit to preserve
callers of `AccessExpression`.
Reviewed By: mbouaziz
Differential Revision: D13377645
fbshipit-source-id: 71338d1f3
Summary:
`AccessExpression.t` and `HilExp.t` are about to become mutually
recursive, this will help distinguish the actual changes from the moving
of code around.
Reviewed By: mbouaziz
Differential Revision: D13377644
fbshipit-source-id: 9d6f290b6
Summary:
This will be useful for proper support of array indexes in pulse and in
HIL in general.
Reviewed By: mbouaziz
Differential Revision: D13377642
fbshipit-source-id: e431121fb
Summary:
It's useful for checkers to know when variables go out of scope to
perform garbage collection in their domains, especially for complex
domains with non-trivial joins. This makes the analyses more precise at
little cost.
This could have been added as a custom function call to a builtin, but I
decided against it because this instruction doesn't have the semantics
of any function call. It's better for each checker to explicitly not
deal with the custom instruction instead.
Reviewed By: jberdine
Differential Revision: D13102951
fbshipit-source-id: 33be22fab
Summary:
Before, the liveness pre-analysis would place extra instructions in the
CFG for either:
1. marking an `Ident.t` as dead, or
2. marking a `Pvar.t` as `= 0`
But we have no way of marking pvars dead without setting them to 0. This
is bad because setting pvars to 0 is not possible everywhere they are
dead. Indeed, we only do it when we haven't seen their address being
taken anyway. This prevents the following situation, recorded in our tests:
```
int address_taken() {
int** x;
int* y;
int i = 7;
y = &i;
x = &y;
// if we don't reason about taken addresses while adding nullify instructions,
// we'll add
// `nullify(y)` here and report a false NPE on the next line
return **x;
}
```
So we want to mark pvars as dead without nullifying them. This diff
extends the `Remove_temps` SIL instruction to accept pvars as well, and
so renames it to `ExitScope`.
Reviewed By: da319
Differential Revision: D13102953
fbshipit-source-id: aa7f03a52
Summary:
It enables the translation of casting expression. As of now, it
translates only the castings of pointers to integer types, in order to
avoid too much of change, which may mess the checkers up.
Reviewed By: jvillard
Differential Revision: D12920568
fbshipit-source-id: a5489df24
Summary:
Useful to understand the changes in the pre-analysis, or to inspect the
CFG that checkers actually get.
This means that the pre-analysis always runs when we output the dotty,
but I don't really see a reason why not. In fact, we could probably
*always* store the CFGs as pre-analysed.
Reviewed By: mbouaziz
Differential Revision: D13102952
fbshipit-source-id: 89f3102ec
Summary: The aligned width of bool should be 1-byte, while the range of bool [0,1].
Reviewed By: jvillard
Differential Revision: D12932394
fbshipit-source-id: be1a5d6d1
Summary:
When initialising a variable via semi-exotic means, the frontend loses
the information that the variable was initialised. For instance, it
translates:
```
struct Foo { int i; };
...
Foo s = {42};
```
as:
```
s.i := 42
```
This can be confusing for backends that need to know that `s` actually
got initialised, eg pulse.
The solution implemented here is to insert of dummy call to
`__variable_initiazition`:
```
__variable_initialization(&s);
s.i := 42;
```
Then checkers can recognise that this builtin function does what its
name says.
Reviewed By: mbouaziz
Differential Revision: D12887122
fbshipit-source-id: 6e7214438
Summary:
The upcoming ocamlformat has the ability to parse and format
docstrings. This requires that the docstrings conform to the ocamldoc
spec a bit more strongly. If a docstring does not parse, it is left
alone, but if it is morally ill-formed but parses by chance, it can be
reformatted incorrectly. This patch fixes the existing instances of
this problem.
Reviewed By: mbouaziz
Differential Revision: D12911937
fbshipit-source-id: 1c2eb590b
Summary:
In order to know whether a global variable is an integral constant
expression in C, this diff adds a field for the results of isInitICE.
The controller you requested could not be found.: facebook-clang-plugins
Reviewed By: mbouaziz
Differential Revision: D12838521
fbshipit-source-id: 388bff1f3
Summary: Make the whole type private, introduce constructors for each variant, and deal with the consequences.
Reviewed By: da319
Differential Revision: D12825810
fbshipit-source-id: a01922812
Summary:
Seems useful to know when we're printing one instruction only, but not when we
print lots of them for readability.
Reviewed By: mbouaziz
Differential Revision: D12823481
fbshipit-source-id: 2beb339f2
Summary:
Now the domain can reason about `&` and `*` too. When recording `&`
between two locations also record a back-edge `*`, and vice-versa.
Reviewed By: mbouaziz
Differential Revision: D10509335
fbshipit-source-id: 8091b6ec0
Summary: To avoid reporting on private methods, ignore those starting with underscore. Other cleanups.
Reviewed By: jvillard
Differential Revision: D10558970
fbshipit-source-id: 0572f1e70
Summary: This diff changes pp of binary operation condition in order to avoid a `make test` failure. For the same `uint64_t` type, it is translated to `unsigned long long` in 64bit mac, but `unsigned long` in 64bit linux, which made a `make test` failure.
Reviewed By: mbouaziz
Differential Revision: D10459466
fbshipit-source-id: 449ab548e
Summary:
Model `x[y]` and `x.push_back(i)` to catch the classic bug of "take
reference inside vector, invalidate, then use again".
Reviewed By: da319
Differential Revision: D10445824
fbshipit-source-id: 21ffd9677
Summary:
It gets built-in integer type widths of C from the clang plugin. For Java, it uses fixed widths.
The controller you requested could not be found.: facebook-clang-plugins
Reviewed By: jvillard
Differential Revision: D10397409
fbshipit-source-id: 73958742e
Summary:
Store the correct version of the proc desc into the DB when specialising
it. This doesn't seem to be used but is useful for investigating after
the fact (eg, if we could print individual cfgs).
Reviewed By: mbouaziz
Differential Revision: D10380708
fbshipit-source-id: fd72dbfc2
Summary:
Use same code for deciding whether two accesses conflict across java/clang, by adapting that of the clang version.
Eliminate/simplify some code.
Reviewed By: mbouaziz, jberdine
Differential Revision: D10217383
fbshipit-source-id: dc0986d05
Summary: It enables placement_new to get three parameters, which happens when placement_new is overloaded (e.g. Boost).
Reviewed By: mbouaziz
Differential Revision: D10100324
fbshipit-source-id: 0ecb0a404
Summary:
Load proc descs from the "procedures" sqlite table instead of from
file-wide cfgs stored in the "source_files" table. This removes the need
for a cache of these file-wide CFGs, which was needed because loading
them is expensive and potentially needed in case we need to load the
proc descs of several procedures in the same file. Now we can just load
the proc descs one by one and not worry about caching.
Reviewed By: jberdine
Differential Revision: D10173355
fbshipit-source-id: 665636121
Summary:
Fix the logic for computing duplicate symbols. It was broken at some point and some duplicate symbols creeped into our tests. Fix these, and add a test to avoid duplicate symbols detection to regress again.
Also, this removes one use of `Cfg.load`, on the way to removing file-wide CFGs from the database.
Reviewed By: ngorogiannis
Differential Revision: D10173349
fbshipit-source-id: a0d2365b3
Summary:
First step: record the proc desc of each procedure in the "procedures"
table. Update them according to the attributes logic. Bonus: this
proc-desc for a procedure is now always in sync with its attributes.
For now nothing uses these per-procedure cfgs. Later diffs make more and
more use of them and eventually kill off file-wide CFGs from the
database.
Reviewed By: jberdine
Differential Revision: D10173350
fbshipit-source-id: b6d222bee
Summary:
There's nothing to analyse for declared procedures, and if there is then
that's because they are defined outside the source file and should not
be analysed unless ondemand needs them.
Reviewed By: ngorogiannis
Differential Revision: D10173353
fbshipit-source-id: 39c42eb7a
Summary:
In a future commit `Attributes` will depend on `Procdesc` and that
creates a cycle for the functions concerned with specialising proc
descs, which need `Attributes`.
Reviewed By: jberdine
Differential Revision: D10173354
fbshipit-source-id: 6c4ff82f0
Summary: The Nullsafe checker integration is filtering out the pre-existing warnings based on the bug hash only. However, there was a typo in the regexp and the bug hash for methods in anonymous classes was then depending on the name (in the bytecode) of the anonymous class, i.e. depending on the `N` in `ClassName$N.methodName()` where `N` is the occurrence of the anonymous class in `ClassName`. As a consequence, introducing a new anonymous class in a file was leading to all the reports in the subsequent anonymous classes to be marked as introduced.
Reviewed By: jberdine
Differential Revision: D10186651
fbshipit-source-id: 42e27c132
Summary:
Instead of many successive implicit transactions to write each
attributes of the procedures in a file, write them all in a single
transaction.
Reviewed By: jberdine
Differential Revision: D10173351
fbshipit-source-id: 5f2a5ffb5
Summary: It uses big int, instead of 63bits int of OCaml, in the interval domain in order to get preciser numeric values in the future.
Reviewed By: jvillard
Differential Revision: D10123364
fbshipit-source-id: c217f4366
Summary:
Before storing attributes to disk, we fix their location information if needed.
Ideally we wouldn't be creating bogus attributes but sometimes the frontends
are built in a way that makes it difficult to do otherwise, thus we have to
live with this. However, what's aggravating is that attributes are also saved
in the proc descs of these procedures but in their wrong version. This makes
the two versions (inside the procedures sqlite table and inside the procdesc in
the cfg of the source_files table) agree.
Reviewed By: jeremydubreil
Differential Revision: D10084708
fbshipit-source-id: 5bfd5da3a
Summary:
Callsites of `Reporting.log_error/warning` always use `Exceptions.Checkers`, let's simplify the API.
Under the hood it still creates an exception, but this can be cleaned up later.
Reviewed By: jeremydubreil
Differential Revision: D9799860
fbshipit-source-id: 6492a60b4
Summary:
The constructor `` `Typ`` is never used to build values. Removing type
substitutions from Sil.ml had knock-on effect on Typ.ml etc., resulting in more
deleted code around type substitutions \o/
Reviewed By: mbouaziz
Differential Revision: D9769340
fbshipit-source-id: 509cbd284
Summary:
- Let's call `IssueType.from_string` once only
- Use properly defined issue types for builtin linters
Reviewed By: martinoluca
Differential Revision: D9654105
fbshipit-source-id: 947b50a51
Summary:
Now that we got rid of dummy nodes used non-dummily (biabduction state, reporting), `pname` don't need to be an option anymore.
Let's save a boxing on all nodes.
Reviewed By: jeremydubreil
Differential Revision: D9654152
fbshipit-source-id: 83b00f239
Summary: No dummy node key, as a consequence the option `--skip-duplicated-types` will have no effect on issues with no node key, i.e. issues reported by non-biabduction non-eradicate checkers.
Reviewed By: martinoluca
Differential Revision: D9633564
fbshipit-source-id: 9ff8abf21
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:
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: 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:
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: 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: `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:
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:
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
Summary: Do not start with an invalid source file when we can avoid it. Follow up from D8418447.
Reviewed By: jeremydubreil
Differential Revision: D8732168
fbshipit-source-id: 28a183b
Summary: Otherwise the dead code checker sometimes crashes with a not-totally-related error.
Reviewed By: mbouaziz
Differential Revision: D8732546
fbshipit-source-id: 65caabd
Summary: Trying to convert a large int literal to an OCaml int raises an exception. The use case here actually needed a float anyway, so add an API for that.
Reviewed By: jeremydubreil
Differential Revision: D8550410
fbshipit-source-id: 382495b
Summary: Removing an internal error in SIL to HIL translation which I had added before to log how often the particular case was happening. It happens quite often, and I have a task to investigate the issue. Removing it as it spams the analysis output a lot.
Reviewed By: dulmarod
Differential Revision: D8316822
fbshipit-source-id: 4047cbe
Summary:
`make doc` will use `jbuilder` (which in turn uses `odoc`) to generate the
documentation for infer's modules. This is useful to browse the APIs of infer
and gives a more discoverable place to host more general documentation about
infer's internals.
Besides the actual plumbing necessary to generate the docs, this diff also
- Moves the various infer/src/*/README.md to index.mld files that make it to the generated docs
- Fixes some doc comments that would anger `ocamldoc`
Closes#435
Reviewed By: mbouaziz
Differential Revision: D8314572
fbshipit-source-id: 4a5c70e
Summary: We get a lot of false positives for union types as union fields are treated as separate memory locations at the moment. For now we do not treat union fields as uninitialised.
Reviewed By: mbouaziz
Differential Revision: D8277363
fbshipit-source-id: efe5b4a
Summary:
Change the license of the source code from BSD + PATENTS to MIT.
Change `checkCopyright` to reflect the new license and learn some new file
types.
Generated with:
```
git grep BSD | xargs -n 1 ./scripts/checkCopyright -i
```
Reviewed By: jeremydubreil, mbouaziz, jberdine
Differential Revision: D8071249
fbshipit-source-id: 97ca23a
Summary:
For now: just moving this list behind an abstract type.
Next: changing the internal representation.
Reviewed By: ngorogiannis
Differential Revision: D8140926
fbshipit-source-id: 5b959b0
Summary:
Append can be costly, let's do it once only.
Depends on D8185619
Reviewed By: jeremydubreil
Differential Revision: D8185634
fbshipit-source-id: 67f84a9
Summary:
- do not `List.rev` for `List.last`
- `List.rev_filter_map` rather than `filter |> map |> rev`
Reviewed By: da319
Differential Revision: D8185619
fbshipit-source-id: aeb41a4
Summary: The order of nodes means nothing, and should not matter, let's save the whales!
Reviewed By: ngorogiannis
Differential Revision: D8182137
fbshipit-source-id: bc14a2c
Summary:
Moving away from C++ include-based models means that we cannot reliably detect
anymore whether a file includes <iostream> or not. In order not to be too
spammy, let's always assume standard streams are initialized for now when the
include models are off.
Recent versions of libstdc++ make these models redundant so there is hope that in a
bright future the analysis of std streams initialisation will work correctly without infer
having to have its own models anyway.
Reviewed By: mbouaziz
Differential Revision: D8043467
fbshipit-source-id: d118043
Summary: The type of array element is not preserved correctly in the translation from SIL to HIL. When array element is passed by a reference, i.e. `f(&(array[0]))`, the type of array element gets the type of a pointer of array element.
Reviewed By: jvillard
Differential Revision: D8071188
fbshipit-source-id: 3e6635e
Summary: Use AccessExpressions instead of AccessPath in uninit analysis. This will allow us to distinguish between pointers and their dereferences.
Reviewed By: jvillard
Differential Revision: D8042359
fbshipit-source-id: 604bcbc
Summary: Moving this function since it's about a single procdesc. Slight rewrite too.
Reviewed By: da319
Differential Revision: D8030494
fbshipit-source-id: f7cc58e
Summary:
This diff:
- translates C++ `catch` blocks
- adds an exceptional control-flow edge from the end of a `try` block to the beginning of a `catch` block
This obviously doesn't reflect the way exceptions actually work, but I think it is better than what we have now. For one thing, we'll see/translate code inside `catch` blocks, which were opaque before. If Clang analyses don't want this behavior, they can simply use `ProcCfg.Normal` (which, up until this diff, behaved identically to `ProcCfg.Exceptional`.
In the future, we can extend `trans_state` to track blocks that might throw an exception, and have each of these blocks transition to `catch` instead.
Reviewed By: jvillard
Differential Revision: D7814521
fbshipit-source-id: 67b86a6
Summary:
Previously, the type of `trans_result` contained a list of SIL expressions.
However, most of the time we expect to get exactly one, and getting a different
number is a soft(!) error, usually returning `-1`.
This splits `trans_result` into `control`, which contains the information
needed for temporary computation (hence when we don't necessarily know the
return value yet), and a new version of `trans_result` that includes `control`,
the previous `exps` list but replaced by a single `return` expression instead,
and a couple other values that made sense to move out of `control`. This allows
some flexibility in the frontend compared to enforcing exactly one return
expression always: if they are not known yet we stick to `control` instead (see
eg `compute_controls_to_parent`).
This creates more garbage temporary identifiers, however they do not show up in
the final cfg. Instead, we see that temporary IDs are now often not
consecutive...
The most painful complication is in the treatment of `DeclRefExpr`, which was
actually returning *two* expressions: the method name and the `this` object.
Now the method name is a separate (optional) field in `trans_result`.
Reviewed By: mbouaziz
Differential Revision: D7881088
fbshipit-source-id: 41ad3b5
Summary:
This is an attempt to make things more consistent, and maybe save some work
from the `Format` module in case flambda doesn't have our backs.
Reviewed By: jberdine
Differential Revision: D7775496
fbshipit-source-id: 59a6314
Summary:
One source of non-determinism is racing on procedure summaries when reporting. In particular, the summary of a method may be computed and stored by one thread, but another may be trying to report on it (eg, in cluster checkers).
One solution (at least until everything is in sqlite) is to have separate files just for the reports, a la linters. This diff improves the interface of LintIssues and generalises it ahead of using it in other analysers.
Reviewed By: jeremydubreil
Differential Revision: D7859973
fbshipit-source-id: 8672d3b
Summary:
This simplifies the frontends and backends in most cases. Before this diff,
returning `void` could be modelled either with a `None` return, or a dummy
return variable with type `Tvoid`. Now it's always the latter.
Reviewed By: sblackshear, dulmarod
Differential Revision: D7832938
fbshipit-source-id: 0a403d1
Summary: Returning the list of sub-expressions is not right and can cause assertion failures elsewhere in the frontend.
Reviewed By: dulmarod
Differential Revision: D7813493
fbshipit-source-id: 33ac9c1
Summary:
Add warning 60 (unused module) to the list of fatal warnings. Whitelisting
modules at toplevel is tricky (see inline comments) but doable.
Reviewed By: mbouaziz
Differential Revision: D7790073
fbshipit-source-id: 6f591c4
Summary:
Add a `--source-files` option to `infer explore` to print information about the source files captured by infer.
More precisely, `infer explore --source-files` will print each row of the "source_files" table in the results database.
Option `--source-files-filter` can be used to filter output to file names matching an SQLite "LIKE" pattern.
Flags `--source-files-cfgs`, `--source-files-type-environment`, `--source-files-procedure-names` and `--source-files-freshly-captured` control which columns to print.
The printers for some existing types have been tweaked to improve the output.
Reviewed By: jvillard
Differential Revision: D7735535
fbshipit-source-id: 572389a
Summary:
When looking at large CFGs, at least in `xdot`, it's often difficult to find
the procedure you're looking for. Sorting the proc names puts them in
alphabetical order, which makes searching one procedure easier.
Reviewed By: mbouaziz
Differential Revision: D7758521
fbshipit-source-id: 8e9997f
Summary:
[This is a stepping stone before moving the specs data to sqlite.]
Previously, things worked like this (ignore ObjC):
1. capture & analyse C models
2. capture & analyse C++ models
3. copy C *.specs files to lib/specs/
4. copy C++ *.specs files to lib/specs/
Now it works like this:
1. capture C models
2. capture C++ models
3. analyse both together
4. install *.specs files to lib/specs
Reviewed By: sblackshear
Differential Revision: D7639322
fbshipit-source-id: 58d7c53
Summary:
Upgrade ocamlformat, and base which needs to be done in sync in order to build
ocamlformat, and the other deps can come for the ride.
Reviewed By: jvillard
Differential Revision: D7663537
fbshipit-source-id: 3e90970
Summary: We already suppress race reports if the field is marked in this way; makes sense to do the same thing for these reports.
Reviewed By: ngorogiannis
Differential Revision: D7589275
fbshipit-source-id: 8f0aeab
Summary:
We were using the "filename" as the key because it's (kinda) unique *and* human
readable, but with the `infer explore --procedures` interface we don't really
need the human readable part anymore, so we can just use the OCaml marshalling
of the pname as the key. The human-readable version (sans unique-fying hash) is
now another column in the table, used to match procedure names in
`--procedures-filter`.
Reviewed By: sblackshear
Differential Revision: D7639158
fbshipit-source-id: e714605
Summary:
Add a `--procedures` option to `infer explore` to print information about the
procedures captured by infer. More precisely, `infer explore --procedures` will
print each row of the "procedures" table in the results database. A new
`--procedures-filter` controls which procedures to print information about, and
there is one flag per column in the db too to print more or less options about
each procedure (in particular, we can now print attributes), with some defaults.
Reviewed By: sblackshear
Differential Revision: D7639062
fbshipit-source-id: 034a2b8
Summary:
- Less `^`
- `pp_print_string` instead of `F.fprintf fmt "%s"`
- and stuff like that
Reviewed By: jvillard
Differential Revision: D7607336
fbshipit-source-id: 5d985ef
Summary:
This is unused, but the logic for keeping its value up to date was still alive
and kicking.
I don't know that making use of this flag would be easy: we could just use it
in `Ondemand.should_analyze`, but it would be unsound because a procedure might
need to be re-analysed becauese its dependencies have changed. Since there's
not code to deal with that currently I think it's best to remove it and
re-introduce it when we have some idea how to use it.
Reviewed By: sblackshear, jeremydubreil
Differential Revision: D7444179
fbshipit-source-id: 99a1ec5
Summary:
Report nullable inconsistencies by relying on the bytecode, and not on the presence of analysis summary on disk.
This use the `--external-java-packages` to avoid reporting inconsistencies outside of the codebase.
Reviewed By: sblackshear
Differential Revision: D7481101
fbshipit-source-id: 281135d
Summary:
There's actually a nice separation between IR/, base/, istd/, and the rest of
infer, so they can be made into separate jbuilder libraries so that the
separation remains. This helps make sense of the infer codebase.
Also:
- move everything biabduction-related out of backend/ and into a new
biabduction/ directory. This clarifies the current situation where backend/
contains a mix of analysis-independent code (still there now), and
biabduction-specific code (moved to biabduction/).
- move everything from base/ that is not infer-specific into istd/, e.g. IList.ml
- kill unused `FbTraceCalls`
- A couple of files needed to move around to complete the separation of base/ and IR/
Reviewed By: mbouaziz
Differential Revision: D7381842
fbshipit-source-id: cd86dea
Summary:
Limit the scope of what gets included into IStd.ml to only values that we want
to shadow. New values go into other files.
Also, build istd/ with `Core` open.
Reviewed By: mbouaziz
Differential Revision: D7382111
fbshipit-source-id: 969f0e8
Summary:
There's no real reason not to use `Core` lists in this module. Changed the
interface to be more `Core`-like. Changed the `*_changed` functions to use a
ref to track changes instead of passing the changed state around.
Reviewed By: mbouaziz
Differential Revision: D7123211
fbshipit-source-id: b27791a
Nikos Gorogiannis
Jules Villard
Sungkeun Cho
Martin Trojer
Radu Grigore
Mehdi Bouaziz
Ezgi Çiçek
Josh Berdine
Jeremy Dubreil
David Lively
Nikos Gorogiannis
Daiva Naudziuniene
Julian Sutherland
Martino Luca
Dulma Churchill
Sam Blackshear
Oliver Castillo
Frédéric Bour