From 3871d92abb7ea089c7ba120d28fd4efe17f541f8 Mon Sep 17 00:00:00 2001 From: Jake Hillion Date: Mon, 13 Nov 2023 10:31:52 -0800 Subject: [PATCH] collapse TreeBuilderV2 features Summary: Currently there are two features between CodeGen v2 (TypeGraph) and TreeBuilder v2. These are TypedDataSegment and TreeBuilderTypeChecking. Each of these features currently has a full set of tests run in the CI and each have specific exclusions. Collapse these features into TreeBuilder v2. This allows for significantly simplified testing as any OIL tests run under TreeBuilder v2 and any OID tests run under TreeBuilder v1. The reasoning behind this is I no longer intend to partially roll out this feature. Full TreeBuilder v2 applies different conditions to containers than the intermediate states, and writing these only to have them never deployed is a waste of time. Test Plan: - it builds - CI --- .circleci/config.yml | 22 ------- oi/CodeGen.cpp | 64 ++++++-------------- oi/ContainerInfo.cpp | 10 +-- oi/ContainerInfo.h | 1 - oi/Features.cpp | 13 +--- oi/Features.h | 26 ++++---- oi/FuncGen.cpp | 114 ----------------------------------- oi/FuncGen.h | 5 -- oi/OICompiler.cpp | 6 +- oi/OIGenerator.cpp | 2 - oi/OILibraryImpl.cpp | 2 - test/integration/arrays.toml | 2 +- types/README.md | 10 +-- 13 files changed, 39 insertions(+), 238 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 286f759..7e805ad 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -21,20 +21,6 @@ workflows: oid_test_args: "-ftype-graph" tests_regex: "OidIntegration\\..*" exclude_regex: ".*inheritance_polymorphic.*|.*arrays_member_int0" - - test: - name: test-typed-data-segment-gcc - requires: - - build-gcc - oid_test_args: "-ftyped-data-segment" - tests_regex: "OidIntegration\\..*" - exclude_regex: ".*inheritance_polymorphic.*|.*pointers.*|.*arrays_member_int0|.*cycles_.*" - - test: - name: test-tree-builder-type-checking-gcc - requires: - - build-gcc - oid_test_args: "-ftree-builder-type-checking" - tests_regex: "OidIntegration\\..*" - exclude_regex: ".*inheritance_polymorphic.*|.*pointers.*|.*arrays_member_int0|.*cycles_.*" - coverage: name: coverage requires: @@ -43,14 +29,6 @@ workflows: name: coverage-type-graph requires: - test-type-graph-gcc - - coverage: - name: coverage-typed-data-segment - requires: - - test-typed-data-segment-gcc - - coverage: - name: coverage-tree-builder-type-checking - requires: - - test-tree-builder-type-checking-gcc - build: name: build-clang diff --git a/oi/CodeGen.cpp b/oi/CodeGen.cpp index 2fe0566..73725e4 100644 --- a/oi/CodeGen.cpp +++ b/oi/CodeGen.cpp @@ -119,17 +119,14 @@ void addIncludes(const TypeGraph& typeGraph, FeatureSet features, std::string& code) { std::set includes{"cstddef"}; - if (features[Feature::TypedDataSegment]) { + if (features[Feature::TreeBuilderV2]) { + code += "#define DEFINE_DESCRIBE 1\n"; // added before all includes + includes.emplace("functional"); + includes.emplace("oi/exporters/inst.h"); + includes.emplace("oi/types/dy.h"); includes.emplace("oi/types/st.h"); } - if (features[Feature::TreeBuilderTypeChecking]) { - includes.emplace("oi/types/dy.h"); - - code += "#define DEFINE_DESCRIBE 1\n"; // added before all includes - } - if (features[Feature::TreeBuilderV2]) - includes.emplace("oi/exporters/inst.h"); if (features[Feature::Library]) { includes.emplace("vector"); includes.emplace("oi/IntrospectionResult.h"); @@ -832,35 +829,20 @@ void CodeGen::genClassTypeHandler(const Class& c, std::string& code) { code += " using type = "; genClassStaticType(c, code); code += ";\n"; - if (config_.features[Feature::TreeBuilderV2]) - genClassTreeBuilderInstructions(c, code); + genClassTreeBuilderInstructions(c, code); genClassTraversalFunction(c, code); code += "};\n"; } namespace { -void genContainerTypeHandler(FeatureSet features, - std::unordered_set& used, +void genContainerTypeHandler(std::unordered_set& used, const ContainerInfo& c, std::span templateParams, std::string& code) { if (!used.insert(&c).second) return; - if (!features[Feature::TreeBuilderV2]) { - const auto& handler = c.codegen.handler; - if (handler.empty()) { - LOG(ERROR) << "`codegen.handler` must be specified for all containers " - "under \"-ftyped-data-segment\", not specified for \"" + - c.typeName + "\""; - throw std::runtime_error("missing `codegen.handler`"); - } - auto fmt = boost::format(c.codegen.handler) % c.typeName; - code += fmt.str(); - return; - } - code += c.codegen.extra; // TODO: Move this check into the ContainerInfo parsing once always enabled. @@ -1088,7 +1070,7 @@ constexpr inst::Field make_field(std::string_view name) { "0"}, }; genContainerTypeHandler( - features, used, FuncGen::GetOiArrayContainerInfo(), arrayParams, code); + used, FuncGen::GetOiArrayContainerInfo(), arrayParams, code); } } // namespace @@ -1098,14 +1080,10 @@ void CodeGen::addTypeHandlers(const TypeGraph& typeGraph, std::string& code) { if (const auto* c = dynamic_cast(&t)) { genClassTypeHandler(*c, code); } else if (const auto* con = dynamic_cast(&t)) { - genContainerTypeHandler(config_.features, - definedContainers_, - con->containerInfo_, - con->templateParams, - code); + genContainerTypeHandler( + definedContainers_, con->containerInfo_, con->templateParams, code); } else if (const auto* cap = dynamic_cast(&t)) { - genContainerTypeHandler(config_.features, - definedContainers_, + genContainerTypeHandler(definedContainers_, cap->containerInfo(), cap->container().templateParams, code); @@ -1227,14 +1205,14 @@ void CodeGen::generate( if (!config_.features[Feature::Library]) { FuncGen::DeclareExterns(code); } - if (!config_.features[Feature::TypedDataSegment]) { + if (!config_.features[Feature::TreeBuilderV2]) { defineMacros(code); } addIncludes(typeGraph, config_.features, code); defineInternalTypes(code); FuncGen::DefineJitLog(code, config_.features); - if (config_.features[Feature::TypedDataSegment]) { + if (config_.features[Feature::TreeBuilderV2]) { if (config_.features[Feature::Library]) { FuncGen::DefineBackInserterDataBuffer(code); } else { @@ -1242,10 +1220,8 @@ void CodeGen::generate( } code += "using namespace oi;\n"; code += "using namespace oi::detail;\n"; - if (config_.features[Feature::TreeBuilderV2]) { - code += "using oi::exporters::ParsedData;\n"; - code += "using namespace oi::exporters;\n"; - } + code += "using oi::exporters::ParsedData;\n"; + code += "using namespace oi::exporters;\n"; code += "namespace OIInternal {\nnamespace {\n"; FuncGen::DefineBasicTypeHandlers(code, config_.features); code += "} // namespace\n} // namespace OIInternal\n"; @@ -1265,7 +1241,7 @@ void CodeGen::generate( * process faster. */ code += "namespace OIInternal {\nnamespace {\n"; - if (!config_.features[Feature::TypedDataSegment]) { + if (!config_.features[Feature::TreeBuilderV2]) { FuncGen::DefineEncodeData(code); FuncGen::DefineEncodeDataSize(code); FuncGen::DefineStoreData(code); @@ -1280,7 +1256,7 @@ void CodeGen::generate( genExclusiveSizes(typeGraph, code); } - if (config_.features[Feature::TypedDataSegment]) { + if (config_.features[Feature::TreeBuilderV2]) { addStandardTypeHandlers(typeGraph, config_.features, code); addTypeHandlers(typeGraph, code); } else { @@ -1297,10 +1273,8 @@ void CodeGen::generate( code += "} // namespace\n} // namespace OIInternal\n"; const auto typeName = SymbolService::getTypeName(drgnType); - if (config_.features[Feature::Library]) { + if (config_.features[Feature::TreeBuilderV2]) { FuncGen::DefineTopLevelIntrospect(code, typeName); - } else if (config_.features[Feature::TypedDataSegment]) { - FuncGen::DefineTopLevelGetSizeRefTyped(code, typeName, config_.features); } else { FuncGen::DefineTopLevelGetSizeRef(code, typeName, config_.features); } @@ -1310,8 +1284,6 @@ void CodeGen::generate( typeName, calculateExclusiveSize(rootType), enumerateTypeNames(rootType)); - } else if (config_.features[Feature::TreeBuilderTypeChecking]) { - FuncGen::DefineOutputType(code, typeName); } if (!linkageName_.empty()) diff --git a/oi/ContainerInfo.cpp b/oi/ContainerInfo.cpp index 068da95..603c3c3 100644 --- a/oi/ContainerInfo.cpp +++ b/oi/ContainerInfo.cpp @@ -274,10 +274,6 @@ ContainerInfo::ContainerInfo(const fs::path& path) { } else { throw ContainerInfoError(path, "`codegen.decl` is a required field"); } - if (std::optional str = - codegenToml["handler"].value()) { - codegen.handler = std::move(*str); - } if (std::optional str = codegenToml["traversal_func"].value()) { codegen.traversalFunc = std::move(*str); @@ -323,8 +319,6 @@ ContainerInfo::ContainerInfo(std::string typeName_, matcher(getMatcher(typeName)), ctype(ctype_), header(std::move(header_)), - codegen(Codegen{"// DummyDecl %1%\n", - "// DummyFunc %1%\n", - "// DummyHandler %1%\n", - "// DummyFunc\n"}) { + codegen(Codegen{ + "// DummyDecl %1%\n", "// DummyFunc %1%\n", "// DummyFunc\n"}) { } diff --git a/oi/ContainerInfo.h b/oi/ContainerInfo.h index 0ad9da9..376274d 100644 --- a/oi/ContainerInfo.h +++ b/oi/ContainerInfo.h @@ -36,7 +36,6 @@ struct ContainerInfo { struct Codegen { std::string decl; std::string func; - std::string handler = ""; std::string traversalFunc = ""; std::string extra = ""; std::vector processors{}; diff --git a/oi/Features.cpp b/oi/Features.cpp index baf9344..9d82b17 100644 --- a/oi/Features.cpp +++ b/oi/Features.cpp @@ -40,11 +40,6 @@ std::optional featureHelp(Feature f) { return "Use Type Graph for code generation (CodeGen v2)."; case Feature::PruneTypeGraph: return "Prune unreachable nodes from the type graph"; - case Feature::TypedDataSegment: - return "Use Typed Data Segment in generated code."; - case Feature::TreeBuilderTypeChecking: - return "Use Typed Data Segment to perform runtime Type Checking in " - "TreeBuilder."; case Feature::Library: return std::nullopt; // Hide in OID help case Feature::TreeBuilderV2: @@ -65,14 +60,8 @@ std::optional featureHelp(Feature f) { std::span requirements(Feature f) { switch (f) { - case Feature::TypedDataSegment: - static constexpr std::array tds = {Feature::TypeGraph}; - return tds; - case Feature::TreeBuilderTypeChecking: - static constexpr std::array tc = {Feature::TypedDataSegment}; - return tc; case Feature::TreeBuilderV2: - static constexpr std::array tb2 = {Feature::TreeBuilderTypeChecking}; + static constexpr std::array tb2 = {Feature::TypeGraph}; return tb2; case Feature::Library: static constexpr std::array lib = {Feature::TreeBuilderV2}; diff --git a/oi/Features.h b/oi/Features.h index 2e92ba0..959a576 100644 --- a/oi/Features.h +++ b/oi/Features.h @@ -22,20 +22,18 @@ #include "oi/EnumBitset.h" -#define OI_FEATURE_LIST \ - X(ChaseRawPointers, "chase-raw-pointers") \ - X(PackStructs, "pack-structs") \ - X(GenPaddingStats, "gen-padding-stats") \ - X(CaptureThriftIsset, "capture-thrift-isset") \ - X(TypeGraph, "type-graph") \ - X(PruneTypeGraph, "prune-type-graph") \ - X(TypedDataSegment, "typed-data-segment") \ - X(TreeBuilderTypeChecking, "tree-builder-type-checking") \ - X(Library, "library") \ - X(TreeBuilderV2, "tree-builder-v2") \ - X(GenJitDebug, "gen-jit-debug") \ - X(JitLogging, "jit-logging") \ - X(JitTiming, "jit-timing") \ +#define OI_FEATURE_LIST \ + X(ChaseRawPointers, "chase-raw-pointers") \ + X(PackStructs, "pack-structs") \ + X(GenPaddingStats, "gen-padding-stats") \ + X(CaptureThriftIsset, "capture-thrift-isset") \ + X(TypeGraph, "type-graph") \ + X(PruneTypeGraph, "prune-type-graph") \ + X(Library, "library") \ + X(TreeBuilderV2, "tree-builder-v2") \ + X(GenJitDebug, "gen-jit-debug") \ + X(JitLogging, "jit-logging") \ + X(JitTiming, "jit-timing") \ X(PolymorphicInheritance, "polymorphic-inheritance") namespace oi::detail { diff --git a/oi/FuncGen.cpp b/oi/FuncGen.cpp index 9d6805b..7a1dd09 100644 --- a/oi/FuncGen.cpp +++ b/oi/FuncGen.cpp @@ -352,103 +352,6 @@ void FuncGen::DefineTopLevelGetSizeRef(std::string& testCode, testCode.append(fmt.str()); } -/* - * DefineTopLevelGetSizeRefTyped - * - * Top level function to run OI on a type utilising static types and enabled - * with feature '-ftyped-data-segment'. - */ -void FuncGen::DefineTopLevelGetSizeRefTyped(std::string& testCode, - const std::string& rawType, - FeatureSet features) { - std::string func = R"( - #pragma GCC diagnostic push - #pragma GCC diagnostic ignored "-Wunknown-attributes" - /* RawType: %1% */ - void __attribute__((used, retain)) getSize_%2$016x(const OIInternal::__ROOT_TYPE__& t) - #pragma GCC diagnostic pop - { - )"; - if (features[Feature::JitTiming]) { - func += " const auto startTime = std::chrono::steady_clock::now();\n"; - } - func += R"( - pointers.initialize(); - pointers.add((uintptr_t)&t); - auto data = reinterpret_cast(dataBase); - - // TODO: Replace these with types::st::Uint64 once the VarInt decoding - // logic is moved out of OIDebugger and into new TreeBuilder. - size_t dataSegOffset = 0; - data[dataSegOffset++] = oidMagicId; - data[dataSegOffset++] = cookieValue; - uintptr_t& writtenSize = data[dataSegOffset++]; - writtenSize = 0; - uintptr_t& timeTakenNs = data[dataSegOffset++]; - - dataSegOffset *= sizeof(uintptr_t); - JLOG("%1% @"); - JLOGPTR(&t); - - using ContentType = OIInternal::TypeHandler::type; - using SuffixType = types::st::Pair< - DataBuffer::DataSegment, - types::st::VarInt, - types::st::VarInt - >; - using DataBufferType = types::st::Pair< - DataBuffer::DataSegment, - ContentType, - SuffixType - >; - - DataBufferType db = DataBuffer::DataSegment(dataSegOffset); - SuffixType suffix = db.delegate([&t](auto ret) { - return OIInternal::getSizeType(t, ret); - }); - types::st::Unit end = suffix - .write(123456789) - .write(123456789); - - dataSegOffset = end.offset(); - writtenSize = dataSegOffset; - dataBase += dataSegOffset; - )"; - if (features[Feature::JitTiming]) { - func += R"( - timeTakenNs = std::chrono::duration_cast( - std::chrono::steady_clock::now() - startTime).count(); - )"; - } - func += R"( - } - )"; - - boost::format fmt = - boost::format(func) % rawType % std::hash{}(rawType); - testCode.append(fmt.str()); -} - -/* - * DefineOutputType - * - * Present the dynamic type of an object for OID/OIL/OITB to link against. - */ -void FuncGen::DefineOutputType(std::string& code, const std::string& rawType) { - std::string func = R"( - #pragma GCC diagnostic push - #pragma GCC diagnostic ignored "-Wunknown-attributes" - /* RawType: %1% */ - extern const types::dy::Dynamic __attribute__((used, retain)) outputType%2$016x = - OIInternal::TypeHandler::type::describe; - #pragma GCC diagnostic pop -)"; - - boost::format fmt = - boost::format(func) % rawType % std::hash{}(rawType); - code.append(fmt.str()); -} - void FuncGen::DefineTreeBuilderInstructions( std::string& code, const std::string& rawType, @@ -804,23 +707,6 @@ ContainerInfo FuncGen::GetOiArrayContainerInfo() { UNKNOWN_TYPE, "cstdint"}; // TODO: remove the need for a dummy header - oiArray.codegen.handler = R"( -template -struct TypeHandler> { - using type = types::st::List::type>; - static types::st::Unit getSizeType( - const %1% &container, - typename TypeHandler>::type returnArg) { - auto tail = returnArg.write(N); - for (size_t i=0; i::getSizeType(container.vals[i], ret); - }); - } - return tail.finish(); - } -}; -)"; oiArray.codegen.traversalFunc = R"( auto tail = returnArg.write(N0); for (size_t i=0; i>, 7>{{ - {Feature::TypedDataSegment, - {headers::oi_types_st_h, "oi/types/st.h"}}, - {Feature::TreeBuilderTypeChecking, - {headers::oi_types_dy_h, "oi/types/dy.h"}}, + {Feature::TreeBuilderV2, {headers::oi_types_st_h, "oi/types/st.h"}}, + {Feature::TreeBuilderV2, {headers::oi_types_dy_h, "oi/types/dy.h"}}, {Feature::TreeBuilderV2, {headers::oi_exporters_inst_h, "oi/exporters/inst.h"}}, {Feature::TreeBuilderV2, diff --git a/oi/OIGenerator.cpp b/oi/OIGenerator.cpp index 6c4590f..1cb2a49 100644 --- a/oi/OIGenerator.cpp +++ b/oi/OIGenerator.cpp @@ -184,8 +184,6 @@ int OIGenerator::generate(fs::path& primaryObject, SymbolService& symbols) { std::map featuresMap = { {Feature::TypeGraph, true}, - {Feature::TypedDataSegment, true}, - {Feature::TreeBuilderTypeChecking, true}, {Feature::TreeBuilderV2, true}, {Feature::Library, true}, {Feature::PackStructs, true}, diff --git a/oi/OILibraryImpl.cpp b/oi/OILibraryImpl.cpp index 0f33356..6442e9a 100644 --- a/oi/OILibraryImpl.cpp +++ b/oi/OILibraryImpl.cpp @@ -180,8 +180,6 @@ namespace { std::map convertFeatures(std::unordered_set fs) { std::map out{ {Feature::TypeGraph, true}, - {Feature::TypedDataSegment, true}, - {Feature::TreeBuilderTypeChecking, true}, {Feature::TreeBuilderV2, true}, {Feature::Library, true}, {Feature::PackStructs, true}, diff --git a/test/integration/arrays.toml b/test/integration/arrays.toml index 3efb2d2..4a06b13 100644 --- a/test/integration/arrays.toml +++ b/test/integration/arrays.toml @@ -62,7 +62,7 @@ definitions = ''' }]}]''' [cases.multidim_legacy] # Test for legacy behaviour. Remove with OICodeGen oil_disable = 'oil only runs on codegen v2' - cli_options = ["-Ftype-graph", "-Ftyped-data-segment", "-Ftree-builder-type-checking", "-Ftree-builder-v2"] + cli_options = ["-Ftype-graph", "-Ftree-builder-v2"] param_types = ["const MultiDim&"] setup = "return {};" expect_json = '''[{ diff --git a/types/README.md b/types/README.md index ebde86c..e2b70fd 100644 --- a/types/README.md +++ b/types/README.md @@ -62,14 +62,10 @@ This document describes the format of the container definition files contained i will collide if they share the same name. -## Changes introduced with Typed Data Segment -- `decl` and `func` fields are ignored when using `-ftyped-data-segment` and the - `handler` field is used instead. - ## Changes introduced with TreeBuilder V2 -- `decl`, `func`, and `handler` fields are ignored when using `-ftree-builder-v2`. - The `TypeHandler` is instead constructed from `traversal_func` and `processor` - entries. +- `decl` and `func` fields are ignored when using `-ftree-builder-v2`. The + `TypeHandler` is constructed from `traversal_func` field and `processor` + entries. ## Changes introduced with TypeGraph - `typeName` and `matcher` fields have been merged into the single field `type_name`.