[PATCH] D95307: [StaticAnalyzer] Add checking for degenerate base class in MemRegion

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95307