[analyzer] C++17: PR40022: Support aggregate initialization with compound values in presence of base classes.

[NoQ]

Summary

I'll add more tests soon, to see if it is actually initialized *correctly* now, but for now here's the code that fixes the crash in https://bugs.llvm.org/show_bug.cgi?id=40022 .

It turns out that C++17 aggregate initialization with base classes wasn't really ever implemented for the case when it *doesn't* involve calling any sub-object constructors. That it, when the aggregate is initialized with a simple initializer list and the resulting value is a nonloc::CompoundVal, we simply didn't know that we need to perform the store bind for the base classes as well. Now it should work.

The original bug report causes us to crash when there's a flexible array at the end of the structure (which is a non-standard thing), which made me underestimate the problem a bit :)

The case where we *do* have constructors in base classes is, of course, still unimplemented; it was last touched in D40841.

[alexfh]

LG in general. A couple of comments.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

2348 ↗

(On Diff #189594)

Should this be const auto&?

2349 ↗

(On Diff #189594)

!B.isVirtual()

Consider also adding && "more details about this assertion".

[Szelethus]

At first glance this looks OK, but I'll take the time to understand the infrastructure behind it and give a proper review.

clang/test/Analysis/array-struct-region.cpp

3 ↗

(On Diff #189594)

I didn't see -analyzer-config c++-inlining=constructors in the bugreport -- why is it needed?

[NoQ]

Thx! Also added the more direct test.

I'll take the time to understand the infrastructure behind it

Well, roughly, the CompoundVal kind of value is used very rarely, unlike LazyCompoundVal. The raw CompoundVal is essentially a symbolic InitListExpr: an (immutable) list of other values. It appears pretty much only when there's an actual initializer list expression in the program, and the analyzer tries to unwrap it as soon as possible.

This code is where such unwrap happens: when the compound value is put into the object that it was supposed to initialize (it's an *initializer* list, after all), instead of binding the whole value to the whole object, we bind sub-values to sub-objects. Sub-values may themselves be compound values, and in this case the procedure becomes recursive.

The annoying part about compound values is that they don't carry any sort of information about which value corresponds to which sub-object. It's simply a list of values in the middle of nowhere; the Store expects to match them to sub-objects, essentially, by *index*: first value binds to the first field, second value binds to the second field, etc. The crash was occuring when it was accidentally trying to bind a base-class value to a field object - because it simply didn't know that base objects are a thing.

[NoQ]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

2348 ↗

(On Diff #189594)

Whoops. Also i still somehow hate it that these aren't pointers :)

clang/test/Analysis/array-struct-region.cpp

3 ↗

(On Diff #189594)

Hmm, right, these flags are outdated long time ago. Wiped them out from this test.

[Szelethus]

Ooooh you explained that very well, I'm clear on what's happening then :D Still, I'll poke the code here and there to learn about this part of the analyzer, I might even come up with some meaningful feedback (or a meaningful patch acceptance).

[NoQ]

Added this as a comment.

If the patch is not obvious at a glance, feel free to demand comments! Every second you spend trying to figure out how the code works is multiplied by the number of people who will have to do the same in the future.

[alexfh]

LG

[Szelethus]

LGTM!

cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp

if (V.getAs<nonloc::SymbolVal>())
     return bindAggregate(B, R, V);
 
   // We may get non-CompoundVal accidentally due to imprecise cast logic or
   // that we are binding symbolic struct value. Kill the field values, and if
   // the value is symbolic go and bind it as a "default" binding.
   if (V.isUnknown() || !V.getAs<nonloc::CompoundVal>())
     return bindAggregate(B, R, UnknownVal());
 
+  // The raw CompoundVal is essentially a symbolic InitListExpr: an (immutable)
+  // list of other values. It appears pretty much only when there's an actual
+  // initializer list expression in the program, and the analyzer tries to
+  // unwrap it as soon as possible.
+  // This code is where such unwrap happens: when the compound value is put into
+  // the object that it was supposed to initialize (it's an *initializer* list,
+  // after all), instead of binding the whole value to the whole object, we bind
+  // sub-values to sub-objects. Sub-values may themselves be compound values,
+  // and in this case the procedure becomes recursive.
+  // FIXME: The annoying part about compound values is that they don't carry
+  // any sort of information about which value corresponds to which sub-object.
+  // It's simply a list of values in the middle of nowhere; we expect to match
+  // them to sub-objects, essentially, "by index": first value binds to
+  // the first field, second value binds to the second field, etc.
+  // It would have been much safer to organize non-lazy compound values as
+  // a mapping from fields/bases to values.
   const nonloc::CompoundVal& CV = V.castAs<nonloc::CompoundVal>();
   nonloc::CompoundVal::iterator VI = CV.begin(), VE = CV.end();
 
-  RecordDecl::field_iterator FI, FE;
   RegionBindingsRef NewB(B);
 
+  // In C++17 aggregates may have base classes, handle those as well.
+  // They appear before fields in the initializer list / compound value.
+  if (const auto *CRD = dyn_cast<CXXRecordDecl>(RD)) {
+    assert(CRD->isAggregate() &&
+           "Non-aggregates are constructed with a constructor!");
+
+    for (const auto &B : CRD->bases()) {
+      // (Multiple inheritance is fine though.)
+      assert(!B.isVirtual() && "Aggregates cannot have virtual base classes!");
+
+      if (VI == VE)
+        break;
+
+      QualType BTy = B.getType();
+      assert(BTy->isStructureOrClassType() && "Base classes must be classes!");
+
+      const CXXRecordDecl *BRD = BTy->getAsCXXRecordDecl();
+      assert(BRD && "Base classes must be C++ classes!");
+
+      const CXXBaseObjectRegion *BR =
+          MRMgr.getCXXBaseObjectRegion(BRD, R, /*IsVirtual=*/false);
+
+      NewB = bindStruct(NewB, BR, *VI);
+
+      ++VI;
+    }
+  }
+
+  RecordDecl::field_iterator FI, FE;
+
   for (FI = RD->field_begin(), FE = RD->field_end(); FI != FE; ++FI) {
 
     if (VI == VE)
       break;
 
     // Skip any unnamed bitfields to stay in sync with the initializers.
     if (FI->isUnnamedBitfield())
       continue;

cfe/trunk/test/Analysis/array-struct-region.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -x c %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -x c++ -analyzer-config c++-inlining=constructors %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,debug.ExprInspection -DINLINE -verify -x c %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,debug.ExprInspection -DINLINE -verify -x c++ -analyzer-config c++-inlining=constructors %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
+// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -x c %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
+// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -x c++ -std=c++14 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
+// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -x c++ -std=c++17 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
+// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -DINLINE -x c %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
+// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -DINLINE -x c++ -std=c++14 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
+// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
+// RUN:                    -DINLINE -x c++ -std=c++17 %s
 
 void clang_analyzer_eval(int);
 
 struct S {
   int field;
 
 #if __cplusplus
   const struct S *getThis() const { return this; }
namespace EmptyClass {
   // There used to be a warning here, because analyzer treated Derived as empty.
   int test() {
     int a;
     ref(a) = 42;
     return a; // no warning
   }
 }
 
+#if __cplusplus >= 201703L
+namespace aggregate_inheritance_cxx17 {
+struct A {
+  int x;
+};
+
+struct B {
+  int y;
+};
+
+struct C: B {
+  int z;
+};
+
+struct D: A, C {
+  int w;
+};
+
+void foo() {
+  D d{1, 2, 3, 4};
+  clang_analyzer_eval(d.x == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(d.y == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(d.z == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(d.w == 4); // expected-warning{{TRUE}}
+}
+} // namespace aggregate_inheritance_cxx17
+#endif
+
+namespace flex_array_inheritance_cxx17 {
+struct A {
+  int flexible_array[];
+};
+
+struct B {
+  long cookie;
+};
+
+struct C : B {
+  A a;
+};
+
+void foo() {
+  C c{}; // no-crash
+}
+} // namespace flex_array_inheritance_cxx17
 #endif