From 7ba47abb47058eb568c6294266c62568c76c05f4 Mon Sep 17 00:00:00 2001 From: Jake Hillion Date: Wed, 5 Jul 2023 04:49:49 -0700 Subject: [PATCH] type_graph: add CycleFinder pass --- .circleci/config.yml | 2 +- oi/CodeGen.cpp | 31 +++++++++ oi/type_graph/CMakeLists.txt | 1 + oi/type_graph/CycleFinder.cpp | 106 ++++++++++++++++++++++++++++++ oi/type_graph/CycleFinder.h | 67 +++++++++++++++++++ oi/type_graph/NameGen.cpp | 24 +++++++ oi/type_graph/NameGen.h | 1 + oi/type_graph/Printer.cpp | 8 +++ oi/type_graph/Printer.h | 1 + oi/type_graph/TopoSorter.cpp | 5 ++ oi/type_graph/TopoSorter.h | 1 + oi/type_graph/Types.h | 55 +++++++++++++++- oi/type_graph/Visitor.h | 3 + test/CMakeLists.txt | 1 + test/TypeGraphParser.cpp | 4 ++ test/integration/cycles.toml | 62 +++++++++++++++++ test/integration/type_cycles.toml | 35 ++++++++++ test/test_cycle_finder.cpp | 95 ++++++++++++++++++++++++++ test/test_name_gen.cpp | 18 +++++ 19 files changed, 516 insertions(+), 4 deletions(-) create mode 100644 oi/type_graph/CycleFinder.cpp create mode 100644 oi/type_graph/CycleFinder.h create mode 100644 test/integration/type_cycles.toml create mode 100644 test/test_cycle_finder.cpp diff --git a/.circleci/config.yml b/.circleci/config.yml index b5b779f..ef49a27 100644 --- a/.circleci/config.yml +++ b/.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: diff --git a/oi/CodeGen.cpp b/oi/CodeGen.cpp index f722bac..1f29010 100644 --- a/oi/CodeGen.cpp +++ b/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(&t)) { genDeclsEnum(*e, code); + } else if (const auto* b = dynamic_cast(&t)) { + genDeclsCycleBreaker(*b, code); } } } @@ -731,6 +741,22 @@ void getContainerTypeHandler(std::unordered_set& used, code += fmt.str(); } +void addCycleBreakerTypeHandler(const CycleBreaker& b, std::string& code) { + code += (boost::format(R"(template +struct TypeHandler { + public: + struct type { + type(DB buf) : _buf(buf) {} + DB _buf; + }; + static types::st::Unit getSizeType(const %1%& t, TypeHandler::type ret) { + return getSizeType(reinterpret_cast(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(&t)) { getContainerTypeHandler(definedContainers_, *con, code); + } else if (const auto* b = dynamic_cast(&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)); diff --git a/oi/type_graph/CMakeLists.txt b/oi/type_graph/CMakeLists.txt index 38a3ca9..20f1442 100644 --- a/oi/type_graph/CMakeLists.txt +++ b/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 diff --git a/oi/type_graph/CycleFinder.cpp b/oi/type_graph/CycleFinder.cpp new file mode 100644 index 0000000..d761645 --- /dev/null +++ b/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 + +#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(t); + replacements_.emplace(&t, e); + return e; + } +} + +void CycleFinder::breakCycle(std::span> cycle) { + if (cycle.size() >= 2) { + Type& from = cycle.back(); + Type& to = cycle.front(); + + if (auto* p = dynamic_cast(&from); + p && &p->pointeeType() == &to) { + p->setPointeeType(edge(to)); + return; + } + + if (auto* c = dynamic_cast(&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 diff --git a/oi/type_graph/CycleFinder.h b/oi/type_graph/CycleFinder.h new file mode 100644 index 0000000..cdeccd0 --- /dev/null +++ b/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 +#include +#include +#include + +#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>); + bool push(Type& t); + void pop(); + CycleBreaker& edge(Type& t); + + TypeGraph& typeGraph_; + + std::vector> currentPath_; + std::unordered_set currentPathSet_; + std::unordered_map> + replacements_; +}; + +} // namespace type_graph diff --git a/oi/type_graph/NameGen.cpp b/oi/type_graph/NameGen.cpp index 9123c82..4cbc00c 100644 --- a/oi/type_graph/NameGen.cpp +++ b/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 diff --git a/oi/type_graph/NameGen.h b/oi/type_graph/NameGen.h index 8e952de..c76f75c 100644 --- a/oi/type_graph/NameGen.h +++ b/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"; diff --git a/oi/type_graph/Printer.cpp b/oi/type_graph/Printer.cpp index 125b446..980966a 100644 --- a/oi/type_graph/Printer.cpp +++ b/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; diff --git a/oi/type_graph/Printer.h b/oi/type_graph/Printer.h index 36723f5..505d2d1 100644 --- a/oi/type_graph/Printer.h +++ b/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); diff --git a/oi/type_graph/TopoSorter.cpp b/oi/type_graph/TopoSorter.cpp index 4131a70..4a0a6d1 100644 --- a/oi/type_graph/TopoSorter.cpp +++ b/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. diff --git a/oi/type_graph/TopoSorter.h b/oi/type_graph/TopoSorter.h index d9f8105..9709f10 100644 --- a/oi/type_graph/TopoSorter.h +++ b/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 visited_; diff --git a/oi/type_graph/Types.h b/oi/type_graph/Types.h index 593fa82..f3a00a7 100644 --- a/oi/type_graph/Types.h +++ b/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 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 diff --git a/oi/type_graph/Visitor.h b/oi/type_graph/Visitor.h index 8fefa6e..b112166 100644 --- a/oi/type_graph/Visitor.h +++ b/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()); + } }; /* diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 20b90d4..e476353 100644 --- a/test/CMakeLists.txt +++ b/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 diff --git a/test/TypeGraphParser.cpp b/test/TypeGraphParser.cpp index c9a78b1..058c709 100644 --- a/test/TypeGraphParser.cpp +++ b/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}}; } diff --git a/test/integration/cycles.toml b/test/integration/cycles.toml index d98d9eb..8729245 100644 --- a/test/integration/cycles.toml +++ b/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&"] setup = ''' diff --git a/test/integration/type_cycles.toml b/test/integration/type_cycles.toml new file mode 100644 index 0000000..70b0701 --- /dev/null +++ b/test/integration/type_cycles.toml @@ -0,0 +1,35 @@ +includes = ["vector"] +definitions = ''' +struct Tree { + std::vector 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}] + } + ] + }] + ''' diff --git a/test/test_cycle_finder.cpp b/test/test_cycle_finder.cpp new file mode 100644 index 0000000..ae15179 --- /dev/null +++ b/test/test_cycle_finder.cpp @@ -0,0 +1,95 @@ +#include + +#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 next; + // }; + // + // After: + // class fake_UniqueNode; + // class UniqueNode { + // int value; + // std::unique_ptr 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] +)"); +} diff --git a/test/test_name_gen.cpp b/test/test_name_gen.cpp index 35af699..677f3e8 100644 --- a/test/test_name_gen.cpp +++ b/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"); + EXPECT_EQ(pointer.name(), "std::vector*"); + EXPECT_EQ(cycle.name(), "fake_std__vectorTMyClass_0TP_1"); +}