Constructor without visible definition breaks the static analyzer's reasoning about unrelated fields

[josephcsible]

Consider this C++ code:

struct SomeStruct {
  SomeStruct() noexcept;
};

//SomeStruct::SomeStruct() noexcept {}

class SomeClass {
  const bool b;
  const SomeStruct s;

public:
  SomeClass() : b(true) {}
  operator bool() const { return b; }
};

void f() {
  int *p = new int;
  if (SomeClass())
    delete p;
}

When I run clang --analyze -Xanalyzer -analyzer-output=text on it, I get this:

q72007867.cpp:20:1: warning: Potential leak of memory pointed to by 'p' [cplusplus.NewDeleteLeaks]
}
^
q72007867.cpp:17:12: note: Memory is allocated
  int *p = new int;
           ^~~~~~~
q72007867.cpp:18:7: note: Assuming the condition is false
  if (SomeClass())
      ^~~~~~~~~~~
q72007867.cpp:18:3: note: Taking false branch
  if (SomeClass())
  ^
q72007867.cpp:20:1: note: Potential leak of memory pointed to by 'p'
}
^
1 warning generated.

Uncommenting the definition of SomeStruct's constructor makes the warning go away, though. Swapping the order of const bool b; and const SomeStruct s; also makes it go away.

[llvmbot]

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

[steakhal]

It seems like the first exploded node/state from which the two cases differ is State 988, which is tagged by ExprEngine : Prepare for object construction.

We can see that if we cannot see the sub-object constructors, e.g. we don't see the ctor of SomeStruct, we do an invalidation. I guess, the s member of a SomeClass might be able to reinterpret-cast its this pointer to something else, do some pointer arithmetic, and modify the const b member in some way. However, I would rather say that we shouldn't invalidate the neighbor members in such cases. Or at least, we should respect const members to preserve their values.

Consequently, at the operator bool() call, we will do a state-split, and construct a path on which we don't delete p; causing this FP.

In contrast to this, when we have the body of the SomeStruct ctor, we won't invalidate, thus we can accurately track that operator boo() should result in true; releasing p.

@haoNoQ WDYT?

These dumps had to be suffixed with the txt extension to make GitHub happy. without.html.txt with.html.txt

[josephcsible]

I guess, the s member of a SomeClass might be able to reinterpret-cast its this pointer to something else, do some pointer arithmetic, and modify the const b member in some way.

Since b is const, modifying it like that would be UB. If we had to assume that code we can't see might invoke UB, then we can't reason about anything, since then we'd also have to assume it could reach up into the stack and change our p to be nullptr before we can release it. So I agree we should assume that doesn't happen.

Two other things worth mentioning:

• Another change that I discovered makes the warning disappear is changing const bool b; to const bool b = true; and then getting rid of : b(true) from the constructor.
• The const SomeStruct in this code is really a std::unique_ptr<std::string> in the real-world code where I discovered this behavior. Even if we can't assume that arbitrary code will be well-behaved here, maybe making the assumption just for STL code would be warranted.