[StaticAnalyzer] Add checking for degenerate base class in MemRegion

[RedDocMD]

Summary

In the function isValidBaseClass() of
clang/lib/StaticAnalyzer/Core/MemRegion.cpp,
added a case to return true when BaseClass and Super refer to
the same CXXRecordDecl. This case arises when a pointer-to-member
field is declared with a static cast from a pointer-to-member of
a sub-class.

[steakhal]

Looks fine to me.
Thank you for addressing this.

[NoQ]

This patch shoots the messenger but someone still needs to conduct a proper investigation. The assertion is losing a lot more of its bug-finding power than necessary to uncover the current cornercase; i really hope to preserve it instead.

[vsavchenko]

In D95307#2518592, @NoQ wrote:

This patch shoots the messenger but someone still needs to conduct a proper investigation. The assertion is losing a lot more of its bug-finding power than necessary to uncover the current cornercase; i really hope to preserve it instead.

You mean the failing assertion from the tests?

[RedDocMD]

In D95307#2519283, @vsavchenko wrote:

In D95307#2518592, @NoQ wrote:

This patch shoots the messenger but someone still needs to conduct a proper investigation. The assertion is losing a lot more of its bug-finding power than necessary to uncover the current cornercase; i really hope to preserve it instead.

You mean the failing assertion from the tests?

Few hours before there was a message saying that there was some buggy commit to the build-bot. Are you referring to the build failure above?

[vsavchenko]

In D95307#2519300, @RedDocMD wrote:

In D95307#2519283, @vsavchenko wrote:

In D95307#2518592, @NoQ wrote:

This patch shoots the messenger but someone still needs to conduct a proper investigation. The assertion is losing a lot more of its bug-finding power than necessary to uncover the current cornercase; i really hope to preserve it instead.

You mean the failing assertion from the tests?

Few hours before there was a message saying that there was some buggy commit to the build-bot. Are you referring to the build failure above?

Yep. TBH I didn't take a good look at the failure, but judging even by the name of the failing test, it looks directly related to the subject of this patch.

[RedDocMD]

Funnily enough, when I run ninja clang-check I don't get any errors.

[vsavchenko]

In D95307#2519309, @RedDocMD wrote:

Funnily enough, when I run ninja clang-check I don't get any errors.

I believe that ninja check-clang is the right command (clang-check is a tool) :-)

[NoQ]

In D95307#2519283, @vsavchenko wrote:

In D95307#2518592, @NoQ wrote:

This patch shoots the messenger but someone still needs to conduct a proper investigation. The assertion is losing a lot more of its bug-finding power than necessary to uncover the current cornercase; i really hope to preserve it instead.

You mean the failing assertion from the tests?

I mean, isValidBaseClass() is a function that's only ever used as part of an assert condition; it has no other purpose. The only thing this patch does is partially disable the assertion. The patch also disables a lot more of the assertion than necessary for it to not fail on the newly added test.

Disabling the assertion has relatively little value on its own because assertions are already disabled in release/production builds of clang. We need to understand whether it is the assertion that's incorrect, or it's the surrounding code that's incorrect. Given that we're performing a derived-to-base cast when such cast is not necessary, that's a good indication that the problem is in the surrounding code, i.e. the behavior implemented in the transfer function isn't a correct abstraction over the actual runtime behavior of the program under analysis. It might be hard to judge because typed pointer values are a pretty questionable abstraction to begin with but i think at least some investigation is justified.

[NoQ]

In D95307#2519597, @vsavchenko wrote:

In D95307#2519309, @RedDocMD wrote:

Funnily enough, when I run ninja clang-check I don't get any errors.

I believe that ninja check-clang is the right command (clang-check is a tool) :-)

Inb4 ninja check-clang-analysis :)

[RedDocMD]

Thanks @NoQ for the tip on the right test command!
My own belief is that this patch is just a hack that squelches the problem, instead of solving it. I am limited by my own inexperience with the codebase and am trying to learn more to help better.

That said, couple of observations that I made while exploring the bug:

1. Referring to loop in line 1119 of SimpleSValBuilder.cpp. This loop is necessary since we need to access the right sub-object. (The test at line 203 in pointer-to-member.cpp is a good demonstration why)
2. However, when we have static casts on the pointer-to-member, this loop is also triggered.

The trouble is that the above two cases behave inconsistently with each other. Consider the following example for case 1:

struct B {
  int f;
};
struct L1 : public B { };
struct R1 : public B { };
struct M : public L1, R1 { };
struct L2 : public M { };
struct R2 : public M { };
struct D2 : public L2, R2 { };

void diamond() {
  M m;

  static_cast<L1 *>(&m)->f = 7;
  static_cast<R1 *>(&m)->f = 16;

  int L1::* pl1 = &B::f;
  int M::* pm_via_l1 = pl1;

  int R1::* pr1 = &B::f;
  int M::* pm_via_r1 = pr1;

  (m.*(pm_via_l1) == 7); 
  (m.*(pm_via_r1) == 16);
}

This behaves correctly. If we run I->getType()->dump() on each iteration of the loop, we get something like this:

RecordType 0x55a70ad60d50 'struct L1'
`-CXXRecord 0x55a70ad60cb8 'L1'
RecordType 0x55a70ad60b20 'struct B'
`-CXXRecord 0x55a70ad60a90 'B'
2 levels of base specifiers
RecordType 0x55a70ad60f50 'struct R1'
`-CXXRecord 0x55a70ad60ec0 'R1'
RecordType 0x55a70ad60b20 'struct B'
`-CXXRecord 0x55a70ad60a90 'B'
2 levels of base specifiers

