type_graph: add CycleFinder pass

.circleci/config.yml

@@ -27,7 +27,7 @@ workflows:
             - build-gcc
           oid_test_args: "-ftyped-data-segment"
           tests_regex: "OidIntegration\\..*"
-          exclude_regex: ".*inheritance_polymorphic.*|.*pointers.*|.*arrays_member_int0|.*cycles_.*|OidIntegration.unions.*|.*thrift_unions_dynamic_.*"
+          exclude_regex: ".*inheritance_polymorphic.*|.*pointers.*|.*arrays_member_int0|OidIntegration.unions.*|.*thrift_unions_dynamic_.*|.*type_cycles.*"
       - test:
           name: test-tree-builder-type-checking-gcc
           requires:

oi/CodeGen.cpp

@@ -28,6 +28,7 @@
 #include "type_graph/AddChildren.h"
 #include "type_graph/AddPadding.h"
 #include "type_graph/AlignmentCalc.h"
+#include "type_graph/CycleFinder.h"
 #include "type_graph/DrgnParser.h"
 #include "type_graph/Flattener.h"
 #include "type_graph/NameGen.h"
@@ -40,6 +41,7 @@
 
 using type_graph::Class;
 using type_graph::Container;
+using type_graph::CycleBreaker;
 using type_graph::Enum;
 using type_graph::Member;
 using type_graph::Type;
@@ -126,6 +128,12 @@ void addIncludes(const TypeGraph& typeGraph,
   }
 }
 
+void genDeclsCycleBreaker(const CycleBreaker& b, std::string& code) {
+  code += "struct ";
+  code += b.name();
+  code += ";\n";
+}
+
 void genDeclsClass(const Class& c, std::string& code) {
   if (c.kind() == Class::Kind::Union)
     code += "union ";
@@ -161,6 +169,8 @@ void genDecls(const TypeGraph& typeGraph, std::string& code) {
       genDeclsClass(*c, code);
     } else if (const auto* e = dynamic_cast<const Enum*>(&t)) {
       genDeclsEnum(*e, code);
+    } else if (const auto* b = dynamic_cast<const CycleBreaker*>(&t)) {
+      genDeclsCycleBreaker(*b, code);
     }
   }
 }
@@ -731,6 +741,22 @@ void getContainerTypeHandler(std::unordered_set<const ContainerInfo*>& used,
   code += fmt.str();
 }
 
+void addCycleBreakerTypeHandler(const CycleBreaker& b, std::string& code) {
+  code += (boost::format(R"(template <typename DB>
+struct TypeHandler<DB, %1%> {
+ public:
+  struct type {
+    type(DB buf) : _buf(buf) {}
+    DB _buf;
+  };
+  static types::st::Unit<DB> getSizeType(const %1%& t, TypeHandler<DB, %1%>::type ret) {
+    return getSizeType<DB>(reinterpret_cast<const %2%&>(t), ret._buf);
+  }
+};)") % b.name() %
+           b.to().name())
+              .str();
+}
+
 }  // namespace
 
 void CodeGen::addTypeHandlers(const TypeGraph& typeGraph, std::string& code) {
@@ -739,6 +765,8 @@ void CodeGen::addTypeHandlers(const TypeGraph& typeGraph, std::string& code) {
       getClassTypeHandler(*c, code);
     } else if (const auto* con = dynamic_cast<const Container*>(&t)) {
       getContainerTypeHandler(definedContainers_, *con, code);
+    } else if (const auto* b = dynamic_cast<const CycleBreaker*>(&t)) {
+      addCycleBreakerTypeHandler(*b, code);
     }
   }
 }
@@ -798,6 +826,9 @@ void CodeGen::transform(type_graph::TypeGraph& typeGraph) {
   }
   pm.addPass(type_graph::RemoveIgnored::createPass(config_.membersToStub));
   pm.addPass(type_graph::RemoveTopLevelPointer::createPass());
+  if (config_.features[Feature::TypedDataSegment]) {
+    pm.addPass(type_graph::CycleFinder::createPass());
+  }
 
   // 2. Fixup passes to repair type graph after partial transformations
   pm.addPass(type_graph::AddPadding::createPass(config_.features));

oi/type_graph/CMakeLists.txt

