[PATCH] D27854: [analyzer] Add check for mutex acquisition during interrupt context in Magenta kernel

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This checker looks good to me! I don't see any obvious problems, and i think we can land it into non-alpha (enabled by default) once reviewers' comments are addressed.



================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:78
 
+def Magenta : Package<"magenta">, InPackage<OptIn>;
+
----------------
`optin` is for checkers for which we cannot make a good guess if the user wants those or not.

Can we make a guess by looking at the target triple in this case? If we can, then it should not go into `optin`, but rather auto-enabled on specific platform, similarly to how it works for eg. `osx`.


================
Comment at: lib/StaticAnalyzer/Checkers/MagentaInterruptContextChecker.cpp:32
+  void reportMutexAcquisition(SymbolRef FileDescSym,
+                                const CallEvent &call,
+                                CheckerContext &C) const;
----------------
Indent slightly off.


================
Comment at: lib/StaticAnalyzer/Checkers/MagentaInterruptContextChecker.cpp:54
+  /// a call to that function.
+  void checkBeginFunction(CheckerContext &C) const;
+
----------------
To see if we're in an interrupt, take current `LocationContext` and see if its stack frame or a parent stack frame is for `x86_exception_handler`. I think you don't need a program state trait for that check - it's already in your location context - and `checkBeginFunction()`/`checkEndFunction()` can be removed. Also, as far as i remember (i may be wrong), `AnalysisDeclContext` will bring you the current top-level function declaration under analysis directly, without ascending the stack frames, if that's in fact all you need.

For exit-functions you'd still need a flag in the program state that says that there was an exit.


================
Comment at: lib/StaticAnalyzer/Checkers/MagentaInterruptContextChecker.cpp:93
+  ProgramStateRef State = C.getState();
+  unsigned inInterrupt = State->get<InInterrupt>();
+  if (!(FD->getName().compare(ExceptionHandler))) {
----------------
Probably a bool was intended. Also, this may be moved inside the assert body if not used anywhere else.


================
Comment at: lib/StaticAnalyzer/Checkers/MagentaInterruptContextChecker.cpp:109
+  ProgramStateRef State = C.getState();
+  if (!(FD->getName().compare(ExceptionHandler)))
+    State = State->set<InInterrupt>(false);
----------------
I'd suggest to use `operator==` for such cases because it's easier to read.


https://reviews.llvm.org/D27854