[PATCH] D85817: [analyzer] Fix crash with pointer to members values

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D85817#2213435 <https://reviews.llvm.org/D85817#2213435>, @NoQ wrote:

> That's a fix for https://bugs.llvm.org/show_bug.cgi?id=46264.
>
> Your code looks great but i don't understand at a glance what the crash was caused by and how does your code fix it, can you explain? Like, the original test doesn't have any `void *`s and it doesn't have any indirect fields. Also the crash happens during visitor phase but the fixes are entirely during modeling phase.

Sure! The problem was in the **deleted code**, we modeled `FieldDecl` and `IndirectFieldDecl` in such matter that `&b::d` ended up as `void *` and it was totally fine for path exploration, but the moment we try to //explain// such value is where we hit the assertion (I tried to explain that in the summary). I could've fixed the symptom and patch that, but it didn't seem right.  It is not a pointer to void and it never was.  Then I found out about the fact that we actually have a mechanism to deal with pointer to members.  So I decided to remove the code with 2 FIXMEs (by doing one of the FIXMEs).  Removing only `FieldDecl` from the condition would've fixed the original problem, but wouldn't have solved a very similar example with indirect fields (maybe I should expand the test to include that one as well).  That's how I came to fixing the behavior for indirect fields as well.



================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:2533-2534
   if (isa<FieldDecl>(D) || isa<IndirectFieldDecl>(D)) {
-    // FIXME: Compute lvalue of field pointers-to-member.
-    // Right now we just use a non-null void pointer, so that it gives proper
-    // results in boolean contexts.
-    // FIXME: Maybe delegate this to the surrounding operator&.
-    // Note how this expression is lvalue, however pointer-to-member is NonLoc.
-    SVal V = svalBuilder.conjureSymbolVal(Ex, LCtx, getContext().VoidPtrTy,
-                                          currBldrCtx->blockCount());
-    state = state->assume(V.castAs<DefinedOrUnknownSVal>(), true);
-    Bldr.generateNode(Ex, Pred, state->BindExpr(Ex, LCtx, V), nullptr,
-                      ProgramPoint::PostLValueKind);
+    // Delegate all work related to pointer to members to the surrounding
+    // operator&.
     return;
----------------
NoQ wrote:
> Ok so you're saying that there's *always* going to be a surrounding operator `&`? That kind of makes sense but if you add more explanation/proof of how you figured this out that'd be great.
I don't know actually that there's *always* a surrounding `&`, I mean it makes sense and I guess it is the only case we care about.


================
Comment at: clang/test/Analysis/PR46264.cpp:5
+
+namespace a {
+class b {
----------------
NoQ wrote:
> I'm pretty sure the namespace is not actually important for the test. `creduce` is great but sometimes it misses stuff. Generally, i'm a big fan of manually cleaning up `creduce`d test to make them look more like real code. At least turn things like `b h;` and `e *i;` into something like `A a;` and `B *b;`. Also replace `class` with `struct` because we never care about this entire public/private business and `struct` provides a nice default.
I didn't change the code because it is the actual snippet from the bug report, but OK


================
Comment at: clang/test/Analysis/PR46264.cpp:9
+  typedef int b::*c;
+  operator c() { return &b::d; }
+  int d;
----------------
NoQ wrote:
> It's worth it to comment where exactly is this conversion operator used in the text (i.e., within the if-condition). People do in fact sometimes do this kind of thing in real life: provide a conversion to pointer-to-member *instead of* conversion to `bool` because it causes fewer potential further accidental implicit conversions to be possible.
OK, will do


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85817