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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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.



================
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;
----------------
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.


================
Comment at: clang/test/Analysis/PR46264.cpp:5
+
+namespace a {
+class b {
----------------
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.


================
Comment at: clang/test/Analysis/PR46264.cpp:9
+  typedef int b::*c;
+  operator c() { return &b::d; }
+  int d;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85817