@@ -2,6 +2,7 @@ add_library(type_graph
   AddChildren.cpp
   AddPadding.cpp
   AlignmentCalc.cpp
+  CycleFinder.cpp
   DrgnParser.cpp
   Flattener.cpp
   NameGen.cpp

oi/type_graph/CycleFinder.cpp

@@ -0,0 +1,106 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include "CycleFinder.h"
+
+#include <glog/logging.h>
+
+#include "TypeGraph.h"
+
+namespace type_graph {
+
+Pass CycleFinder::createPass() {
+  auto fn = [](TypeGraph& typeGraph) {
+    CycleFinder finder{typeGraph};
+    for (auto& type : typeGraph.rootTypes()) {
+      finder.accept(type);
+    }
+  };
+
+  return Pass("CycleFinder", fn);
+}
+
+void CycleFinder::accept(Type& type) {
+  if (push(type)) {
+    type.accept(*this);
+    pop();
+    return;
+  }
+
+  auto search = std::find_if(currentPath_.begin(), currentPath_.end(),
+                             [&type](const Type& t) { return &t == &type; });
+  CHECK(search != currentPath_.end()) << "set/stack invariant violated";
+
+  breakCycle(std::span{search, currentPath_.end()});
+}
+
+bool CycleFinder::push(Type& t) {
+  if (!currentPathSet_.emplace(&t).second)
+    return false;
+  currentPath_.emplace_back(t);
+  return true;
+}
+
+void CycleFinder::pop() {
+  const Type& t = currentPath_.back();
+  currentPath_.pop_back();
+  currentPathSet_.erase(&t);
+}
+
+CycleBreaker& CycleFinder::edge(Type& t) {
+  if (auto it = replacements_.find(&t); it != replacements_.end()) {
+    return it->second;
+  } else {
+    auto& e = typeGraph_.makeType<CycleBreaker>(t);
+    replacements_.emplace(&t, e);
+    return e;
+  }
+}
+
+void CycleFinder::breakCycle(std::span<std::reference_wrapper<Type>> cycle) {
+  if (cycle.size() >= 2) {
+    Type& from = cycle.back();
+    Type& to = cycle.front();
+
+    if (auto* p = dynamic_cast<Pointer*>(&from);
+        p && &p->pointeeType() == &to) {
+      p->setPointeeType(edge(to));
+      return;
+    }
+
+    if (auto* c = dynamic_cast<Container*>(&from)) {
+      bool done = false;
+      for (auto& param : c->templateParams) {
+        if (param.type() == &to) {
+          param.setType(&edge(to));
+          done = true;
+        }
+      }
+      if (done)
+        return;
+    }
+  }
+
+  // Unhandled, throw a descriptive error.
+  std::string names;
+  for (const Type& t : cycle) {
+    names += t.name();
+    names += ", ";
+  }
+  names += cycle.front().get().name();
+  throw std::logic_error("Unable to handle type cycle: " + names);
+}
+
+}  // namespace type_graph

oi/type_graph/CycleFinder.h

@@ -0,0 +1,67 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#pragma once
+
+#include <span>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
+
+#include "PassManager.h"
+#include "Types.h"
+#include "Visitor.h"
+
+namespace type_graph {
+
+class TypeGraph;
+
+/*
+ * CycleFinder
+ *
+ * Find a set of edges that must be broken in order to convert the type graph
+ * from a directed graph to a directed acyclic graph (DAG).
+ */
+class CycleFinder : public RecursiveVisitor {
+ public:
+  static Pass createPass();
+
+  CycleFinder(TypeGraph& typeGraph) : typeGraph_(typeGraph) {
+  }
+
+  using RecursiveVisitor::accept;
+  using RecursiveVisitor::visit;
+
+  void accept(Type& type) override;
+
+  void visit(CycleBreaker&) override{
+      // Do not follow a cyclebreaker as it has already been followed
+  };
+
+ private:
+  void breakCycle(std::span<std::reference_wrapper<Type>>);
+  bool push(Type& t);
+  void pop();
+  CycleBreaker& edge(Type& t);
+
+  TypeGraph& typeGraph_;
+
+  std::vector<std::reference_wrapper<Type>> currentPath_;
+  std::unordered_set<const Type*> currentPathSet_;
+  std::unordered_map<const Type*, std::reference_wrapper<CycleBreaker>>
+      replacements_;
+};
+
+}  // namespace type_graph

oi/type_graph/NameGen.cpp

@@ -152,4 +152,28 @@ void NameGen::visit(Typedef& td) {
   accept(td.underlyingType());
 }
 
+void NameGen::visit(CycleBreaker& b) {
+  accept(b.to());
+
+  std::string name = "fake_" + b.to().name();
+  std::transform(name.cbegin(), name.cend(), name.begin(), [](const char& c) {
+    switch (c) {
+      case '*':
+        return 'P';
+      case '<':
+      case '>':
+        return 'T';
+      case ':':
+      case ',':
+      case ' ':
+        return '_';
+      default:
+        return c;
+    }
+  });
+
+  deduplicate(name);
+  b.setName(name);
+}
+
 }  // namespace type_graph

