TypeGraph: Better handling for anonymous types

- Assign names to anonymous types
- Deduplicate all enums (anonymous or not)
- Add tests

oi/type_graph/DrgnParser.cpp

@@ -73,8 +73,6 @@ Primitive::Kind primitiveFloatKind(struct drgn_type* type) {
 
 }  // namespace
 
-// TODO type stubs
-
 Type& DrgnParser::parse(struct drgn_type* root) {
   depth_ = 0;
   return enumerateType(root);
@@ -161,11 +159,8 @@ Type& DrgnParser::enumerateClass(struct drgn_type* type) {
   if (container)
     return *container;
 
-  std::string name;
-  const char* type_tag = drgn_type_tag(type);
-  if (type_tag)
-    name = std::string(type_tag);
-  // else this is an anonymous type
+  const char* typeTag = drgn_type_tag(type);
+  std::string name = typeTag ? typeTag : "";
 
   auto size = get_drgn_type_size(type);
   int virtuality = 0;
@@ -418,17 +413,14 @@ void DrgnParser::enumerateClassFunctions(struct drgn_type* type,
 }
 
 Enum& DrgnParser::enumerateEnum(struct drgn_type* type) {
-  // TODO anonymous enums
-  // TODO incomplete enum?
-  std::string name = drgn_type_tag(type);
+  const char* typeTag = drgn_type_tag(type);
+  std::string name = typeTag ? typeTag : "";
   uint64_t size = get_drgn_type_size(type);
-  ;
   return makeType<Enum>(type, name, size);
 }
 
 Typedef& DrgnParser::enumerateTypedef(struct drgn_type* type) {
   std::string name = drgn_type_name(type);
-  // TODO anonymous typedefs?
 
   struct drgn_type* underlyingType = drgn_type_type(type).type;
   auto& t = enumerateType(underlyingType);
@@ -437,8 +429,7 @@ Typedef& DrgnParser::enumerateTypedef(struct drgn_type* type) {
 
 Type& DrgnParser::enumeratePointer(struct drgn_type* type) {
   if (!chasePointer()) {
-    // TODO dodgy nullptr - primitives should be handled as singletons
-    return makeType<Primitive>(nullptr, Primitive::Kind::UIntPtr);
+    return makeType<Primitive>(type, Primitive::Kind::UIntPtr);
   }
 
   struct drgn_type* pointeeType = drgn_type_type(type).type;

oi/type_graph/DrgnParser.h

@@ -29,6 +29,22 @@ struct ContainerInfo;
 
 namespace type_graph {
 
+/*
+ * DrgnParser
+ *
+ * Reads debug information from a drgn_type to build a type graph.
+ * Returns a reference to the Type node corresponding to the given drgn_type.
+ *
+ * DrgnParser tries to balance two philosophies:
+ * - Simplicity: containing minimal logic other than reading info from drgn
+ * - Performance: reading no more info from drgn than necessary
+ *
+ * For the most part we try to move logic out of DrgnParser and have later
+ * passes clean up the type graph, e.g. flattening parents. However, we do
+ * incorporate some extra logic here when it would allow us to read less DWARF
+ * information, e.g. matching containers in DrngParser means we don't read
+ * details about container internals.
+ */
 class DrgnParser {
  public:
   DrgnParser(TypeGraph& typeGraph,
@@ -63,11 +79,6 @@ class DrgnParser {
   void enumerateClassFunctions(struct drgn_type* type,
                                std::vector<Function>& functions);
 
-  // Store a mapping of drgn types to type graph nodes for deduplication during
-  // parsing. This stops us getting caught in cycles.
-  std::unordered_map<struct drgn_type*, std::reference_wrapper<Type>>
-      drgn_types_;
-
   template <typename T, typename... Args>
   T& makeType(struct drgn_type* drgnType, Args&&... args) {
     auto& newType = typeGraph_.makeType<T>(std::forward<Args>(args)...);
@@ -76,6 +87,11 @@ class DrgnParser {
   }
   bool chasePointer() const;
 
+  // Store a mapping of drgn types to type graph nodes for deduplication during
+  // parsing. This stops us getting caught in cycles.
+  std::unordered_map<struct drgn_type*, std::reference_wrapper<Type>>
+      drgn_types_;
+
   TypeGraph& typeGraph_;
   const std::vector<ContainerInfo>& containers_;
   int depth_;

oi/type_graph/NameGen.cpp

@@ -45,23 +45,33 @@ void NameGen::accept(Type& type) {
   type.accept(*this);
 }
 
+namespace {
 /*
  * Remove template parameters from the type name
  *
  * "std::vector<int>" -> "std::vector"
  */
-void NameGen::removeTemplateParams(std::string& name) {
+void removeTemplateParams(std::string& name) {
   auto template_start_pos = name.find('<');
   if (template_start_pos != std::string::npos)
     name.erase(template_start_pos);
 }
+}  // namespace
+
+void NameGen::deduplicate(std::string& name) {
+  if (name == "")
+    name = AnonPrefix;
+
+  // Append an incrementing number to ensure we don't get duplicates
+  name += "_";
+  name += std::to_string(n++);
+}
 
 void NameGen::visit(Class& c) {
   std::string name = c.name();
   removeTemplateParams(name);
-
-  // Append an incrementing number to ensure we don't get duplicates
-  c.setName(name + "_" + std::to_string(n++));
+  deduplicate(name);
+  c.setName(name);
 
   // Deduplicate member names. Duplicates may be present after flattening.
   for (size_t i = 0; i < c.members.size(); i++) {
@@ -117,6 +127,12 @@ void NameGen::visit(Container& c) {
   c.setName(name);
 }
 
+void NameGen::visit(Enum& e) {
+  std::string name = e.name();
+  deduplicate(name);
+  e.setName(name);
+}
+
 void NameGen::visit(Typedef& td) {
   /*
    * Treat like class names.
@@ -130,9 +146,8 @@ void NameGen::visit(Typedef& td) {
    */
   std::string name = td.name();
   removeTemplateParams(name);
-
-  // Append an incrementing number to ensure we don't get duplicates
-  td.setName(name + "_" + std::to_string(n++));
+  deduplicate(name);
+  td.setName(name);
 
   accept(td.underlyingType());
 }

oi/type_graph/NameGen.h

@@ -16,6 +16,7 @@
 #pragma once
 
 #include <functional>
+#include <string>
 #include <unordered_set>
 #include <vector>
 
@@ -26,6 +27,11 @@
 namespace type_graph {
 
 // TODO make all final
+/*
+ * NameGen
+ *
+ * Generates unique names for all types in a type graph.
+ */
 class NameGen final : public RecursiveVisitor {
  public:
   static Pass createPass();
@@ -36,11 +42,14 @@ class NameGen final : public RecursiveVisitor {
 
   void visit(Class& c) override;
   void visit(Container& c) override;
+  void visit(Enum& e) override;
   void visit(Typedef& td) override;
 
+  static const inline std::string AnonPrefix = "__oi_anon";
+
  private:
   void accept(Type& type) override;
-  void removeTemplateParams(std::string& name);
+  void deduplicate(std::string& name);
 
   std::unordered_set<Type*> visited_;
   int n = 0;

oi/type_graph/Types.h

@@ -324,6 +324,10 @@ class Enum : public Type {
     return name_;
   }
 
+  void setName(std::string name) {
+    name_ = std::move(name);
+  }
+
   virtual size_t size() const override {
     return size_;
   }

test/integration/enums.toml

@@ -17,6 +17,13 @@ definitions = '''
     CASE_B,
     CASE_C,
   };
+
+  struct Holder {
+    enum {
+      One,
+      Two,
+    } e;
+  };
 '''
 [cases]
   [cases.scoped]
@@ -31,3 +38,11 @@ definitions = '''
     param_types = ["UNSCOPED_ENUM"]
     setup = "return {};"
     expect_json = '[{"staticSize":4, "dynamicSize":0}]'
+  [cases.anonymous]
+    skip = "TreeBuilder crashes" # https://github.com/facebookexperimental/object-introspection/issues/232
+    param_types = ["Holder&"]
+    setup = "return {};"
+    expect_json = '''[
+      {"staticSize":4, "dynamicSize":0, "exclusiveSize":0, "members":[
+        {"name":"e", "staticSize":4, "dynamicSize":0, "exclusiveSize":4}
+    ]}]'''

test/integration/typedefs.toml

@@ -3,6 +3,7 @@ definitions = '''
   typedef uint64_t TdUInt64;
   using UsingUInt64 = uint64_t;
   using IntVector = std::vector<int>;
+  using Anonymous = struct { int n; };
 '''
 [cases]
   [cases.c_style]
@@ -59,3 +60,29 @@ definitions = '''
           "NOT":"members"
         }
       ]}]'''
+  [cases.anonymous]
+    oil_skip = "TreeBuilder crashes" # https://github.com/facebookexperimental/object-introspection/issues/232
+    param_types = ["const Anonymous&"]
+    setup = "return {};"
+    expect_json = '''[{
+      "staticSize":4,
+      "dynamicSize":0,
+      "exclusiveSize":0,
+      "isTypedef":true,
+      "typeName":"Anonymous",
+      "members":[
+        {
+          "staticSize":4,
+          "dynamicSize":0,
+          "exclusiveSize":0,
+          "isTypedef":false,
+          "members":[
+            {
+              "staticSize":4,
+              "dynamicSize":0,
+              "exclusiveSize":4,
+              "isTypedef":false,
+              "name": "n"
+            }
+        ]}
+      ]}]'''

test/test_name_gen.cpp

@@ -210,6 +210,17 @@ TEST(NameGenTest, ContainerParamsValue) {
   EXPECT_EQ(mycontainer.name(), "std::vector<123, MyEnum::OptionC>");
 }
 
+TEST(NameGenTest, Enum) {
+  auto myenum0 = Enum{"MyEnum", 4};
+  auto myenum1 = Enum{"MyEnum", 4};
+
+  NameGen nameGen;
+  nameGen.generateNames({myenum0, myenum1});
+
+  EXPECT_EQ(myenum0.name(), "MyEnum_0");
+  EXPECT_EQ(myenum1.name(), "MyEnum_1");
+}
+
 TEST(NameGenTest, Array) {
   auto myparam1 = Class{0, Class::Kind::Struct, "MyParam", 13};
   auto myparam2 = Class{1, Class::Kind::Struct, "MyParam", 13};
@@ -332,3 +343,18 @@ TEST(NameGenTest, ContainerCycle) {
   EXPECT_EQ(myclass.name(), "MyClass_0");
   EXPECT_EQ(container.name(), "std::vector<MyClass_0>");
 }
+
+TEST(NameGenTest, AnonymousTypes) {
+  auto myint = Primitive{Primitive::Kind::Int32};
+
+  auto myclass = Class{0, Class::Kind::Class, "", 69};
+  auto myenum = Enum{"", 4};
+  auto mytypedef = Typedef{1, "", myint};
+
+  NameGen nameGen;
+  nameGen.generateNames({myclass, myenum, mytypedef});
+
+  EXPECT_EQ(myclass.name(), "__oi_anon_0");
+  EXPECT_EQ(myenum.name(), "__oi_anon_1");
+  EXPECT_EQ(mytypedef.name(), "__oi_anon_2");
+}