[PATCH] D26342: [analyzer] Add MutexChecker for the Magenta kernel

[Artem Dergachev via cfe-commits]

NoQ added a comment.

It's great to see more domain-specific checks coming in! We're glad to be useful. The code is also well-commented, and a lot of tests are provided, which is great.

This checker seems to be relatively similar to the existing `alpha.unix.PthreadLock` checker (which also handles XNU kernel locks already). In fact, the part that checks init-lock-unlock-destroy paths might actually fit in there by simply adding more function identifiers; it also shares the same problems, like not supporting pointer escapes (if a pointer to a mutex escapes to an external function, this function may lock or unlock the mutex without us knowing), or never cleaning up the program state maps (even when regions are dead and could no longer be referenced - useful for performance, and also may find more defects, eg. mutex region becoming dead in a locked state is probably a problem). So i think there's an opportunity for some code re-use, if you're interested (it wouldn't immediately benefit any of the checkers, but any later changes would naturally land into both checkers). And the escape and cleanup problems should be easy to address.

I see you also have a feature that's not covered by the existing checker, which is checking the constructor-destructor lock-unlock match. I suspect that this sub-check needs a bit more testing. My first concern is that `checkEndFunction` is a callback that fires once per path through end of function, not once per function. Here's a test i could come up with, which currently fails - i'm deliberately tricking the analyzer into analyzing branches in different order by alternating between if-branch of a condition and else-branch of an inverted condition; i think there may be more false positives of this kind if there's more of the complicated control flow in constructors/destructors:

  bool glob;
  
  class Foo {
    mutex_t lock;
  
  public:
    Foo() {
      if (glob) {
      } else {
        mutex_init(&lock);
      }
    }
    ~Foo() {
      if (!glob) {
        mutex_destroy(&lock);
      } else {
      }
    }
  };
  
  void foo() {
    Foo F; // Warns "Mutex was not destroyed in the destructor", perhaps shouldn't.
  }

I also have another kind of concern for this sub-check: it relies on the core analyzing all paths through the constructor/destructor. However, the analyzer never promises to cover all paths during analysis, and in fact reserves the right to drop paths arbitrarily - this is a kind of fundamental limitation for the symbolic execution technique, otherwise the analysis would never terminate on most code. So additional hacks might be needed to avoid false positives that may arise if, for example, the path on which the mutex_destroy call in the destructor was completely skipped during analysis. It should be possible to hang up to `checkEndAnalysis` to see if there is extra stuff in the worklist at the end, which would indicate that some paths were dropped.

It also might be a good idea to make this check path-insensitive instead, which should be more reliable and transparent for this case - simply scan the AST of constructor and destructor bodies for references to mutex variables within init/destroy call expressions, compose the sets of variable declarations referenced that way, and compare those.

To summarize, depending on how much time you'd like to invest into this checker, and if any of the thoughts above sounded as a useful feedback, in no particular order and with no urgency and without really blocking this patch from landing you may want to:

1. Try to merge with PthreadLock checker,
2. Handle mutex pointer escapes,
3. Handle dead symbols,
4. Move constructor-destructor init-destroy sets to the program state to make them path-aware,
5. Or otherwise move the checks to `checkEndOfTranslationUnit` to make sure all paths were covered;
6. Check if any paths were skipped during analysis (not sure if it'd be necessary after 4.),
7. Maybe make the constructor-destructor init-destroy check AST-based (instead of 4.-6.).

Generally, i think this checker is good in its current shape, and issues above can anyway be addressed later, in an incremental manner. Unless you prefer to make major changes to this patch at this point, i should be able to follow up with minor comments.



================
Comment at: lib/StaticAnalyzer/Checkers/MutexChecker.cpp:228
+  St = St->set<MutexMap>(MutexMemRegion, MutexInfo::getInitialized());
+  C.addTransition(St);
+}
----------------
Before i forget: this API is a bit tricky, and there's a common but completely unobvious mistake that you're making here. `reportMutexError(..., false)` is producing a transition to a non-fatal error node. The whole point of the non-fatal error node is that analysis continues past this node. After adding the transition on this line, you effectively split the analysis into two paths, one following the non-fatal error node, another containing the `MutexMap` change.

This doubles up the time it takes to analyze the rest of the code (growing exponentially if encountered multiple times; probably rises to the node limit very soon, destroying coverage), and the additional path through the error node is not expected here (but it might be for some checkers, so the API is not quite at fault) - the behavior of the checker on this path might also be unexpected (it essentially skips the initialization of the mutex).

The right way to do this is to remember the exploded node generated by `generateNonFatalErrorNode()`, and put the new transition on top of that node (if the report has actually been thrown) by using the `addTransition(St, N)` override (or make the transition normally if it isn't).

I really wish there was a good and universal solution to make the transition API more obvious, feel sorry about that every time i see one of those problems.


https://reviews.llvm.org/D26342