[PATCH] D114718: [analyzer] Implement a new checker for Strict Aliasing Rule.

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Nov 29 12:57:06 PST 2021

NoQ added a comment.

Awesome, I love it!

What would it take to enable this by default? Note that we can enable the checker without emitting any warnings; sinking exotic execution paths (on which strict aliasing is known to be violated) is valuable on its own.

Other than that, we'll probably need a bug visitor to add a note for where the original type comes from (it should probably be the place where the location becomes *live*), and then some real-world testing.

Comment at: clang/lib/StaticAnalyzer/Checkers/StrictAliasingChecker.cpp:48-49
+  //
+  // NOTE: User must provide canonical and unqualified QualType's for the
+  // correct result.
+  static bool canAccess(QualType From, QualType To, ASTContext &Ctx) {
`assert()` it? Or maybe canonicalize defensively so that not to duplicate code in the caller, given that there's really only one correct way to do that?

Comment at: clang/lib/StaticAnalyzer/Checkers/StrictAliasingChecker.cpp:80
+class StrictAliasingChecker : public Checker<check::Location> {
+  mutable std::unique_ptr<BugType> BT;
The modern solution is
BugType BT{this, "...", "..."};

Comment at: clang/lib/StaticAnalyzer/Checkers/StrictAliasingChecker.cpp:113
+  QualType getOriginalType(CheckerContext &C, SVal V, QualType T) const {
+    assert(V.getAs<Loc>() && "Location shall be a Loc.");
+    V = C.getState()->getSVal(V.castAs<Loc>(), T);
I suspect it might also be `UnknownVal` (?) It usually makes sense to protect against such scenarios with an early return.

Comment at: clang/lib/StaticAnalyzer/Checkers/StrictAliasingChecker.cpp:141-145
+    auto ER = dyn_cast<ElementRegion>(R);
+    while (ER) {
+      R = ER->getSuperRegion();
+      ER = dyn_cast<ElementRegion>(R);
+    }
You're basically running into https://bugs.llvm.org/show_bug.cgi?id=43364 here. The element region is not necessarily a consequence of a pointer cast; it may also be a legitimate array element access, and there's no way to figure out what it really was. So I suspect that you may fail the following test case:
void foo() {
  int x[10];
  int *p = &x[1];
  *p = 1; // int incompatible with int[10]

Separately from that, ultimatetly you'll most likely have to handle //regions with symbolic base// separately. These regions are special because they are built when the information about the original type is not really unavailable. For now you're dodging this problem by only supporting `VarRegion`s with element sub-regions. But in the general case you may encounter code like this
void foo(void *p) {
  int *x = p;
  float *y = x;
  *y = 1.0;
which may be valid if the original pointer `p` points to a `float`. But this can be delayed until later.

Comment at: clang/lib/StaticAnalyzer/Checkers/StrictAliasingChecker.cpp:157
+    ExplodedNode *Node = C.generateNonFatalErrorNode();
+    if (!BT)
I suggest a fatal error node. Undefined behavior already occured, there's nothing interesting in the follow-up.

Comment at: clang/lib/StaticAnalyzer/Checkers/StrictAliasingChecker.cpp:178-179
+  const LangOptions &LO = CM.getLangOpts();
+  // Ideally, the condition should be `LO.CPlusPlus11 || LO.CPlusPlus2b` but
+  // implemented checks can be partially applied for C++17 and lower versions.
+  return LO.CPlusPlus;
We should probably keep the checker disabled in these language modes until we're sure it works correctly (in a conservative sense, i.e. doesn't emit false positives).

Can we check whether `-fstrict-aliasing` is enabled here? That's probably appropriate.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list