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_.*|.*cycles_unique_ptr|.*cycles_shared_ptr"
       - 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);
     }
   }
 }
@@ -732,6 +742,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) {
@@ -740,6 +766,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);
     }
   }
 }
@@ -799,6 +827,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,57 @@
+/*
+ * 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 "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 (!currentPathSet_.emplace(&type).second) {
+    auto& from = currentPath_.top().get();
+
+    auto& edge = typeGraph_.makeType<CycleBreaker>(from, type);
+    if (auto* p = dynamic_cast<Pointer*>(&from)) {
+      p->setPointeeType(edge);
+    } else if (auto* c = dynamic_cast<Container*>(&from)) {
+      throw std::runtime_error("TODO: cycle is from a Container");
+    } else {
+      throw std::logic_error(
+          "all cycles are expected to come from a Pointer or Container");
+    }
+
+    return;
+  }
+
+  // no cycle, add to path and continue
+  currentPath_.push(type);
+  type.accept(*this);
+  currentPath_.pop();
+  currentPathSet_.erase(&type);
+}
+
+}  // namespace type_graph

oi/type_graph/CycleFinder.h

@@ -0,0 +1,58 @@
+/*
+ * 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 <stack>
+#include <unordered_set>
+
+#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:
+  TypeGraph& typeGraph_;
+
+  std::stack<std::reference_wrapper<Type>> currentPath_;
+  std::unordered_set<const Type*> currentPathSet_;
+};
+
+}  // namespace type_graph

oi/type_graph/NameGen.cpp

@@ -152,4 +152,32 @@ void NameGen::visit(Typedef& td) {
   accept(td.underlyingType());
 }
 
+void NameGen::visit(CycleBreaker& b) {
+  // Assume we have already visited from() (graph is directed), and that it has
+  // a name. Visit to() first as it likely hasn't been visited yet.
+  accept(b.to());
+
+  std::string name = b.name();
+  deduplicate(name);
+
+  std::for_each(name.begin(), name.end(), [](char& c) {
+    switch (c) {
+      case '*':
+        c = 'P';
+        return;
+      case '<':
+      case '>':
+        c = 't';
+        return;
+      case ':':
+      case ',':
+      case ' ':
+        c = '_';
+        return;
+    }
+  });
+
+  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

@@ -30,8 +30,10 @@
  */
 
 #include <cstddef>
+#include <functional>
 #include <optional>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "oi/ContainerInfo.h"
@@ -46,7 +48,8 @@
   X(Typedef)         \
   X(Pointer)         \
   X(Dummy)           \
-  X(DummyAllocator)
+  X(DummyAllocator)  \
+  X(CycleBreaker)
 
 struct ContainerInfo;
 
@@ -484,7 +487,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 +502,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 +594,53 @@ class DummyAllocator : public Type {
   uint64_t align_;
 };
 
+class CycleBreaker : public Type {
+ public:
+  explicit CycleBreaker(NodeId id, Type& from, Type& to)
+      : from_(from), to_(to), id_(id) {
+    name_ = from.name() + "_" + to.name();
+  }
+
+  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& from() const {
+    return from_;
+  }
+
+  Type& to() const {
+    return to_;
+  }
+
+ private:
+  Type& from_;
+  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

@@ -53,6 +53,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/integration/cycles.toml

@@ -14,6 +14,13 @@ definitions = '''
     int value;
     std::shared_ptr<struct SharedNode> next;
   };
+
+  struct Foo {
+    struct Bar* b;
+  };
+  struct Bar {
+    struct Foo* f;
+  };
 '''
 [cases]
   [cases.raw_ptr]
@@ -89,6 +96,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/test_cycle_finder.cpp

@@ -0,0 +1,61 @@
+#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 RawNodeP_RawNode;
+  //   class RawNode {
+  //     int value;
+  //     RawNodeP_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]
+)");
+}