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