Summary:
I found it very confusing that running infer with --debug makes the
report to be different.
Intuitively, I expect (and I think majority of users would expect) that
`--debug` makes things more verbose (and potentially more slow / consuming
more memory and disk space), but does not change anything apart from it.
One pro of preserving existing behavior, pointed by jvillard:
- Suppose some check is experimental or disabled in the config. The
users expect the issue to be found, but it does not show up. They run
`infer --debug` to understand the behavior, and suddenly the issue shows
up.
I, hovewer, find this pro not important enough and potentially confusing
the users even more.
(If they want to investigate seriously, they can always use
--no-filtering, and there are a lot of cases when the issue does not
show up for others, much hard to undertand reasons, than the fact that
it is disabled).
Reviewed By: jvillard
Differential Revision: D17113750
fbshipit-source-id: 46cc93503
Summary:
Not everything is here yet, and there is some confusion on what to do
about the size values. However, the semantics has the right general
shape and will be a nice starting point for thinking about the details.
Reviewed By: jberdine
Differential Revision: D17111041
fbshipit-source-id: cc75651c6
Summary:
The purpose of DefinitelyNotNullable currently is bit unclear; let's
rename it so that the intention is obvious.
Reviewed By: artempyanykh
Differential Revision: D16984529
fbshipit-source-id: 696d58315
Summary:
`nullsafe` currently allows the following:
```
public void Nonnull Object willBeOK() { return null; }
```
But disallows the following:
```
public void Object willBeAnIssue() { return null; }
```
This was a deliberate choice made back in 2014.
The motivation was to provide a way to tell the checker "I know it can not be null, trust me".
A huge problem with that approach is that it is extremely non-intuitive and surprising, and contradicts with pretty much everything when Nonnull or similar annotations are used in external world.
This is not the way how checkers should be supressed.
We do provide 2 options to express this intention, namely `assertNotNull` and `assumeNotNull` would do the thing.
This is a much better approach for additional reason: assertNotNull is
granular and applies only to the exact expression that is under
question. In contrast, suppressing the check on the whole function level
make any modifications of a function dangerous.
Reviewed By: artempyanykh
Differential Revision: D16984213
fbshipit-source-id: 0ba0f623b
Summary:
The translation from LLVM to llair now builds expressions up across
blocks, following the implementation. This is easy to do because of the
dominance restrictions in SSA, but might be difficult to reason
about.
Reviewed By: jberdine
Differential Revision: D17111040
fbshipit-source-id: a8e99147d
Summary:
This diff revises some models of Java String.
They had been implemented by C's string models such as models of
`strlen` or `strcat`, however, Java's String is different to C's,
rather is similar to C++'s String object.
Reviewed By: ezgicicek
Differential Revision: D17093136
fbshipit-source-id: b4f2cb4d0
Summary:
LLVM and llair have similar memory models, and we don't want to
duplicate any definitions or theorems. This adds a new memory model
theory which should be understandable in its own right. A heap is a
mapping from addresses to bytes, alongside a set of valid addresses, and
intervals that have been allocated already. Primitives are defined for
allocating and de-allocating as well as reading and writing chuncks of
bytes.
There is also a generic type of structured values, and functions for
converting them to/from byte arrays.
Reviewed By: jberdine
Differential Revision: D17074470
fbshipit-source-id: bdab6089f
Summary: Numeric attribute ranks are getting confused with addresses. Add an option (false by default) to MakePPUniqRankSet which prevents printing of the ranks.
Reviewed By: jvillard
Differential Revision: D17094269
fbshipit-source-id: 353c52fca
Summary:
`from_string` is too benign in constrast with what this method is really
doing (and oh my what it is really doing).
There are a lot of potential follow ups to clean this up even more, but
this is beyond the scope of this diff
Reviewed By: jvillard
Differential Revision: D17070826
fbshipit-source-id: 3d190039e
Summary:
In some cases inlining pure expressions into their use sites causes
code blowup. This diff changes the frontend to inline expressions only
if there is a single use, and otherwise adds a move instruction.
Reviewed By: ngorogiannis
Differential Revision: D17071770
fbshipit-source-id: d866a0622
Summary:
`__inferbo_empty`, `__inferbo_min`, and `__inferbo_set_size` were in the
"include-based" cpp model.
Reviewed By: jvillard
Differential Revision: D17072034
fbshipit-source-id: dd840331f
Summary:
This diff uses the models of vector for modelling string in Cpp.
Depends on D16963153
Reviewed By: ezgicicek
Differential Revision: D16963166
fbshipit-source-id: 5effe2d72
Summary:
This is more powerful than `"symbols"` for more advanced use-cases. Keep
`"symbols"` unchanged to make migrating easier.
Differential Revision: D16985756
fbshipit-source-id: dfbb09393
Summary:
This has been out of date since arithmetic was changed from a purely
uninterpreted treatment to having a solver.
Reviewed By: jvillard
Differential Revision: D16985159
fbshipit-source-id: 39e42069c
Summary:
While SSA can be useful for code transformation purposes, it offers
little for semantic static analyses. Essentially, such analyses
explore the dynamic semantics of code, and the *static* single
assignment property does not buy much. For example, once an execution
visits a loop body that assigns a variable, there are multiple
assignments that the analysis must deal with. This leads to the need
to treat blocks as if they assign all their local variables, renaming
to avoid name clashes a la Floyd's assignment axiom. That is fine, but
it makes it much more involved to implement a version that is
economical with respect to renaming only when necessary. Additionally
the scoping constraints of SSA are cumbersome and significantly
complicate interprocedural analysis (where there is a long history of
incorrect proof rules for procedures, and SSA pushes the
interprocedural analysis away from being able to use known-good
ones). So this diff changes Llair from a functional SSA form to a
traditional imperative language.
Reviewed By: jvillard
Differential Revision: D16905898
fbshipit-source-id: 0fd835220
Summary:
Before this diff symbolic execution of instructions assumed that
assigned variables were unconstrained in the precondition. This is
ensured by symbolic execution of control flow, which renames all local
variables of a block when it is entered.
This diff changes symbolic execution of instructions to rename
modified variables that appear in the precondition when necessary, and
accounts for the modified variable occurrence condition on the frame
rule. This will enable more economically renaming variables, as most
of the time it is not needed.
Reviewed By: jvillard
Differential Revision: D16905893
fbshipit-source-id: 3a53525d7
Summary:
Each variable now contains its type, alongside its name. This is more
uniform than in LLVM, where the name is usually paired with a type, but
not always, for example, the register type of the result of an
extractvalue is left implicit.
Reviewed By: jberdine
Differential Revision: D16984630
fbshipit-source-id: 1c3bc4985
Summary:
HOL now lets us omit quotations on Datatypes and make them look more
like the other new-style HOL definitions.
Reviewed By: jberdine
Differential Revision: D16983934
fbshipit-source-id: f8ef3abb5
Summary:
This sketches out how translation can be approached. It is partially
based on the Sledge code.
For basic blocks, isn't based on the Sledge code, but just my own
thoughts as a starting point. Essentially, we are trying to build up
larger expressions, and so not assigning to temporary registers that
don't live past the end of the block. This does remove sharing, so a
fancier approach could check for multiple uses of end-of-block dead
registers, or look at the sizes of expressions. The approach should be
flexible enough to accommodate such changes.
Fix icmp syntax
Using finite maps is elegant in the semantics, but awkward for writing
the translation function. Refactor the mappings from labels to functions
and from labels to blocks to use association lists instead.
To remove phi nodes, the translation takes every edge in the control
flow graph and makes a new basic block that contains a single parallel
move instruction that corresponds to the action of the phi node of the
target block.
Reviewed By: jberdine
Differential Revision: D16831051
fbshipit-source-id: 005663e26
Summary:
The AST is not complete on expressions, but it should have most of the
important features.
The representation is in some ways very different from the OCaml
implementation, because the OCaml code uses mutation to build the CFG as
an actual pointer graph in memory, and also because the expression
representation is optimised for the backend. For the former, it should
be easy to see that the AST here is isomorphic, representing the CFG
with finite maps from block labels. The correspondence is less clear in
the latter case, but the point here is not to model or verify
implementation optimisations, but to give a semantics to llair as a
language.
Reviewed By: jberdine
Differential Revision: D16807132
fbshipit-source-id: b0f64b3ec
Summary: Adding new predicate for checking whether a variable is defined as extern. May be useful in AL rules.
Reviewed By: jvillard
Differential Revision: D16961690
fbshipit-source-id: 0677077dc
Summary:
The clang frontend has bugs. When a bug we know about happens some
exception is raised and, most of the time, logged away so as not to
crash the whole process. This catching of exceptions wasn't done from
testDeterminator so it could crash where capture didn't. This diff wraps
the crashy function in test determinator to avoid that.
Reviewed By: ngorogiannis
Differential Revision: D16963178
fbshipit-source-id: 87a4ff70b
Summary:
This isn't really a "config" part of the frontend and this change is
needed later to catch these errors more robustly.
Reviewed By: ngorogiannis
Differential Revision: D16963177
fbshipit-source-id: 293b23acf
Summary: This diff prunes array sizes in Java by adding the size alias on the `get_array_length` function calls.
Reviewed By: ezgicicek
Differential Revision: D16983501
fbshipit-source-id: a924af09d
Summary:
Rename some AL source files so they mention AL explicitly instead of
"cFrontend" which could be confused with the clang frontend itself.
Reviewed By: ezgicicek
Differential Revision: D16962539
fbshipit-source-id: 29237cd1c
Summary: AL makes for close to a third of the source files in clang/. Put the code in its own folder for clarity.
Reviewed By: ezgicicek
Differential Revision: D16962438
fbshipit-source-id: 3373e69b9
Summary:
This diff avoids that an integer value is pruned to the bottom by
comparing to a pointer.
For example, before this diff,
assume((int*)x == p);
assume((int*)x != p);
where x is an integer, x is pruned to the bottom in both of the assume
cases. So, there were some, unintentional and false, unreachable
code.
Depends on D16960199
Reviewed By: ezgicicek
Differential Revision: D16964735
fbshipit-source-id: 90a3c8c80
Summary:
It changes the order of StdBasicString and StdVector for easier
reviewing of the following diff.
Reviewed By: ezgicicek
Differential Revision: D16963153
fbshipit-source-id: 50325e4e1
Summary:
The access path format forced some weird patterns on this code, simplify
using the access expression structure.
Reviewed By: ezgicicek
Differential Revision: D16960660
fbshipit-source-id: e8faf619e
Summary:
It prunes the size of collections when the size function is called in the condition expression. The diff extended the alias domain to understand temporary variables of SIL from Java.
Depends on D16761461
Reviewed By: ezgicicek
Differential Revision: D16761611
fbshipit-source-id: 849c5c71c
Summary:
This adds logging of the number of nodes in the reverse analysis call graph, and the number of these nodes that are invalidated by incremental analysis.
This data will show the precision of incremental analysis.
Reviewed By: ngorogiannis
Differential Revision: D16939101
fbshipit-source-id: 1e465f1a6
Summary:
It revises Java's cast model to keep type in the location when it has a field.
The type information is useful especially when generating ondemand values of Collection elements.
Depends on D16807299
Reviewed By: ezgicicek
Differential Revision: D16807378
fbshipit-source-id: 636e54429
Summary:
It doesn't make sense to use incremental analysis without specifying changed files. This is a possible source of future bugs.
This commit causes infer to die if incremental analysis is used without changed files.
---
Previously:
I think this code is currently a bit brittle because the CI shadow builds sometimes use `--incremental-analysis` because they are called with the same command as the diff analysis.
I am worried that in the future the shadow builds could be broken by this, although everything looks like it works right now. This diff would prevent breaking smoke builds in future because the shadow builds don't set changed-files (afaik).
However, possibly this is not the right place to fix the problem. It might be better to change the CI, I'm not sure.
Reviewed By: mityal
Differential Revision: D16829192
fbshipit-source-id: 5ee1ce9d0
Summary:
Use whatever information we can to decide whether to use C or Java
syntax when outputting an access expression, now that we store them as
such.
Also, make cluster callbacks explicitly set the language, as this was not done before and led to some confusion (Clang being set when analysing a Java file).
Reviewed By: skcho
Differential Revision: D16884160
fbshipit-source-id: 40adf9f35
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: Ideally, we should be able to handle them like pruning if statements but for now, let's add the test.
Reviewed By: skcho
Differential Revision: D16938842
fbshipit-source-id: 04fae9559
Summary:
It uses inline record for Loc.Field
Depends on D16807279
Reviewed By: ezgicicek
Differential Revision: D16807299
fbshipit-source-id: 45eab34a4
Summary: Checker configs were defined as tuples which are amenable to problems with wrong ordering. Let's make convert them to a record type to prevent such issues.
Reviewed By: jvillard
Differential Revision: D16936737
fbshipit-source-id: 32aad6e97
Mitya Lyubarskiy
Scott Owens
Sungkeun Cho
Ezgi Çiçek
Josh Berdine
Jules Villard
Dulma Churchill
Nikos Gorogiannis
Phoebe Nichols