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