[PATCH] D37023: [analyzer] Fix bugreporter::getDerefExpr() again.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 116542.
NoQ added a comment.

@dcoughlin: You're right, my reasoning and understanding was not correct, and your explanation is much more clear. My code still makes sense to me though, so i updated the comments to match. And moved the unusual logic for the lvalue-to-rvalue cast unwrap to the bottom of the function.

Essentially, `getDerefExpr()` tries its best to explain where the null pointer comes from, on the syntactic level, by unpacking expressions that wrap the expression from which the pointer "originates", where "originates" is understood in a vague manner defined only by what the callers of this utility function expect to see. This leaves us with a few kinds of expressions that we need to unwrap, and we shouldn't touch other expressions, leaving pattern-matching over them to the caller.

There are other facilities that work on non-syntactic level, like your example where we might jump from function expression to return statement within the callee if the callee was inlined, or how `getNilReceiver()` function unwraps `ObjCMessageExpr` to the `self` pointer *iff* it `self` was `nil` during symbolic execution (which implies that the whole message expression is `nil`).

So that's right - lvalue-to-rvalue casts are not "the whole point", but merely an example of an expression kind that we do not have a right to unwrap. In fact, the callers still expect us to unwrap it once, but immediately stop afterwards. This inconsistency should probably be fixed, but `getDerefExpr` is used by checkers, and current code in checkers seems clean and concise enough.

I had a look at other cast kinds in `OperationKinds.def`, and all of them seemed as if they're worth unwrapping, apart from, of course, `CK_LValueToRValue`.

@xazax.hun: So, i guess, if the cast is earlier than we'd expect, then we'd just fail to unwrap something. So we won't find where the pointer comes from, and give up prematurely on our pattern-matching, while looking at roughly the same expression/value, plus-minus offsets. It sounds better than the case when the cast is //later// than we'd expect, where we may accidentally unwrap wrong stuff and start tracking a completely unrelated value, so i hope it shouldn't be a problem. Thanks for sharing the concern - the AST is huge and very few people possess really deep understanding of it, so any curious findings about it are really nice to share.


https://reviews.llvm.org/D37023

Files:
  lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  test/Analysis/null-deref-path-notes.m

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D37023.116542.patch
Type: text/x-patch
Size: 15641 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170925/f5f64f7d/attachment-0001.bin>