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

[Artem Dergachev via Phabricator via cfe-commits]

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
```lang=c++
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:
```lang=c++
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
```lang=c++
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114718/new/

https://reviews.llvm.org/D114718