CSA Exploded Graph will end on multiple non-shouldInlineCall destructors

[BoYanZh]

Requirements to trigger the bug:

1. Define a class with >=2 member objects of the same type T.
2. Type T must be a class that has a non-inlineable (a.k.a ExprEngine::shouldInlineCall return false) dtor.

struct Test {
  Test() {}
  ~Test();
};

int foo() {
  struct a {
    Test b, c;
  } d;
  return 1;
}

int main() {
  if (foo()) return 1;
}

llvm-project/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

Line 1076 in 5bc4e1c

return false;

will return false as there is no definition of Test::~Test. And I also tried to add a default dtor on Test (which will make shouldInlineCall return true) but force shouldInlineCall to return false when analyzing b::~Test() and c::~Test(), and the exploded graph will also end immediately.

Since the exploded graph ends here, it will report a false positive on unreachable code in the end.

warning: This statement is never executed [alpha.deadcode.UnreachableCode]
  if (foo()) return 1;
                    ^
1 warning generated.

I found this patch aac73a3 may be related since it remove the following comments. And I tried to fix the bug by imitating this patch but failed.

  // FIXME: We need to run the same destructor on every element of the array.
  // This workaround will just run the first destructor (which will still
  // invalidate the entire array).

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

[haoNoQ]

Yeah looks like a program point bug, the next step receives the same program point, so when the state is also accidentally the same, the exploded graph turns into a loop.

Also in this case the state is the same because conjured symbol identity is broken as it requires a statement but destructors don't correspond to any statement, which is an old bug in and of itself.

@isuckatcs you might enjoy this one!

Also I don't recommend using the dead code checker. It's alpha for a reason. Even without bugs like this, it cannot work correctly until we explicitly annotate all places where we intentionally drop coverage.

[isuckatcs]

The egraph on my side look like this using clang 16:

The loop inside the egraph is a finite loop, otherwhise we would have a crash. We are inside the destructor of struct a and we call the destructor of struct Test twice after each other immediately, hence the loop. I checked inside lldb and the analyzer does exactly this.

These are the lines responsible for generating the program points:

  static SimpleProgramPointTag PT("ExprEngine",
                                  "Prepare for object destruction");
  PreImplicitCall PP(DtorDecl, Member->getLocation(), LCtx, &PT);
  Pred = Bldr.generateNode(PP, State, Pred);

The only difference here in evaluating the destructor of a.c and a.b is the source location, but it seems that for some reason we don't differentiate between them when we render the egraph. The exploded nodes that are generated here are not the same.

[BoYanZh]

Thanks for the reply! I have tried to let the adjacent destructor have different types but it also fails, but I have no idea why this will happen.

struct Test {
  Test() {}
  ~Test();
};

struct Test1 {
  Test1() {}
  ~Test1();
};


int foo() {
  struct a {
    Test b;
    Test1 c;
    Test d;
    Test1 e;
  } z;
  return 1;
}

int main() {
  if (foo()) return 1;
}

[isuckatcs]

In this new example the loop still looks correct, because you loop trough the same sequence of nodes.
Note that the node you attached contains 2 destructor evaluations.

The destructors are called in the following order:

~Test1() -> ~Test() -> ~Test1() -> ~Test()
                       ^ here the pattern repeats itself, hence the loop

If I modify struct a like this, there are no loops in the egraph on my side:

struct a {
  Test b;
  Test1 c;
};

[haoNoQ]

The exploded graph shouldn't contain a loop unless the program under analysis contains an actual loop. The loop in the program could be implemented with gotos or recursion, but simply calling the same function twice in a row shouldn't cause loops in the graph, because there's simply no way for it to go through the same context-sensitive program point more than once.

[isuckatcs]

D143328