AddChildren: Filter out false children

Use fully qualified names to determine if a class is really the child of
our type. It may be that it is the child of another type with an
identical name in another namespace.

oi/type_graph/AddChildren.cpp

@@ -75,10 +75,28 @@ void AddChildren::visit(Class& c) {
         dynamic_cast<Class*>(&childType);  // TODO don't use dynamic_cast
     if (!childClass)                       // TODO dodgy error handling
       abort();
-    c.children.push_back(*childClass);
 
-    //    // Add recursive children to this class as well
+    /*
-    //    enumerateClassChildren(drgnChild, children);
+     * Confirm that this child is actually our child.
+     *
+     * We previously used unqualified names for speed, so types with the same
+     * name in different namespaces would have been grouped together.
+     */
+    bool isActuallyChild = false;
+    for (const auto& parent : childClass->parents) {
+      // TODO support parent containers?
+      auto* parentClass = dynamic_cast<Class*>(&stripTypedefs(parent.type()));
+      if (!parentClass)
+        continue;
+
+      if (parentClass->fqName() == c.fqName()) {
+        isActuallyChild = true;
+        break;
+      }
+    }
+
+    if (isActuallyChild)
+      c.children.push_back(*childClass);
   }
 
   // Recurse to find children-of-children
@@ -87,35 +105,6 @@ void AddChildren::visit(Class& c) {
   }
 }
 