Running the same analysis for the regressing code:

struct Base {
  int field;
};

struct Derived : public Base {};

void static_cast_test() {
  int Derived::* derived_field = &Derived::field;
  Base base;
  base.field = 5;
  int Base::* base_field = static_cast<int Base::*>(derived_field);
  (base.*base_field == 5);

we get the following dump;

RecordType 0x55676ded06a0 'struct Base'
`-CXXRecord 0x55676ded0610 'Base'
RecordType 0x55676ded06a0 'struct Base'
`-CXXRecord 0x55676ded0610 'Base'
2 base specifier

Clearly, there is a bug elsewhere. The method PointerToMember::begin() returns an iterator to a list of CXXBaseSpecifiers, which according to the docs is for keeping track of static_casts. However, it seems that the base-specifier list is being used for something else. I am not sure in which function introduces this problem. Probably somewhere in the ASTVisitor for the StaticAnalyzer this problem is introduced. I will keep on digging and add more comments.

[RedDocMD]

On further digging as to how pointer-to-members are handled in StaticAnalyzer, I discovered that in BasicValueFactory::accumCXXBase() for the example given in my test produces the two CXXBaseSpecifier` for the pointer-to-member. At line 195, one base specifier is added while the other is added in the loop in 201. The loop accounts for the static_cast while line 195 adds a base specifier from the expression being cast.
This causes the duplication. I will try to update this patch to handle the duplication.

[RedDocMD]

Removing duplication of CXXBaseSpecifiers when they are created for PointerToMember
Removed hackish weakening of assert

[kromanenkov]

Looks like you run formatter on the whole file, maybe narrow down its scope a little? For example, only for the touched function?

[kromanenkov]

clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp

199 ↗

(On Diff #319607)

What if we get the situation of the repeated types in the PathRange?

Like Derived --(cast to)-> Base --(cast to)-> Derived --(cast to)-> Base and so on.

Do we need to store the pair of QualType and PathRange index for that case or only type will be enough?

[RedDocMD]

@kromanenkov I guess it won't work then. Thanks for the idea!
Now that I think about it removing duplicates is probably not the right thing to do. The problem arises because the call PTMD->getCXXBaseList() should ideally return the list of previous casts, but instead returns the base class(es) of the struct whose pointer-to-member we are defining.

[RedDocMD]

I clearly am taking the wrong approach to this problem. I will need to reconsider how PointerToMember is actually modelled before solving this problem

[vsavchenko]

In D95307#2532669, @RedDocMD wrote:

I clearly am taking the wrong approach to this problem. I will need to reconsider how PointerToMember is actually modelled before solving this problem

I don't know if this is useful or not, but pointer-to-members used to be void * up until very recently. Here is my change on that topic (and it probably where the bug you're trying to fix initiated): https://reviews.llvm.org/D85817
I thought, it can be useful to see the areas of code where it all happens.

[RedDocMD]

In D95307#2533259, @vsavchenko wrote:

I don't know if this is useful or not, but pointer-to-members used to be void * up until very recently. Here is my change on that topic (and it probably where the bug you're trying to fix initiated): https://reviews.llvm.org/D85817
I thought, it can be useful to see the areas of code where it all happens.

Thank you @vsavchenko. I had actually seen this review by examining the file blames. I don't think the problem is in your review. The trouble is that when the CXXBaseSpecifier list in PointerToMemberData is initialized, it receives a list. That list I suspect contains passed in the base class of the struct. But this list is supposed to be for following static casts. I have to localize that logic.

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

static bool isValidBaseClass(const CXXRecordDecl *BaseClass,
                              const TypedValueRegion *Super,
                              bool IsVirtual) {
   BaseClass = BaseClass->getCanonicalDecl();
 
   const CXXRecordDecl *Class = Super->getValueType()->getAsCXXRecordDecl();
   if (!Class)
     return true;
 
+  if (Class == BaseClass)
+    return true;
+
   if (IsVirtual)
     return Class->isVirtuallyDerivedFrom(BaseClass);
 
   for (const auto &I : Class->bases()) {
     if (I.getType()->getAsCXXRecordDecl()->getCanonicalDecl() == BaseClass)
       return true;
   }
 

clang/test/Analysis/pointer-to-member.cpp

void double_diamond() {
 
   clang_analyzer_eval(d2.*(static_cast<int D2::*>(static_cast<int L2::*>(static_cast<int L1::*>(&B::f)))) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(d2.*(static_cast<int D2::*>(static_cast<int R2::*>(static_cast<int L1::*>(&B::f)))) == 2); // expected-warning {{TRUE}}
   clang_analyzer_eval(d2.*(static_cast<int D2::*>(static_cast<int L2::*>(static_cast<int R1::*>(&B::f)))) == 3); // expected-warning {{TRUE}}
   clang_analyzer_eval(d2.*(static_cast<int D2::*>(static_cast<int R2::*>(static_cast<int R1::*>(&B::f)))) == 4); // expected-warning {{TRUE}}
 }
 } // end of testPointerToMemberDiamond namespace
 
+namespace testPointerToMemberViaStaticCast {
+// From bug #48739
+struct Base {
+  int field;
+};
+
+struct Derived : public Base {};
+
+void static_cast_test() {
+  int Derived::* derived_field = &Derived::field;
+  Base base;
+  base.field = 5;
+  int Base::* base_field = static_cast<int Base::*>(derived_field);
+  clang_analyzer_eval(base.*base_field == 5); // expected-warning {{TRUE}}
+}
+}
+
 namespace testAnonymousMember {
 struct A {
   int a;
   struct {
     int b;
     int c;
   };
   struct {