[Analyzer] Add PreStmt and PostStmt callbacks for OffsetOfExpr

[MTC]

Summary

PreStmt and PostStmt callbacks for OffsetOfExpr are necessary to implement Cert ARR39-C: Do not add or subtract a scaled integer to a pointer. And should I define the offsetof macro in clang/test/Analysis/Inputs/system-header-simulator.h? Or, like clang/test/Analysis/malloc-sizeof.c, use #include <stddef.h>?

[a.sidorin]

Hello Henry,

The patch looks reasonable. I think it can be landed after comments are resolved.

lib/StaticAnalyzer/Core/ExprEngine.cpp

1497

Could you C++11-fy this loop?

test/Analysis/offsetofexpr-callback.c

3

Answering your question, we should put the declaration into system-header-simulator.h.

7

This decl seems unused. Should we remove it?

[NoQ]

The patch seems correct, but i suspect that your overall approach to the checker you're trying to make is not ideal. offsetof returns a concrete value (because, well, symbolic offsetofs are not yet supported in the analyzer), and even if you see the concrete value, you'd be unable to track it, because when you get another concrete value with the same integer inside, you won't be able to easily figure out if its the same value or a different value. My intuition suggests that this checker shouldn't be path-sensitive; our path-sensitive analysis does very little to help you with this particular checker, and you might end up with a much easier and more reliable checker if you turn it into a simple AST visitor or an AST matcher. Just a heads up.

[MTC]

• Use C++11 range-based for loop to traverse ExplodedNodeSet.
• Define the macro offsetof in system-header-simulator.h.

[MTC]

In D42300#982187, @NoQ wrote:

My intuition suggests that this checker shouldn't be path-sensitive; our path-sensitive analysis does very little to help you with this particular checker, and you might end up with a much easier and more reliable checker if you turn it into a simple AST visitor or an AST matcher. Just a heads up.

This is a very useful suggestion, many thanks, Noq! Path-sensitive is really a bit too heavy for this checker.

[MTC]

rebase

[MTC]

@NoQ Sorry to bother you again. It seems that this patch is useless to analyzer temporarily, if you think so, I will abandon it : ).

[NoQ]

All right, i guess we already do have a pair of callbacks for IntegerLiteral and it doesn't hurt, especially because here they'd eventually be actually useful. I think it should land, just to make it easier to add the actual support later.

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

 
 class AnalysisOrderChecker
     : public Checker<check::PreStmt<CastExpr>,
                      check::PostStmt<CastExpr>,
                      check::PreStmt<ArraySubscriptExpr>,
                      check::PostStmt<ArraySubscriptExpr>,
                      check::PreStmt<CXXNewExpr>,
                      check::PostStmt<CXXNewExpr>,
+                     check::PreStmt<OffsetOfExpr>,
+                     check::PostStmt<OffsetOfExpr>,
                      check::PreCall,
                      check::PostCall,
                      check::NewAllocator,
                      check::Bind,
                      check::RegionChanges> {
   bool isCallbackEnabled(AnalyzerOptions &Opts, StringRef CallbackName) const {
     return Opts.getBooleanOption("*", false, this) ||
         Opts.getBooleanOption(CallbackName, false, this);
if (isCallbackEnabled(C, "PreStmtCXXNewExpr"))
       llvm::errs() << "PreStmt<CXXNewExpr>\n";
   }
 
   void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const {
     if (isCallbackEnabled(C, "PostStmtCXXNewExpr"))
       llvm::errs() << "PostStmt<CXXNewExpr>\n";
   }
 
+  void checkPreStmt(const OffsetOfExpr *OOE, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PreStmtOffsetOfExpr"))
+      llvm::errs() << "PreStmt<OffsetOfExpr>\n";
+  }
+
+  void checkPostStmt(const OffsetOfExpr *OOE, CheckerContext &C) const {
+    if (isCallbackEnabled(C, "PostStmtOffsetOfExpr"))
+      llvm::errs() << "PostStmt<OffsetOfExpr>\n";
+  }
+
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
     if (isCallbackEnabled(C, "PreCall")) {
       llvm::errs() << "PreCall";
       if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Call.getDecl()))
         llvm::errs() << " (" << ND->getQualifiedNameAsString() << ')';
       llvm::errs() << '\n';
     }
   }

lib/StaticAnalyzer/Core/ExprEngine.cpp

case Stmt::CXXThrowExprClass:
       break;
 
     case Stmt::ReturnStmtClass:
       Bldr.takeNodes(Pred);
       VisitReturnStmt(cast<ReturnStmt>(S), Pred, Dst);
       Bldr.addNodes(Dst);
       break;
 
-    case Stmt::OffsetOfExprClass:
+    case Stmt::OffsetOfExprClass: {
       Bldr.takeNodes(Pred);
-      VisitOffsetOfExpr(cast<OffsetOfExpr>(S), Pred, Dst);
+      ExplodedNodeSet PreVisit;
+      getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
+
+      ExplodedNodeSet PostVisit;
+      for (ExplodedNodeSet::iterator i = PreVisit.begin(), e = PreVisit.end();
+           i != e; ++i) {
+        VisitOffsetOfExpr(cast<OffsetOfExpr>(S), *i, PostVisit);
+      }
+
+      getCheckerManager().runCheckersForPostStmt(Dst, PostVisit, S, *this);
       Bldr.addNodes(Dst);
       break;
-
+    }
     case Stmt::UnaryExprOrTypeTraitExprClass:
       Bldr.takeNodes(Pred);
       VisitUnaryExprOrTypeTraitExpr(cast<UnaryExprOrTypeTraitExpr>(S),
                                     Pred, Dst);
       Bldr.addNodes(Dst);
       break;
 
     case Stmt::StmtExprClass: {

test/Analysis/offsetofexpr-callback.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:PreStmtOffsetOfExpr=true,debug.AnalysisOrder:PostStmtOffsetOfExpr=true %s 2>&1 | FileCheck %s
+
+#include <stddef.h>
+
+struct S {
+    char c;
+    double d;
+};
+
+void test() {
+  offsetof(struct S, c);
+}
+
+// CHECK: PreStmt<OffsetOfExpr>
+// CHECK-NEXT: PostStmt<OffsetOfExpr>