[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.

[Anna Zaks via Phabricator via cfe-commits]

zaks.anna added a comment.

Thanks!!!



================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:965
+
+    // Performing operator `&' on an lvalue expression is essentially a no-op.
+    // Then, if we are taking addresses of fields or elements, these are also
----------------
NoQ wrote:
> alexshap wrote:
> > "Address-of" operator can be overloaded, 
> > just wondering - doest this code work correctly in that case ?
> In this case we'd see a `CXXOperatorCallExpr` instead of `UnaryOperator` (all hail clang AST!).
Adding a test case for that would be good.


================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:968
+    // unlikely to matter.
+    // FIXME: Currently offsets of fields are computed incorrectly,
+    // being always equal to 0. See the FIXME in StoreManager's
----------------
Incorrect implies that there is a better "correct" model and invites a fix. Do we know what better model would be? If so, we could add that to the comment. If not, I'd prefer explaining why it works this way (similarly to how you did in the comment below). Maybe adding an example of what does not work. And you could add a FIXME to say that it's worth investigating if there is a better way of handling it. (The majority of this info should probably go to Store.cpp)

Also, maybe it's just the choice of words here. "Incorrect" sounds like something that needs to be corrected. Whereas you could use something like "is modeled imprecisely with respect to what happens at execution time", which could still mean that this is how we do want to model it going forward.

It seems that the problem with modeling this way is demonstrated with a test case in explain-svals.cpp. So the benefits of the current modeling seem to be worth it.

Can we add a note along the path saying that "p" in "p->f" is null? This would address the user confusion with imprecise modeling.


================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:977
+          Ex = Op->getSubExpr()->IgnoreParenCasts();
+          while (true) {
+            if (const auto *ME = dyn_cast<MemberExpr>(Ex)) {
----------------
Why do we need the "while (true)"? Can we just use "dyn_cast<MemberExpr>(Ex)" as the loop condition?

Take a look at the getDerefExpr(const Stmt *S) and see if that would be a better place to add this code. Maybe not..



================
Comment at: lib/StaticAnalyzer/Core/Store.cpp:405
   case loc::ConcreteIntKind:
     // While these seem funny, this can happen through casts.
     // FIXME: What we should return is the field offset.  For example,
----------------
Could you rephrase this existing comment while you are here? Using word "funny" seems content-free and a bit strange. 




================
Comment at: lib/StaticAnalyzer/Core/Store.cpp:409
     //  like this work properly:  &(((struct foo *) 0xa)->f)
+    //  However, that's not easy to fix without reducing our abilities
+    //  to catch null pointer dereference. Eg., ((struct foo *)0x0)->f = 7
----------------
Thanks for adding the explanation!

Can you think of other cases where the same would apply? (Ex: array index)


https://reviews.llvm.org/D31982