CodeGen v2: Finish polymorphic inheritance support

ajor · ajor · commit be562f50d234 · 2023-07-07T08:09:49.000-07:00
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -20,7 +20,6 @@ workflows:
             - build-gcc
           oid_test_args: "-ftype-graph"
           tests_regex: "OidIntegration\\..*"
-          exclude_regex: ".*inheritance_polymorphic.*"
       - test:
           name: test-typed-data-segment-gcc
           requires:
@@ -57,7 +56,7 @@ workflows:
           oid_test_args: "-ftype-graph"
           tests_regex: "OidIntegration\\..*"
           # Tests disabled due to bad DWARF generated by the old clang compiler in CI
-          exclude_regex: ".*inheritance_polymorphic.*|.*fbstring.*|.*std_string_*|.*multi_arg_tb_.*|.*ignored_a"
+          exclude_regex: ".*fbstring.*|.*std_string_*|.*multi_arg_tb_.*|.*ignored_a"
 
 executors:
   ubuntu-docker:
diff --git a/oi/CodeGen.cpp b/oi/CodeGen.cpp
@@ -366,11 +366,15 @@ void addStandardGetSizeFuncDefs(std::string& code) {
     }
   )";
 }
+}  // namespace
 
-void getClassSizeFuncDecl(const Class& c, std::string& code) {
+void CodeGen::getClassSizeFuncDecl(const Class& c, std::string& code) const {
+  if (config_.features[Feature::PolymorphicInheritance] && c.isDynamic()) {
+    code += "void getSizeTypeConcrete(const " + c.name() +
+            " &t, size_t &returnArg);\n";
+  }
   code += "void getSizeType(const " + c.name() + " &t, size_t &returnArg);\n";
 }
-}  // namespace
 
 /*
  * Generates a getSizeType function for the given concrete class.
@@ -421,7 +425,7 @@ void CodeGen::getClassSizeFuncConcrete(std::string_view funcName,
   code += "}\n";
 }
 
-void CodeGen::getClassSizeFuncDef(const Class& c, std::string& code) {
+void CodeGen::getClassSizeFuncDef(const Class& c, std::string& code) const {
   if (!config_.features[Feature::PolymorphicInheritance] || !c.isDynamic()) {
     // Just directly use the concrete size function as this class' getSizeType()
     getClassSizeFuncConcrete("getSizeType", c, code);
@@ -438,9 +442,7 @@ void CodeGen::getClassSizeFuncDef(const Class& c, std::string& code) {
     if (childClass == nullptr) {
       abort();  // TODO
     }
-    //      TODO:
-    //      auto fqChildName = *fullyQualifiedName(child);
-    auto fqChildName = "TODO - implement me";
+    auto fqChildName = childClass->fqName();
 
     // We must split this assignment and append because the C++ standard lacks
     // an operator for concatenating std::string and std::string_view...
@@ -511,8 +513,10 @@ void getContainerSizeFuncDef(std::unordered_set<const ContainerInfo*>& used,
       boost::format(c.containerInfo_.codegen.func) % c.containerInfo_.typeName;
   code += fmt.str();
 }
+}  // namespace
 
-void addGetSizeFuncDecls(const TypeGraph& typeGraph, std::string& code) {
+void CodeGen::addGetSizeFuncDecls(const TypeGraph& typeGraph,
+                                  std::string& code) const {
   for (const Type& t : typeGraph.finalTypes) {
     if (const auto* c = dynamic_cast<const Class*>(&t)) {
       getClassSizeFuncDecl(*c, code);
@@ -522,8 +526,6 @@ void addGetSizeFuncDecls(const TypeGraph& typeGraph, std::string& code) {
   }
 }
 
-}  // namespace
-
 void CodeGen::addGetSizeFuncDefs(const TypeGraph& typeGraph,
                                  std::string& code) {
   for (const Type& t : typeGraph.finalTypes) {
diff --git a/oi/CodeGen.h b/oi/CodeGen.h
@@ -66,9 +66,13 @@ class CodeGen {
       thriftIssetMembers_;
 
   void genDefsThrift(const type_graph::TypeGraph& typeGraph, std::string& code);
+  void addGetSizeFuncDecls(const type_graph::TypeGraph& typeGraph,
+                           std::string& code) const;
+  void getClassSizeFuncDecl(const type_graph::Class& c,
+                            std::string& code) const;
   void addGetSizeFuncDefs(const type_graph::TypeGraph& typeGraph,
                           std::string& code);
-  void getClassSizeFuncDef(const type_graph::Class& c, std::string& code);
+  void getClassSizeFuncDef(const type_graph::Class& c, std::string& code) const;
   void getClassSizeFuncConcrete(std::string_view funcName,
                                 const type_graph::Class& c,
                                 std::string& code) const;
diff --git a/oi/type_graph/AddChildren.cpp b/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,20 +130,20 @@ void AddChildren::recordChildren(drgn_type* type) {
     }
 
     const char* parentName = drgn_type_tag(parent);
-    if (parentName == nullptr) {
-      //      VLOG(1) << "No name for parent class (" << parent << ") of "
-      //              << drgn_type_tag(type);
+    if (!parentName) {
       continue;
     }
 
     /*
      * 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 << ")";
   }
 }
 
diff --git a/oi/type_graph/AddChildren.h b/oi/type_graph/AddChildren.h
@@ -34,6 +34,16 @@ class TypeGraph;
 /*
  * AddChildren
  *
+ * Populates the "children" field of Class types.
+ *
+ * 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.
+ *
  * TODO
  * what about children which inherit through a typedef?  don't think that'll
  * work yet
diff --git a/oi/type_graph/Flattener.cpp b/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());
diff --git a/oi/type_graph/NameGen.cpp b/oi/type_graph/NameGen.cpp
@@ -66,6 +66,10 @@ void NameGen::visit(Class& c) {
   // Deduplicate member names. Duplicates may be present after flattening.
   for (size_t i = 0; i < c.members.size(); i++) {
     c.members[i].name += "_" + std::to_string(i);
+
+    // GCC includes dots in vptr member names, e.g. "_vptr.MyClass"
+    // These aren't valid in C++, so we must replace them
+    std::replace(c.members[i].name.begin(), c.members[i].name.end(), '.', '$');
   }
 
   for (const auto& param : c.templateParams) {
diff --git a/oi/type_graph/Types.cpp b/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
diff --git a/oi/type_graph/Types.h b/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)
-      : name_(name), size_(size) {
+  explicit Enum(std::string name, size_t 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)
-      : name_(name), underlyingType_(underlyingType), id_(id) {
+  explicit Typedef(NodeId id, std::string name, Type& underlyingType)
+      : 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
diff --git a/test/CMakeLists.txt b/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
diff --git a/test/test_add_children.cpp b/test/test_add_children.cpp
diff --git a/test/test_drgn_parser.cpp b/test/test_drgn_parser.cpp
diff --git a/test/test_drgn_parser.h b/test/test_drgn_parser.h