-// TODO how to flatten children of children?
-// void AddChildren::enumerateClassChildren(struct drgn_type *type,
-// std::vector<std::reference_wrapper<Class>> &children) {
-//  // This function is called recursively to find children-of-children, so the
-//  // "children" vector argument will not necessarily be empty.
-//
-//  const char* tag = drgn_type_tag(type);
-//  if (tag == nullptr) {
-//    return;
-//  }
-//  auto it = childClasses_.find(tag);
-//  if (it == childClasses_.end()) {
-//    return;
-//  }
-//
-//  const auto& drgnChildren = it->second;
-//  for (drgn_type* drgnChild : drgnChildren) {
-//    // TODO there shouldn't be any need for a dynamic cast here...
-//    Type *ttt = enumerateClass(drgnChild);
-//    auto *child = dynamic_cast<Class*>(ttt);
-//    if (!child)
-//      abort();
-//    children.push_back(*child);
-//
-//    // Add recursive children to this class as well
-//    enumerateClassChildren(drgnChild, children);
-//  }
-//}
-
 void AddChildren::recordChildren(drgn_type* type) {
   drgn_type_template_parameter* parents = drgn_type_parents(type);
 
@@ -141,9 +130,7 @@ void AddChildren::recordChildren(drgn_type* type) {
     }
 
     const char* parentName = drgn_type_tag(parent);
-    if (parentName == nullptr) {
+    if (!parentName) {
-      //      VLOG(1) << "No name for parent class (" << parent << ") of "
-      //              << drgn_type_tag(type);
       continue;
     }
 
@@ -151,10 +138,12 @@ void AddChildren::recordChildren(drgn_type* type) {
      * drgn pointers are not stable, so use string representation for reverse
      * mapping for now. We need to find a better way of creating this
      * childClasses map - ideally drgn would do this for us.
+     *
+     * Use unqualified names because fully-qualified names are too slow. We'll
+     * check against fully-qualified names once we've narrowed down the number
+     * of types to compare.
      */
     childClasses_[parentName].push_back(type);
-    //    VLOG(1) << drgn_type_tag(type) << "(" << type << ") is a child of "
-    //            << drgn_type_tag(parent) << "(" << parent << ")";
   }
 }
 

oi/type_graph/AddChildren.h

@@ -34,9 +34,15 @@ class TypeGraph;
 /*
  * AddChildren
  *
- * TODO
+ * Populates the "children" field of Class types.
- * what about children which inherit through a typedef?  don't think that'll
+ *
- * work yet
+ * DWARF only stores a mapping of [child -> parent], but sometimes we want the
+ * invserse: [parent -> child]. We must iterate over every single type to build
+ * up our own inverse mapping, before applying it to the relevant type nodes.
+ *
+ * This is expensive and only useful for types which make use of dynamic
+ * inheritance hierarchies (e.g. polymorphism), so is not done as part of the
+ * standard DrgnParser stage.
  */
 class AddChildren final : public RecursiveVisitor {
  public:

oi/type_graph/Flattener.cpp

@@ -46,15 +46,6 @@ void Flattener::accept(Type& type) {
 }
 
 namespace {
-// TODO this function is a massive hack. don't do it like this please
-Type& stripTypedefs(Type& type) {
-  Type* t = &type;
-  while (const Typedef* td = dynamic_cast<Typedef*>(t)) {
-    t = &td->underlyingType();
-  }
-  return *t;
-}
-
 void flattenParent(const Parent& parent,
                    std::vector<Member>& flattenedMembers) {
   Type& parentType = stripTypedefs(parent.type());

oi/type_graph/Types.cpp

@@ -122,4 +122,13 @@ bool Class::isDynamic() const {
   return false;
 }
 
+// TODO this function is a massive hack. don't do it like this please
+Type& stripTypedefs(Type& type) {
+  Type* t = &type;
+  while (const Typedef* td = dynamic_cast<Typedef*>(t)) {
+    t = &td->underlyingType();
+  }
+  return *t;
+}
+
 }  // namespace type_graph

oi/type_graph/Types.h

@@ -295,8 +295,8 @@ class Container : public Type {
 
 class Enum : public Type {
  public:
-  explicit Enum(const std::string& name, size_t size)
+  explicit Enum(std::string name, size_t size)
-      : name_(name), size_(size) {
+      : name_(std::move(name)), size_(size) {
   }
 
   DECLARE_ACCEPT
@@ -395,8 +395,8 @@ class Primitive : public Type {
 
 class Typedef : public Type {
  public:
-  explicit Typedef(NodeId id, const std::string& name, Type& underlyingType)
+  explicit Typedef(NodeId id, std::string name, Type& underlyingType)
-      : name_(name), underlyingType_(underlyingType), id_(id) {
+      : name_(std::move(name)), underlyingType_(underlyingType), id_(id) {
   }
 
   DECLARE_ACCEPT
@@ -520,6 +520,8 @@ class DummyAllocator : public Type {
   uint64_t align_;
 };
 
+Type& stripTypedefs(Type& type);
+
 }  // namespace type_graph
 
 #undef DECLARE_ACCEPT

test/CMakeLists.txt

@@ -49,6 +49,7 @@ target_link_libraries(integration_sleepy folly_headers)
 
 add_executable(test_type_graph
   main.cpp
+  test_add_children.cpp
   test_add_padding.cpp
   test_alignment_calc.cpp
   test_codegen.cpp

test/test_add_children.cpp

@@ -0,0 +1,176 @@
+#include <gtest/gtest.h>
+
+#include "oi/SymbolService.h"
+#include "oi/type_graph/AddChildren.h"
+#include "oi/type_graph/Printer.h"
+#include "oi/type_graph/TypeGraph.h"
+#include "test_drgn_parser.h"
+
+using namespace type_graph;
+
+class AddChildrenTest : public DrgnParserTest {
+ protected:
+  std::string run(std::string_view function, bool chaseRawPointers) override;
+};
+
+std::string AddChildrenTest::run(std::string_view function,
+                                 bool chaseRawPointers) {
+  TypeGraph typeGraph;
+  auto drgnParser = getDrgnParser(typeGraph, chaseRawPointers);
+  auto* drgnRoot = getDrgnRoot(function);
+
+  Type& type = drgnParser.parse(drgnRoot);
+  typeGraph.addRoot(type);
+
+  auto pass = AddChildren::createPass(drgnParser, *symbols_);
+  pass.run(typeGraph);
+
+  std::stringstream out;
+  Printer printer{out, typeGraph.size()};
+  printer.print(type);
+
+  return out.str();
+}
+
+TEST_F(AddChildrenTest, SimpleStruct) {
+  // Should do nothing
+  test("oid_test_case_simple_struct", R"(
+[0] Pointer
+[1]   Struct: SimpleStruct (size: 16)
+        Member: a (offset: 0)
+          Primitive: int32_t
+        Member: b (offset: 4)
+          Primitive: int8_t
+        Member: c (offset: 8)
+          Primitive: int64_t
+)");
+}
+
+TEST_F(AddChildrenTest, InheritanceStatic) {
+  // Should do nothing
+  test("oid_test_case_inheritance_access_public", R"(
+[0] Pointer
+[1]   Class: Public (size: 8)
+        Parent (offset: 0)
+[2]       Class: Base (size: 4)
+            Member: base_int (offset: 0)
+              Primitive: int32_t
+        Member: public_int (offset: 4)
+          Primitive: int32_t
+)");
+}
+
+TEST_F(AddChildrenTest, InheritancePolymorphic) {
+  testMultiCompiler("oid_test_case_inheritance_polymorphic_a_as_a", R"(
+[0]  Pointer
+[1]    Class: A (size: 16)
+         Member: _vptr$A (offset: 0)
+[2]        Pointer
+[3]          Pointer
+               Primitive: void
+         Member: int_a (offset: 8)
+           Primitive: int32_t
+         Function: ~A (virtual)
+         Function: myfunc (virtual)
+         Function: A
+         Function: A
+         Child
+[4]        Class: B (size: 40)
+             Parent (offset: 0)
+               [1]
+             Member: vec_b (offset: 16)
+[5]            Container: std::vector (size: 24)
+                 Param
+                   Primitive: int32_t
+                 Param
+[6]                Class: allocator<int> (size: 1)
+                     Param
+                       Primitive: int32_t
+                     Parent (offset: 0)
+[7]                    Typedef: __allocator_base<int>
+[8]                      Class: new_allocator<int> (size: 1)
+                           Param
+                             Primitive: int32_t
+                           Function: new_allocator
+                           Function: new_allocator
+                           Function: allocate
+                           Function: deallocate
+                           Function: _M_max_size
+                     Function: allocator
+                     Function: allocator
+                     Function: operator=
+                     Function: ~allocator
+                     Function: allocate
+                     Function: deallocate
+             Function: ~B (virtual)
+             Function: myfunc (virtual)
+             Function: B
+             Function: B
+             Child
+[9]            Class: C (size: 48)
+                 Parent (offset: 0)
+                   [4]
+                 Member: int_c (offset: 40)
+                   Primitive: int32_t
+                 Function: ~C (virtual)
+                 Function: myfunc (virtual)
+                 Function: C
+                 Function: C
+)",
+                    R"(
+[0]  Pointer
+[1]    Class: A (size: 16)
+         Member: _vptr.A (offset: 0)
+[2]        Pointer
+[3]          Pointer
+               Primitive: void
+         Member: int_a (offset: 8)
+           Primitive: int32_t
+         Function: operator=
+         Function: A
+         Function: A
+         Function: ~A (virtual)
+         Function: myfunc (virtual)
+         Child
+[4]        Class: B (size: 40)
+             Parent (offset: 0)
+               [1]
+             Member: vec_b (offset: 16)
+[5]            Container: std::vector (size: 24)
+                 Param
+                   Primitive: int32_t
+                 Param
+[6]                Class: allocator<int> (size: 1)
+                     Parent (offset: 0)
+[7]                    Class: new_allocator<int> (size: 1)
+                         Param
+                           Primitive: int32_t
+                         Function: new_allocator
+                         Function: new_allocator
+                         Function: allocate
+                         Function: deallocate
+                         Function: _M_max_size
+                     Function: allocator
+                     Function: allocator
+                     Function: operator=
+                     Function: ~allocator
+                     Function: allocate
+                     Function: deallocate
+             Function: operator=
+             Function: B
+             Function: B
+             Function: ~B (virtual)
+             Function: myfunc (virtual)
+             Child
+[8]            Class: C (size: 48)
+                 Parent (offset: 0)
+                   [4]
+                 Member: int_c (offset: 40)
+                   Primitive: int32_t
+                 Function: operator=
+                 Function: C
+                 Function: C
+                 Function: ~C (virtual)
+                 Function: myfunc (virtual)
+)");
+}