oi/type_graph/NameGen.h

@@ -44,6 +44,7 @@ class NameGen final : public RecursiveVisitor {
   void visit(Container& c) override;
   void visit(Enum& e) override;
   void visit(Typedef& td) override;
+  void visit(CycleBreaker& b) override;
 
   static const inline std::string AnonPrefix = "__oi_anon";
 

oi/type_graph/Printer.cpp

@@ -132,6 +132,14 @@ void Printer::visit(const DummyAllocator& d) {
   print(d.allocType());
 }
 
+void Printer::visit(const CycleBreaker& b) {
+  if (prefix(&b))
+    return;
+
+  out_ << "CycleBreaker" << std::endl;
+  print(b.to());
+}
+
 bool Printer::prefix(const Type* type) {
   int indent = baseIndent_ + depth_ * 2;
 

oi/type_graph/Printer.h

@@ -41,6 +41,7 @@ class Printer : public ConstVisitor {
   void visit(const Pointer& p) override;
   void visit(const Dummy& d) override;
   void visit(const DummyAllocator& d) override;
+  void visit(const CycleBreaker& d) override;
 
  private:
   bool prefix(const Type* type = nullptr);

oi/type_graph/TopoSorter.cpp

@@ -117,6 +117,11 @@ void TopoSorter::visit(Typedef& td) {
   sortedTypes_.push_back(td);
 }
 
+void TopoSorter::visit(CycleBreaker& b) {
+  accept(b.to());
+  sortedTypes_.push_back(b);
+}
+
 void TopoSorter::visit(Pointer& p) {
   // Pointers do not create a dependency, but we do still care about the types
   // they point to, so delay them until the end.

oi/type_graph/TopoSorter.h

@@ -46,6 +46,7 @@ class TopoSorter : public RecursiveVisitor {
   void visit(Enum& e) override;
   void visit(Typedef& td) override;
   void visit(Pointer& p) override;
+  void visit(CycleBreaker& p) override;
 
  private:
   std::unordered_set<Type*> visited_;

oi/type_graph/Types.h

@@ -46,7 +46,8 @@
   X(Typedef)         \
   X(Pointer)         \
   X(Dummy)           \
-  X(DummyAllocator)
+  X(DummyAllocator)  \
+  X(CycleBreaker)
 
 struct ContainerInfo;
 
@@ -155,6 +156,10 @@ class TemplateParam {
   TemplateParam(std::string value) : value(std::move(value)) {
   }
 
+  void setType(Type* type) {
+    type_ = type;
+  }
+
   Type* type() const {
     return type_;
   }
@@ -484,7 +489,7 @@ class Pointer : public Type {
   DECLARE_ACCEPT
 
   virtual std::string name() const override {
-    return pointeeType_.name() + "*";
+    return pointeeType_.get().name() + "*";
   }
 
   virtual size_t size() const override {
@@ -499,12 +504,16 @@ class Pointer : public Type {
     return id_;
   }
 
+  void setPointeeType(Type& type) {
+    pointeeType_ = type;
+  }
+
   Type& pointeeType() const {
     return pointeeType_;
   }
 
  private:
-  Type& pointeeType_;
+  std::reference_wrapper<Type> pointeeType_;
   NodeId id_ = -1;
 };
 
@@ -587,6 +596,46 @@ class DummyAllocator : public Type {
   uint64_t align_;
 };
 
+class CycleBreaker : public Type {
+ public:
+  explicit CycleBreaker(NodeId id, Type& to) : to_(to), id_(id) {
+  }
+
+  static inline constexpr bool has_node_id = true;
+
+  DECLARE_ACCEPT
+
+  virtual std::string name() const override {
+    return name_;
+  }
+
+  void setName(std::string name) {
+    name_ = std::move(name);
+  }
+
+  virtual size_t size() const override {
+    return sizeof(uintptr_t);
+  }
+
+  virtual uint64_t align() const override {
+    return size();
+  }
+
+  virtual NodeId id() const override {
+    return id_;
+  }
+
+  Type& to() const {
+    return to_;
+  }
+
+ private:
+  Type& to_;
+
+  std::string name_;
+  NodeId id_ = -1;
+};
+
 Type& stripTypedefs(Type& type);
 
 }  // namespace type_graph

oi/type_graph/Visitor.h

@@ -108,6 +108,9 @@ class RecursiveVisitor : public Visitor {
   virtual void visit(DummyAllocator& d) {
     accept(d.allocType());
   }
+  virtual void visit(CycleBreaker& b) {
+    accept(b.to());
+  }
 };
 
 /*

test/CMakeLists.txt

@@ -40,6 +40,7 @@ add_executable(test_type_graph
   test_add_padding.cpp
   test_alignment_calc.cpp
   test_codegen.cpp
+  test_cycle_finder.cpp
   test_drgn_parser.cpp
   test_flattener.cpp
   test_name_gen.cpp

test/TypeGraphParser.cpp

@@ -72,6 +72,10 @@ ContainerInfo& getContainerInfo(std::string_view name) {
     static ContainerInfo info{"std::allocator", DUMMY_TYPE, "memory"};
     return info;
   }
+  if (name == "std::unique_ptr") {
+    static ContainerInfo info{"std::unique_ptr", UNIQ_PTR_TYPE, "memory"};
+    return info;
+  }
   throw TypeGraphParserError{"Unsupported container: " + std::string{name}};
 }
 

test/integration/cycles.toml

@@ -20,6 +20,13 @@ definitions = '''
     Wrapper(T t_) : t(t_) {};
     T t;
   };
+
+  struct Foo {
+    struct Bar* b;
+  };
+  struct Bar {
+    struct Foo* f;
+  };
 '''
 [cases]
   [cases.raw_ptr]
@@ -153,6 +160,61 @@ definitions = '''
     ]}]}]}]}]}]}]}]
     '''
 
+  [cases.raw_ptr_via_another]
+    oil_disable = "oil can't chase pointers safely"
+    param_types = ["Foo&"]
+    setup = '''
+      Foo *first = new Foo{nullptr};
+      Bar *second = new Bar{nullptr};
+      first->b = second;
+      second->f = first;
+      return *first;
+    '''
+    cli_options = ["-fchase-raw-pointers"]
+    expect_json = '''
+    [{
+      "typeName": "Foo",
+      "staticSize": 8,
+      "dynamicSize": 16,
+      "exclusiveSize": 0,
+      "members": [
+        {
+          "typeName": "struct Bar *",
+          "staticSize": 8,
+          "dynamicSize": 16,
+          "exclusiveSize": 8,
+          "NOT": {"pointer": 0},
+          "members": [
+            {
+              "typeName": "Bar",
+              "staticSize": 8,
+              "dynamicSize": 8,
+              "exclusiveSize": 0,
+              "members": [
+                {
+                  "typeName": "struct Foo *",
+                  "staticSize": 8,
+                  "dynamicSize": 8,
+                  "exclusiveSize": 8,
+                  "NOT": {"pointer": 0},
+                  "members": [
+                    {
+                      "typeName": "Foo",
+                      "staticSize": 8,
+                      "dynamicSize": 0,
+                      "exclusiveSize": 0,
+                      "members": [
+                        {
+                          "typePath": "b",
+                          "typeName": "struct Bar *",
+                          "isTypedef": false,
+                          "staticSize": 8,
+                          "dynamicSize": 0,
+                          "exclusiveSize": 8,
+                          "NOT": {"pointer": 0}
+      }]}]}]}]}]}]
+    '''
+
   [cases.unique_ptr]
     param_types = ["std::reference_wrapper<UniqueNode>&"]
     setup = '''

test/integration/type_cycles.toml

@@ -0,0 +1,35 @@
+includes = ["vector"]
+definitions = '''
+struct Tree {
+  std::vector<Tree> children;
+};
+
+
+'''
+
+[cases]
+  [cases.tree_owns_trees_via_vector]
+    param_types = ["Tree&"]
+    setup = '''
+      Tree t;
+      t.children.emplace_back(Tree{});
+      return t;
+    '''
+    expect_json = '''
+    [{
+      "typeName": "Tree",
+      "staticSize": 24,
+      "dynamicSize": 24,
+      "members": [
+        {
+          "name": "children",
+          "staticSize": 24,
+          "dynamicSize": 24,
+          "length":1,
+          "capacity":1,
+          "elementStaticSize":24,
+          "members": [{"typeName": "Tree", "staticSize": 24, "dynamicSize": 0}]
+        }
+      ]
+    }]
+    '''

test/test_cycle_finder.cpp

@@ -0,0 +1,95 @@
+#include <gtest/gtest.h>
+
+#include "oi/type_graph/CycleFinder.h"
+#include "oi/type_graph/Types.h"
+#include "test/type_graph_utils.h"
+
+using namespace type_graph;
+
+TEST(CycleFinderTest, NoCycles) {
+  // No change
+  // Original and After:
+  //   struct MyStruct { int n0; };
+  //   class MyClass {
+  //     int n;
+  //     MyEnum e;
+  //     MyStruct mystruct;
+  //   };
+  testNoChange(CycleFinder::createPass(), R"(
+[0] Class: MyClass (size: 12)
+      Member: n (offset: 0)
+        Primitive: int32_t
+      Member: e (offset: 4)
+        Enum: MyEnum (size: 4)
+      Member: mystruct (offset: 8)
+[1]     Struct: MyStruct (size: 4)
+          Member: n0 (offset: 0)
+            Primitive: int32_t
+)");
+}
+
+TEST(CycleFinderTest, RawPointer) {
+  // Original:
+  //   class RawNode {
+  //     int value;
+  //     class RawNode* next;
+  //   };
+  //
+  // After:
+  //   class fake_RawNode;
+  //   class RawNode {
+  //     int value;
+  //     fake_RawNode* next;
+  //   };
+  test(CycleFinder::createPass(), R"(
+[0] Struct: RawNode (size: 16)
+      Member: value (offset: 0)
+        Primitive: int32_t
+      Member: next (offset: 8)
+[1]     Pointer
+          [0]
+)",
+       R"(
+[0] Struct: RawNode (size: 16)
+      Member: value (offset: 0)
+        Primitive: int32_t
+      Member: next (offset: 8)
+[1]     Pointer
+[2]       CycleBreaker
+            [0]
+)");
+}
+
+TEST(CycleFinderTest, UniquePointer) {
+  // Original:
+  //   class UniqueNode {
+  //     int value;
+  //     std::unique_ptr<struct UniqueNode> next;
+  //   };
+  //
+  // After:
+  //   class fake_UniqueNode;
+  //   class UniqueNode {
+  //     int value;
+  //     std::unique_ptr<fake_UniqueNode> next;
+  //   };
+  test(CycleFinder::createPass(), R"(
+[0] Struct: UniqueNode (size: 16)
+      Member: value (offset: 0)
+        Primitive: int32_t
+      Member: next (offset: 8)
+[1]     Container: std::unique_ptr (size: 8)
+          Param
+            [0]
+)",
+       R"(
+[0] Struct: UniqueNode (size: 16)
+      Member: value (offset: 0)
+        Primitive: int32_t
+      Member: next (offset: 8)
+[1]     Container: std::unique_ptr (size: 8)
+          Param
+[2]         CycleBreaker
+              [0]
+)");
+}

test/test_name_gen.cpp

@@ -358,3 +358,21 @@ TEST(NameGenTest, AnonymousTypes) {
   EXPECT_EQ(myenum.name(), "__oi_anon_1");
   EXPECT_EQ(mytypedef.name(), "__oi_anon_2");
 }
+
+TEST(NameGenTest, CycleBreaker) {
+  auto myint = Primitive{Primitive::Kind::Int32};
+
+  auto myclass = Class{0, Class::Kind::Class, "MyClass", 69};
+  auto mycontainer = getVector(1);
+  mycontainer.templateParams.push_back(myclass);
+  auto pointer = Pointer{2, mycontainer};
+  auto cycle = CycleBreaker{3, pointer};
+
+  NameGen nameGen;
+  nameGen.generateNames({cycle});
+
+  EXPECT_EQ(myclass.name(), "MyClass_0");
+  EXPECT_EQ(mycontainer.name(), "std::vector<MyClass_0>");
+  EXPECT_EQ(pointer.name(), "std::vector<MyClass_0>*");
+  EXPECT_EQ(cycle.name(), "fake_std__vectorTMyClass_0TP_1");
+}