Summary:
Remove Clang and Java submodules of Typ.Fieldname. They are unnecessary and they reflect a fake dichotomy: there is only one fieldname type. To distinguish between fields of Java classes and other C constructs, there is a helper function provided, but the idea is simple: obtain the class type the field belongs to, and check if it's a Java class.
This diff still preserves behaviour, but removes as many functions as possible from the interface, to leave a small surface.
Reviewed By: mityal
Differential Revision: D18962423
fbshipit-source-id: ffe6933ee
Summary: Guava uses assertions to ensure a future can be gotten without blocking (this means that if the future is not done, the app will crash). This diff teaches the starvation analyser about a number of such assertions, by treating them as assumes (since we don't care about exceptions).
Reviewed By: jvillard
Differential Revision: D18893427
fbshipit-source-id: 4d26a202b
Summary: A future is guaranteed not to block if `isDone()` has returned true first. Add logic for supporting that by remembering the objects that we have called `isDone` on and by making `assume` do the right thing with that knowledge. All this is achieved with the attribute domain.
Reviewed By: ezgicicek
Differential Revision: D18833901
fbshipit-source-id: 7f4ea0cd1
Summary:
When retrieving a value from a container, we previously had an arbitrary hack which would
- In java, give no ownership to the returned object (trying to be sound)
- In C++ give conditional ownership to the current method's first argument (trying to be complete, but doing it badly, as the first argument may not be the `this` object in a static method, or we might be accessing it through another parameter altogether).
Harmonise both by using the existing ownership of the container as ownership value for the returned object (leaning towards completeness).
Reviewed By: jvillard
Differential Revision: D18882800
fbshipit-source-id: f98f8d315
Summary:
Also add logic for recognising excessive timeouts. Refactor the code
around timeouts a little.
Reviewed By: artempyanykh
Differential Revision: D18807836
fbshipit-source-id: df5a1b566
Summary:
`Object.wait()` must be called on a locked monitor and it releases the
lock immediately, as far as other threads are concerned
(it also magically re-takes the lock when the monitor is `notified`).
Starvation can only occur if the UI thread is waiting
a lock that is distinct to that being waited on.
The check present was over-approximate in that it was checking that there exists a lock held by the UI thread and the thread issuing the `wait`, but did not make sure that lock was *not* the one waited on.
Amusingly, the e2e test was correct, but the reporting code wasn't.
Reviewed By: dulmarod
Differential Revision: D18782919
fbshipit-source-id: b3b98239e
Summary: Similar to constructor established attributes, we do the same here for static initializers. That is, attributes of static properties are injected into the initial state of every method.
Reviewed By: ezgicicek
Differential Revision: D18763192
fbshipit-source-id: 3879a27c5
Summary:
This diff enables parsing and auto-formatting documentation
comments (aka docstrings).
I have looked at this entire diff and manually made some changes to
improve the formatting. In some cases it looked like it would take too
much time, or benefit from someone more familiar with the code doing
it, and I instead disabled auto-formatting docstrings in those files.
Also, there are some source files where the docstrings are invalid,
and some where the structure detected by the parser appears not to
match what was intended. Auto-formatting has been disabled for these
files.
Reviewed By: ezgicicek
Differential Revision: D18755888
fbshipit-source-id: 68d72465d
Summary:
RacerD is angelic in the sense that when a method has no summary, no accesses are added to the symbolic state when we call that method. However, when the method returns an object we then proceed to access, this leads to non-angelic behaviour: if the object is assumed to be un-owned, then the accesses may lead to a race.
This manifests itself on Litho components, which are generated code without sources and thus RacerD has no CFG to analyse, and therefore produces no summary. The `Builder` patterns used in these classes are ubiquitous, and full of spurious races due to the fact that the returned objects, even though freshly allocated, are un-owned as far as RacerD knows.
Here, instead of going full-angelic and assuming that every method without a summary returns an owned object (which is a bit extreme), we try to model the `Builder` pattern wrt ownership. That is, static `create` methods returning `Builder` types are assumed to return full ownership; and, non-static methods of a `Builder` class which return the same type as their receiver are assumed to return conditional ownership.
Reviewed By: ezgicicek
Differential Revision: D18748423
fbshipit-source-id: bd53d4b67
Summary:
One standard way to schedule work is by starting a thread. We treat this by
- Treating invocations of `start` on a receiver with the `Runnable _` attribute as scheduling that runnable for parallel execution in the background (as opposed to on the UI thread).
- If `start` is used on an object of a subclass of `Thread` everything already works thanks to the `get_exp_attributes` function which will implicitly ascribe to an expression the attribute `Runnable _` if the expression points to an object with a known `run` method. This will even take care of some degree of dynamic dispatch, yay!
- If `start` is used on a `Thread` object which has been created with a constructor call provided with a `Runnable` argument, we have to appropriately model that constructor call, which is what is done in `do_call`.
Reviewed By: jvillard
Differential Revision: D18726676
fbshipit-source-id: 0bd83c28e
Summary:
A current blind spot is when object construction stores specific executors / runnables to object fields, which are then never mutated and accessed from normal methods. IOW the attributes established in the constructor are necessary to report properly inside a normal method (assuming these attributes are not invalidated by method code).
To achieve this, first we retain a subset of the final state attributes in the summary (only those that affect instance variables, in constructor methods). Then, when we analyse a non-constructor method:
- we analyse all constructors
- remove all attributes from the attribute map whose key is not an expression of the form `this.x. ...`
- re-localise all remaining keys so that they appear as rooted in the `this` local variable of the current procedure
- join (intersect) all such attribute maps
- use the result in place of initial state as far as the attribute map is concerned for the analysis of the current procedure, which can now start.
This means we can catch idioms that use side-effectful initialisation for configuring certain object fields like executors or runnables.
Reviewed By: jvillard
Differential Revision: D18707890
fbshipit-source-id: 42ac6108f
Summary: Another way to schedule work in android is by posting it to a `Handler`. A handler can be constructed out of the main looper, which forces it to schedule work on the UI thread. To model all this, we add syntactic models for getting the main looper and for creating handlers, and dataflow attributes for tracking that an expression is a looper/the main looper, or a handler constructed out of a looper.
Reviewed By: skcho
Differential Revision: D18706768
fbshipit-source-id: 7c46e6913
Summary:
Instead of trying to figure out what runnable is directly passed to an executor,
use attributes to track the flow of a runnable. This has several advantages:
- Can track runnables across function return values.
- Can somewhat overcome the information loss under dynamic dispatch.
- Unifies handling with other attributes.
Reviewed By: skcho
Differential Revision: D18672676
fbshipit-source-id: a06a0e6ff
Summary:
- Unify treatment of modelled and annotated executors by making things go through attributes.
- Add a return attribute to summaries, so that we can track flows of thread guards/executors/future stuff through returned values.
- Dispatch modeled functions to model summaries.
This will help in following diffs where runnables will also go through attributes.
Reviewed By: skcho
Differential Revision: D18660185
fbshipit-source-id: e26b1083e
Summary: When we see a call to schedule some work on an executor and we don't have evidence that it is on some specific thread (UI/BG), instead of dropping the work, assign it `UnknownThread` and treat it as running on the background by default.
Reviewed By: jvillard
Differential Revision: D18615649
fbshipit-source-id: e8bad64b6
Summary: Android may spontaneously call these methods on the UI thread, so recognize the fact.
Reviewed By: ezgicicek
Differential Revision: D18530477
fbshipit-source-id: a8a798779
Summary:
First step towards a global analysis. A new command line flag activates the step in `Driver`.
The whole-program analysis is a simple, quadratic (inefficient-as-yet), iteration over all domain elements. However, it is restricted to those elements that are explicitly scheduled to run.
Reviewed By: skcho
Differential Revision: D17787441
fbshipit-source-id: 9fecd766c
Summary:
In some apps executors are obtained by calling standard framework methods (and not by using DI with annotations).
To treat this style, we need to
- Detect calls that return such executors (`do_executor_effect`) and tag the return result with an `Attribute` indicating that it is now an executor, plus what thread it uses.
- Use that information when calling `execute`, to resolve the executor, if any, and its thread (in `do_work_scheduling` via `AttributeDomain.get_executor_constraint`).
- All this requires a new domain component, mapping variables to attributes. This extends the component previously used for remembering whether a variable is the result of a check on whether we run on the UI thread.
At the same time, I un-nested some functions from the transfer function for readability.
Reviewed By: skcho
Differential Revision: D18476122
fbshipit-source-id: bc39b5c2f
Summary:
`getThis` is an idiom for allowing Builder sub-classes to jump through the hoops of covariance plus java generics with self types. It's declared as abstract in the (generic) inner `Builder` class of a root class, and subclasses declare generic `Builder`s that inherit from the generic root `Builder` and trivially implement this method by returning `this`. Obviously, this returns conditional ownership (if `this` is owned, then the return value is owned).
The way it's typically used is
```
T foo() {
...
return getThis();
}
```
However, because abstract methods need dynamic dispatch for proper summarisation, we miss all that. A workaround was been implemented in D8947992 (see that for context), but it was buggy -- it required that the LHS type in the assignment
```
lhs = this.getThis();
```
is the same as the type of `this`, but this is too strict (eg, when using casts).
Here, the condition is changed to requiring that the return type of the method is the same as the type of `this`.
We also avoid asking for the `procdesc` as everything needed is in the attributes.
Reviewed By: jberdine
Differential Revision: D18450737
fbshipit-source-id: e67f0495c
Summary:
The upcoming whole-program analysis will need to log reports in different source files simultaneously.
Here, the data structure containing the reports, and in charge of deduplication is generalised to
a map from source file to what it was previously. This way, writing out the reports is possible even
with multiple source files.
Reviewed By: skcho
Differential Revision: D18394994
fbshipit-source-id: 5f5ecc27c
Summary: Capture locations where work is scheduled to run in parallel (here, just Executors). Also add a test file with cases the upcoming whole-program analysis for starvation should catch.
Reviewed By: dulmarod
Differential Revision: D18346880
fbshipit-source-id: 57411b052
Summary:
Add precision to analysis by elaborating the thread-status domain. This is done by having unknown (bottom), UI, BG or Any (both/top) elements in the lattice. This way, when we branch on thread-identity (if I am on UI thread do this, otherwise do that), we know that in one branch we are on UI thread and on the other we are *not* on the UI thread (BG thread), where previously the other branch would just go to top.
With this knowledge we can throw away pairs that come from callees which run on a thread that is impossible, given the current caller thread identity. This can happen when annotations are used incorrectly, and since this is the purview of annot-reachability, we just drop those pairs entirely.
Reviewed By: skcho
Differential Revision: D18202175
fbshipit-source-id: be604054e
Summary:
Steal a page from RacerD (and improve interface of) on using certain calls to assert
execution on a particular thread. Reduces FPs and FNs too.
Reviewed By: dulmarod
Differential Revision: D18199843
fbshipit-source-id: 5bdff0dfe
Summary: The way `<=` is used in `AbstractDomain` prevents infix use and forces bracketing it everywhere. Replace with simple `leq`.
Reviewed By: jvillard
Differential Revision: D18201854
fbshipit-source-id: 8175224e4
Summary: Abstract state tracks stuff that is not needed for summaries, wasting space/time.
Reviewed By: jvillard
Differential Revision: D18171499
fbshipit-source-id: 25483ced9
Summary:
The reporting phase would go through the critical pairs in the summary roughly three times, once for each major type of warning (starvation, lockless violation, deadlock). This is wasteful, and also led to some code duplication. Fix.
Also, use the more efficient annotation matcher in `ConcurrencyModels` and move some model matchers to `StarvationModels`.
Reviewed By: artempyanykh
Differential Revision: D18118149
fbshipit-source-id: ff4dc3d07
Summary: It is now possible to push the thread status into each critical pair. This leads to higher precision, because when code branches on whether it is on the UI thread, the final abstract state of the procedure will be `AnyThread`, but pairs created in the UI thread branch should know that their status is `UIThread`, not `AnyThread`.
Reviewed By: jvillard
Differential Revision: D18114273
fbshipit-source-id: cbb99b46f
Summary:
The wrong function was used when we tried to see if the class is
annotated with NullsafeStrict. This made it work only for non-static
methods.
Now we use the proper way.
Reviewed By: ngorogiannis
Differential Revision: D18113848
fbshipit-source-id: 02b7555be
Summary:
Domain for thread-type. The main goals are
- Track code paths that are explicitly on UI thread (via annotations, or assertions).
- Maintain UI-thread-ness through the call stack (if a callee is on UI thread then the
trace any call site must be on the UI thread too).
- If we are not on the UI thread we assume we are on a background thread.
- Traces with "UI-thread" status cannot interleave but all other combinations can.
- We do not track other annotations (eg WorkerThread or AnyThread) as they can be
erroneously applied -- other checkers should catch those errors (annotation reachability).
- Top is AnyThread, and is used as the initial state for analysis.
Interestingly, by choosing the right strategy for choosing initial state and applying callee summaries gets rid of some false negatives in the tests even though we have not introduced any path sensitivity yet.
Reviewed By: jvillard
Differential Revision: D17929390
fbshipit-source-id: d72871034
Summary:
Eventually thread status will be stored inside every critical pair so as to allow path sensitivity. That means that the status can no longer be a whole trace, as this will quickly become intractable, because each domain element would have to maintain its own trace as well as its own thread-status trace.
This is not great, as we lose information here, but I don't see any other way around it that is not super complicated/costly (sharing will be limited when moving from callee to caller).
Other diffs up the stack will clean up infrastructure no longer used meaningfully (ie models and domains).
Reviewed By: mityal
Differential Revision: D17908908
fbshipit-source-id: 3bf353e33
Summary:
By some unfortunate logic, OCaml often decides to use
`sexp_list`/`sexp_option` instead of just `list`/`option`. Sometimes
these get copy/pasted in interface files.
It would be good to tell OCaml not to do that in the first place but in
the meantime: this diff.
Reviewed By: ngorogiannis
Differential Revision: D17907938
fbshipit-source-id: 7546834a2
Summary:
Starvation analysis keeps a trace documenting why a method is seen as on the UI thread (many reasons possible, often confusing). This was a call-stack plus string, for keeping the explanation of why the last callee is on the UI thread. This is bad, because it takes too much memory/storage (each string is custom-made to the classes/method involved), and is effectively untyped.
Switch to a proper type for explaining this, so the cost is just a few pointers plus shared procnames/types, and then convert to string only when reporting. This will also allow to push the UI trace inside each element of the starvation domain, so as to allow path sensitivity etc, without blowing up summary size.
Reviewed By: ezgicicek, artempyanykh
Differential Revision: D17810007
fbshipit-source-id: cdd743975
Summary: StarvationModels depended on StarvationDomain which is the wrong way around, and forbids using *Models from *Domain.
Reviewed By: mityal
Differential Revision: D17809431
fbshipit-source-id: 5aa369e7c
Summary: The type hierarchy was traversed multiple times when searching for annotations: once for methods/overrides annotated and once for superclasses. This can be done in one pass.
Reviewed By: dulmarod
Differential Revision: D17787172
fbshipit-source-id: 248dd4c27
Summary:
At some point it was thought that we can assume that any annotation starting with "On" means the method is on the UI thread.
That's too imprecise and has led to false positives and negatives. Restrict to a well-known safe set.
Reviewed By: ezgicicek
Differential Revision: D17769376
fbshipit-source-id: 0f8fee059
Summary:
Currently, lock state is a map from locks to stacks of lock acquisitions.
Since we now have the separate acquisitions component, we no longer need to
remember the stack of acquisitions for a lock. Instead we only need a lock count,
thus reducing the memory footprint.
At the same time, change acquire and release so that they make one tree operation per
component (map/acquisition) as opposed to two (search/update) operations.
Reviewed By: ezgicicek
Differential Revision: D17736727
fbshipit-source-id: 7579eb61e
Nikos Gorogiannis
Sungkeun Cho
Josh Berdine
Mitya Lyubarskiy
Jules Villard