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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
