From 641a128b391e0c1fcc0a314e5181b5e9b5cafa8a Mon Sep 17 00:00:00 2001 From: Jake Hillion Date: Mon, 17 Apr 2023 11:13:59 -0700 Subject: [PATCH] add command line feature addition/removal --- examples/compile-time-oil/Makefile | 2 +- src/CMakeLists.txt | 1 + src/Features.cpp | 48 ++++++++++ src/Features.h | 46 +++++++++ src/OICodeGen.cpp | 63 +++++++------ src/OICodeGen.h | 15 ++- src/OID.cpp | 93 +++++++++++++------ src/OIDebugger.cpp | 31 +++---- src/OIDebugger.h | 9 +- src/OIGenerator.cpp | 11 ++- src/OILibraryImpl.cpp | 18 ++-- src/OIUtils.cpp | 46 +++++++-- src/OIUtils.h | 10 +- src/TreeBuilder.cpp | 7 +- src/TreeBuilder.h | 8 +- test/integration/gen_tests.py | 17 ++-- test/integration/ignored.toml | 2 +- test/integration/inheritance_polymorphic.toml | 20 ++++ test/integration/pointers.toml | 44 +++++++++ test/integration/runner_common.cpp | 27 ++++-- test/integration/runner_common.h | 8 +- test/integration/thrift_isset.toml | 21 +++++ tools/OITB.cpp | 17 ++-- 23 files changed, 428 insertions(+), 136 deletions(-) create mode 100644 src/Features.cpp create mode 100644 src/Features.h diff --git a/examples/compile-time-oil/Makefile b/examples/compile-time-oil/Makefile index 16d1ebc..f7571d8 100644 --- a/examples/compile-time-oil/Makefile +++ b/examples/compile-time-oil/Makefile @@ -12,7 +12,7 @@ OilVectorOfStrings.o: OilVectorOfStrings.cpp oilgen: rm -f oilgen - (cd ../../ && make oid-devel) + (cd ../../ && make oid) ln -s ../../build/oilgen oilgen OilVectorOfStrings: oilgen OilVectorOfStrings.o diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index c7a1335..32b68ec 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -23,6 +23,7 @@ target_link_libraries(symbol_service add_library(codegen ContainerInfo.cpp + Features.cpp FuncGen.cpp OICodeGen.cpp ) diff --git a/src/Features.cpp b/src/Features.cpp new file mode 100644 index 0000000..644e7ce --- /dev/null +++ b/src/Features.cpp @@ -0,0 +1,48 @@ +/* + * 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 "Features.h" + +#include + +namespace ObjectIntrospection { + +Feature featureFromStr(std::string_view str) { + static const std::map nameMap = { +#define X(name, str) {str, Feature::name}, + OI_FEATURE_LIST +#undef X + }; + + if (auto search = nameMap.find(str); search != nameMap.end()) { + return search->second; + } + return Feature::UnknownFeature; +} + +const char* featureToStr(Feature f) { + switch (f) { +#define X(name, str) \ + case Feature::name: \ + return str; + OI_FEATURE_LIST +#undef X + + default: + return "UnknownFeature"; + } +} + +} // namespace ObjectIntrospection diff --git a/src/Features.h b/src/Features.h new file mode 100644 index 0000000..c1a6f16 --- /dev/null +++ b/src/Features.h @@ -0,0 +1,46 @@ +/* + * 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 + +#define OI_FEATURE_LIST \ + X(ChaseRawPointers, "chase-raw-pointers") \ + X(PackStructs, "pack-structs") \ + X(GenPaddingStats, "gen-padding-stats") \ + X(CaptureThriftIsset, "capture-thrift-isset") \ + X(PolymorphicInheritance, "polymorphic-inheritance") + +namespace ObjectIntrospection { + +enum class Feature { + UnknownFeature, +#define X(name, _) name, + OI_FEATURE_LIST +#undef X +}; + +Feature featureFromStr(std::string_view); +const char* featureToStr(Feature); + +constexpr std::array allFeatures = { +#define X(name, _) Feature::name, + OI_FEATURE_LIST +#undef X +}; + +} // namespace ObjectIntrospection diff --git a/src/OICodeGen.cpp b/src/OICodeGen.cpp index 5b68718..2fcd57b 100644 --- a/src/OICodeGen.cpp +++ b/src/OICodeGen.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -61,6 +62,13 @@ std::unique_ptr OICodeGen::buildFromConfig(const Config& c, OICodeGen::OICodeGen(const Config& c, SymbolService& s) : config{c}, symbols{s} { + chaseRawPointers = config.features.contains(Feature::ChaseRawPointers); + packStructs = config.features.contains(Feature::PackStructs); + genPaddingStats = config.features.contains(Feature::GenPaddingStats); + captureThriftIsset = config.features.contains(Feature::CaptureThriftIsset); + polymorphicInheritance = + config.features.contains(Feature::PolymorphicInheritance); + // TODO: Should folly::Range just be added as a container? auto typesToStub = std::array{ "SharedMutex", @@ -985,7 +993,7 @@ bool OICodeGen::recordChildren(drgn_type* type) { * types in the program to build the reverse mapping. */ bool OICodeGen::enumerateChildClasses() { - if (!config.polymorphicInheritance) { + if (!polymorphicInheritance) { return true; } @@ -1314,7 +1322,7 @@ bool OICodeGen::isEmptyClassOrFunctionType(drgn_type* type, * one or more virtual member functions or virtual base classes). */ bool OICodeGen::isDynamic(drgn_type* type) const { - if (!config.polymorphicInheritance || !drgn_type_has_virtuality(type)) { + if (!polymorphicInheritance || !drgn_type_has_virtuality(type)) { return false; } @@ -1992,7 +2000,7 @@ bool OICodeGen::getDrgnTypeNameInt(drgn_type* type, std::string& outName) { name.assign(drgn_type_name(type)); } else if (drgn_type_kind(type) == DRGN_TYPE_POINTER) { drgn_type* underlyingType = getPtrUnderlyingType(type); - if (config.chaseRawPointers && + if (chaseRawPointers && drgn_type_kind(underlyingType) != DRGN_TYPE_FUNCTION) { // For pointers, figure out name for the underlying type then add // appropriate number of '*' @@ -2262,7 +2270,7 @@ bool OICodeGen::generateStructDef(drgn_type* e, std::string& code) { std::string structDefinition; - if (paddingInfo.paddingSize != 0 && config.genPaddingStats) { + if (paddingInfo.paddingSize != 0 && genPaddingStats) { structDefinition.append("/* offset | size */ "); } @@ -2278,8 +2286,7 @@ bool OICodeGen::generateStructDef(drgn_type* e, std::string& code) { std::to_string(*alignment / CHAR_BIT) + ")"); } - if (config.packStructs && - (kind == DRGN_TYPE_STRUCT || kind == DRGN_TYPE_CLASS) && + if (packStructs && (kind == DRGN_TYPE_STRUCT || kind == DRGN_TYPE_CLASS) && violatesAlignmentRequirement && paddingInfo.paddingSize == 0) { structDefinition.append(" __attribute__((__packed__))"); } @@ -2298,7 +2305,7 @@ bool OICodeGen::generateStructDef(drgn_type* e, std::string& code) { structDefinition.append("};\n"); - if (config.genPaddingStats) { + if (genPaddingStats) { auto paddedStructFound = paddedStructs.find(*tmpStr); if (paddedStructFound == paddedStructs.end()) { @@ -2490,7 +2497,7 @@ std::optional OICodeGen::generateMember( currOffsetBits = 0; VLOG(1) << "Member size: " << memberSize; } else { - addSizeComment(config.genPaddingStats, code, currOffsetBits, memberSize); + addSizeComment(genPaddingStats, code, currOffsetBits, memberSize); currOffsetBits = currOffsetBits + memberSize; } @@ -2683,12 +2690,12 @@ bool OICodeGen::generateStructMembers( bool isThriftIssetStruct = typeName.starts_with("isset_bitset<") && memberName == "__isset"; - if (config.captureThriftIsset && isThriftIssetStruct && + if (captureThriftIsset && isThriftIssetStruct && memberIndex == members.size() - 1) { thriftIssetStructTypes.insert(e); } - if (config.genPaddingStats) { + if (genPaddingStats) { paddingInfo.isThriftStruct = isThriftIssetStruct; /* @@ -3232,12 +3239,12 @@ bool OICodeGen::generateJitCode(std::string& code) { functionsCode.append("namespace OIInternal {\nnamespace {\n"); functionsCode.append("// functions -----\n"); if (!funcGen.DeclareGetSizeFuncs(functionsCode, containerTypesFuncDef, - config.chaseRawPointers)) { + chaseRawPointers)) { LOG(ERROR) << "declaring get size for containers failed"; return false; } - if (config.chaseRawPointers) { + if (chaseRawPointers) { functionsCode.append(R"( template void getSizeType(const T* t, size_t& returnArg); @@ -3258,7 +3265,7 @@ bool OICodeGen::generateJitCode(std::string& code) { } } - if (config.useDataSegment || config.chaseRawPointers) { + if (config.useDataSegment || chaseRawPointers) { funcGen.DeclareStoreData(functionsCode); } @@ -3270,12 +3277,12 @@ bool OICodeGen::generateJitCode(std::string& code) { } if (!funcGen.DefineGetSizeFuncs(functionsCode, containerTypesFuncDef, - config.chaseRawPointers)) { + chaseRawPointers)) { LOG(ERROR) << "defining get size for containers failed"; return false; } - if (config.chaseRawPointers) { + if (chaseRawPointers) { functionsCode.append(R"( template void getSizeType(const T* s_ptr, size_t& returnArg) @@ -3744,22 +3751,22 @@ std::string OICodeGen::Config::toString() const { ignoreMembers += ';'; } - return (useDataSegment ? "" : "Dont") + "UseDataSegment,"s + - (chaseRawPointers ? "" : "Dont") + "ChaseRawPointers,"s + - (packStructs ? "" : "Dont") + "PackStructs,"s + - (genPaddingStats ? "" : "Dont") + "GenPaddingStats,"s + - (captureThriftIsset ? "" : "Dont") + "CaptureThriftIsset,"s + - (polymorphicInheritance ? "" : "Dont") + "PolymorphicInheritance,"s + - ignoreMembers; + return boost::algorithm::join(toOptions(), ",") + "," + ignoreMembers; } std::vector OICodeGen::Config::toOptions() const { - return {"", // useDataSegment is always true? - (chaseRawPointers ? "-n" : ""), - (packStructs ? "" : "-z"), - (genPaddingStats ? "" : "-w"), - (captureThriftIsset ? "-T" : ""), - (polymorphicInheritance ? "-P" : "")}; + std::vector options; + options.reserve(allFeatures.size()); + + for (const auto f : allFeatures) { + if (features.contains(f)) { + options.emplace_back(std::string("-f") + featureToStr(f)); + } else { + options.emplace_back(std::string("-F") + featureToStr(f)); + } + } + + return options; } void OICodeGen::initializeCodeGen() { diff --git a/src/OICodeGen.h b/src/OICodeGen.h index 1c16184..7efb703 100644 --- a/src/OICodeGen.h +++ b/src/OICodeGen.h @@ -28,6 +28,7 @@ struct irequest; #include "Common.h" #include "ContainerInfo.h" +#include "Features.h" #include "FuncGen.h" #include "PaddingHunter.h" @@ -35,6 +36,7 @@ extern "C" { #include } +using namespace ObjectIntrospection; namespace fs = std::filesystem; struct ParentMember { @@ -54,11 +56,8 @@ class OICodeGen { * uninitialized field" warning if they missed any. */ bool useDataSegment; - bool chaseRawPointers; - bool packStructs; - bool genPaddingStats; - bool captureThriftIsset; - bool polymorphicInheritance; + + std::set features{}; std::set containerConfigPaths{}; std::set defaultHeaders{}; @@ -114,6 +113,12 @@ class OICodeGen { Config config{}; FuncGen funcGen; + bool chaseRawPointers; + bool packStructs; + bool genPaddingStats; + bool captureThriftIsset; + bool polymorphicInheritance; + using ContainerTypeMapEntry = std::pair, std::vector>; diff --git a/src/OID.cpp b/src/OID.cpp index a367d37..e1598e7 100644 --- a/src/OID.cpp +++ b/src/OID.cpp @@ -22,15 +22,18 @@ #include #include #include +#include extern "C" { #include #include } +#include "Features.h" #include "Metrics.h" #include "OIDebugger.h" #include "OIOpts.h" +#include "OIUtils.h" #include "PaddingHunter.h" #include "TimeUtils.h" #include "TreeBuilder.h" @@ -156,6 +159,18 @@ constexpr static OIOpts opts{ "Follow runtime polymorphic inheritance hierarchies"}, OIOpt{'m', "mode", required_argument, "[prod]", "Allows to specify a mode of operation/group of settings"}, + OIOpt{'f', "enable-feature", required_argument, nullptr, + "Enable a specific feature: [" +#define X(name, str) str "," + OI_FEATURE_LIST +#undef X + "]"}, + OIOpt{'F', "disable-feature", required_argument, nullptr, + "Disable a specific feature: [" +#define X(name, str) str "," + OI_FEATURE_LIST +#undef X + "]"}, }; void usage() { @@ -266,11 +281,11 @@ struct Config { } // namespace Oid -static ExitStatus::ExitStatus runScript(const std::string& fileName, - std::istream& script, - const Oid::Config& oidConfig, - const OICodeGen::Config& codeGenConfig, - const TreeBuilder::Config& tbConfig) { +static ExitStatus::ExitStatus runScript( + const std::string& fileName, std::istream& script, + const Oid::Config& oidConfig, const OICodeGen::Config& codeGenConfig, + const OICompiler::Config& compilerConfig, + const TreeBuilder::Config& tbConfig) { if (!fileName.empty()) { VLOG(1) << "SCR FILE: " << fileName; } @@ -279,11 +294,11 @@ static ExitStatus::ExitStatus runScript(const std::string& fileName, std::shared_ptr oid; // share oid with the global signal handler if (oidConfig.pid != 0) { - oid = std::make_shared(oidConfig.pid, oidConfig.configFile, - codeGenConfig, tbConfig); + oid = std::make_shared(oidConfig.pid, codeGenConfig, + compilerConfig, tbConfig); } else { - oid = std::make_shared( - oidConfig.debugInfoFile, oidConfig.configFile, codeGenConfig, tbConfig); + oid = std::make_shared(oidConfig.debugInfoFile, codeGenConfig, + compilerConfig, tbConfig); } weak_oid = oid; // set the weak_ptr for signal handlers @@ -463,12 +478,13 @@ int main(int argc, char* argv[]) { std::string configGenOption; std::optional jsonPath{std::nullopt}; + std::map features = { + {Feature::PackStructs, true}, + {Feature::GenPaddingStats, true}, + }; + bool logAllStructs = true; - bool chaseRawPointers = false; - bool packStructs = true; bool dumpDataSegment = false; - bool captureThriftIsset = false; - bool polymorphicInheritance = false; Metrics::Tracing _("main"); #ifndef OSS_ENABLE @@ -485,13 +501,25 @@ int main(int argc, char* argv[]) { while ((c = getopt_long(argc, argv, opts.shortOpts(), opts.longOpts(), nullptr)) != -1) { switch (c) { + case 'F': + [[fallthrough]]; + case 'f': + if (auto f = featureFromStr(optarg); f != Feature::UnknownFeature) { + features[f] = c == 'f'; // '-f' enables, '-F' disables + } else { + LOG(ERROR) << "Invalid feature: " << optarg << " specified!"; + usage(); + return ExitStatus::UsageError; + } + break; + case 'm': { if (strcmp("prod", optarg) == 0) { // change default settings for prod oidConfig.hardDisableDrgn = true; oidConfig.cacheRemoteDownload = true; oidConfig.cacheBasePath = "/tmp/oid-cache"; - chaseRawPointers = true; + features[Feature::ChaseRawPointers] = true; } else { LOG(ERROR) << "Invalid mode: " << optarg << " specified!"; usage(); @@ -616,13 +644,13 @@ int main(int argc, char* argv[]) { oidConfig.removeMappings = true; break; case 'n': - chaseRawPointers = true; + features[Feature::ChaseRawPointers] = true; break; case 'a': logAllStructs = true; break; case 'z': - packStructs = false; + features[Feature::PackStructs] = false; break; case 'B': dumpDataSegment = true; @@ -637,16 +665,16 @@ int main(int argc, char* argv[]) { oidConfig.timeout_s = atoi(optarg); break; case 'w': - oidConfig.genPaddingStats = false; + features[Feature::GenPaddingStats] = false; break; case 'J': jsonPath = optarg != nullptr ? optarg : "oid_out.json"; break; case 'T': - captureThriftIsset = true; + features[Feature::CaptureThriftIsset] = true; break; case 'P': - polymorphicInheritance = true; + features[Feature::PolymorphicInheritance] = true; break; case 'h': default: @@ -694,38 +722,43 @@ int main(int argc, char* argv[]) { scriptSource = "entry:unknown_function:arg0"; } + OICompiler::Config compilerConfig{}; + OICodeGen::Config codeGenConfig{ .useDataSegment = true, - .chaseRawPointers = chaseRawPointers, - .packStructs = packStructs, - .genPaddingStats = oidConfig.genPaddingStats, - .captureThriftIsset = captureThriftIsset, - .polymorphicInheritance = polymorphicInheritance, + .features = {}, // fill in after processing the config file }; TreeBuilder::Config tbConfig{ + .features = {}, // fill in after processing the config file .logAllStructs = logAllStructs, - .chaseRawPointers = chaseRawPointers, - .genPaddingStats = oidConfig.genPaddingStats, .dumpDataSegment = dumpDataSegment, .jsonPath = jsonPath, }; + auto featureSet = OIUtils::processConfigFile(oidConfig.configFile, features, + compilerConfig, codeGenConfig); + if (!featureSet) { + return ExitStatus::UsageError; + } + codeGenConfig.features = *featureSet; + tbConfig.features = *featureSet; + if (!scriptFile.empty()) { if (!std::filesystem::exists(scriptFile)) { LOG(ERROR) << "Non-existent script file: " << scriptFile; return ExitStatus::FileNotFoundError; } std::ifstream script(scriptFile); - auto status = - runScript(scriptFile, script, oidConfig, codeGenConfig, tbConfig); + auto status = runScript(scriptFile, script, oidConfig, codeGenConfig, + compilerConfig, tbConfig); if (status != ExitStatus::Success) { return status; } } else if (!scriptSource.empty()) { std::istringstream script(scriptSource); - auto status = - runScript(scriptFile, script, oidConfig, codeGenConfig, tbConfig); + auto status = runScript(scriptFile, script, oidConfig, codeGenConfig, + compilerConfig, tbConfig); if (status != ExitStatus::Success) { return status; } diff --git a/src/OIDebugger.cpp b/src/OIDebugger.cpp index e9593e8..4a8f840 100644 --- a/src/OIDebugger.cpp +++ b/src/OIDebugger.cpp @@ -272,7 +272,7 @@ bool OIDebugger::segmentInit(void) { VLOG(1) << "segConfig size " << sizeof(segConfig); if (segmentConfigFile.fail()) { - LOG(ERROR) << "init: error in writing configFile" << configFilePath + LOG(ERROR) << "init: error in writing configFile" << segConfigFilePath << strerror(errno); } VLOG(1) << "About to flush segment config file"; @@ -1868,28 +1868,21 @@ bool OIDebugger::removeTrap(pid_t pid, const trapInfo& t) { return true; } -OIDebugger::OIDebugger(std::string configFile, OICodeGen::Config genConfig, +OIDebugger::OIDebugger(OICodeGen::Config genConfig, OICompiler::Config ccConfig, TreeBuilder::Config tbConfig) - : configFilePath{configFile}, + : compilerConfig{std::move(ccConfig)}, generatorConfig{std::move(genConfig)}, treeBuilderConfig{std::move(tbConfig)} { - if (configFilePath.empty()) { - configFilePath = fs::current_path() / "oid.toml"; - } - - VLOG(1) << "config file: " << configFilePath; - debug = true; - OIUtils::processConfigFile(configFilePath, compilerConfig, generatorConfig); cache.generatorConfig = generatorConfig; VLOG(1) << "CodeGen config: " << generatorConfig.toString(); } -OIDebugger::OIDebugger(pid_t pid, std::string configFile, - OICodeGen::Config genConfig, +OIDebugger::OIDebugger(pid_t pid, OICodeGen::Config genConfig, + OICompiler::Config ccConfig, TreeBuilder::Config tbConfig) - : OIDebugger(std::move(configFile), std::move(genConfig), + : OIDebugger(std::move(genConfig), std::move(ccConfig), std::move(tbConfig)) { traceePid = pid; symbols = std::make_shared(traceePid); @@ -1898,10 +1891,10 @@ OIDebugger::OIDebugger(pid_t pid, std::string configFile, cache.symbols = symbols; } -OIDebugger::OIDebugger(fs::path debugInfo, std::string configFile, - OICodeGen::Config genConfig, +OIDebugger::OIDebugger(fs::path debugInfo, OICodeGen::Config genConfig, + OICompiler::Config ccConfig, TreeBuilder::Config tbConfig) - : OIDebugger(std::move(configFile), std::move(genConfig), + : OIDebugger(std::move(genConfig), std::move(ccConfig), std::move(tbConfig)) { symbols = std::make_shared(std::move(debugInfo)); cache.symbols = symbols; @@ -2848,7 +2841,7 @@ bool OIDebugger::processTargetData() { const auto& [rootType, typeHierarchy, paddingInfos] = typeInfo->second; VLOG(1) << "Root type addr: " << (void*)rootType.type.type; - if (treeBuilderConfig.genPaddingStats) { + if (treeBuilderConfig.features.contains(Feature::GenPaddingStats)) { paddingHunter.localPaddedStructs = paddingInfos; typeTree.setPaddedStructs(&paddingHunter.localPaddedStructs); } @@ -2872,7 +2865,7 @@ bool OIDebugger::processTargetData() { continue; } - if (treeBuilderConfig.genPaddingStats) { + if (treeBuilderConfig.features.contains(Feature::GenPaddingStats)) { paddingHunter.processLocalPaddingInfo(); } } @@ -2886,7 +2879,7 @@ bool OIDebugger::processTargetData() { typeTree.dumpJson(); } - if (treeBuilderConfig.genPaddingStats) { + if (treeBuilderConfig.features.contains(Feature::GenPaddingStats)) { paddingHunter.outputPaddingInfo(); } diff --git a/src/OIDebugger.h b/src/OIDebugger.h index 6502ab8..13bace8 100644 --- a/src/OIDebugger.h +++ b/src/OIDebugger.h @@ -32,11 +32,13 @@ namespace fs = std::filesystem; class OIDebugger { - OIDebugger(std::string, OICodeGen::Config, TreeBuilder::Config); + OIDebugger(OICodeGen::Config, OICompiler::Config, TreeBuilder::Config); public: - OIDebugger(pid_t, std::string, OICodeGen::Config, TreeBuilder::Config); - OIDebugger(fs::path, std::string, OICodeGen::Config, TreeBuilder::Config); + OIDebugger(pid_t, OICodeGen::Config, OICompiler::Config, TreeBuilder::Config); + OIDebugger(fs::path, OICodeGen::Config, OICompiler::Config, + TreeBuilder::Config); + bool segmentInit(void); bool stopTarget(void); bool interruptTarget(void); @@ -142,7 +144,6 @@ class OIDebugger { } private: - std::string configFilePath; bool debug = false; bool enableJitLogging = false; pid_t traceePid{}; diff --git a/src/OIGenerator.cpp b/src/OIGenerator.cpp index f4090a6..7df659a 100644 --- a/src/OIGenerator.cpp +++ b/src/OIGenerator.cpp @@ -184,13 +184,20 @@ int OIGenerator::generate(fs::path& primaryObject, SymbolService& symbols) { std::vector> oilTypes = findOilTypesAndNames(prog); + std::map featuresMap = { + {Feature::PackStructs, true}, + }; + OICodeGen::Config generatorConfig{}; OICompiler::Config compilerConfig{}; - if (!OIUtils::processConfigFile(configFilePath, compilerConfig, - generatorConfig)) { + + auto features = OIUtils::processConfigFile(configFilePath, featuresMap, + compilerConfig, generatorConfig); + if (!features) { LOG(ERROR) << "failed to process config file"; return -1; } + generatorConfig.features = *features; generatorConfig.useDataSegment = false; size_t failures = 0; diff --git a/src/OILibraryImpl.cpp b/src/OILibraryImpl.cpp index c4f1675..60bb98b 100644 --- a/src/OILibraryImpl.cpp +++ b/src/OILibraryImpl.cpp @@ -80,16 +80,22 @@ void OILibraryImpl::initCompiler() { symbols = std::make_shared(getpid()); compilerConfig.generateJitDebugInfo = _self->opts.generateJitDebugInfo; - generatorConfig.useDataSegment = false; - generatorConfig.chaseRawPointers = _self->opts.chaseRawPointers; - generatorConfig.packStructs = true; - generatorConfig.genPaddingStats = false; } bool OILibraryImpl::processConfigFile() { - return OIUtils::processConfigFile(_self->opts.configFilePath, compilerConfig, - generatorConfig); + auto features = OIUtils::processConfigFile( + _self->opts.configFilePath, + { + {Feature::ChaseRawPointers, _self->opts.chaseRawPointers}, + {Feature::PackStructs, true}, + }, + compilerConfig, generatorConfig); + if (!features) { + return false; + } + generatorConfig.features = *features; + return true; } template diff --git a/src/OIUtils.cpp b/src/OIUtils.cpp index 4570aba..73df636 100644 --- a/src/OIUtils.cpp +++ b/src/OIUtils.cpp @@ -13,6 +13,8 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "OIUtils.h" + #include #include @@ -22,18 +24,16 @@ #include #include -#include "OICodeGen.h" -#include "OICompiler.h" - namespace fs = std::filesystem; namespace OIUtils { +using namespace ObjectIntrospection; using namespace std::literals; -bool processConfigFile(const std::string& configFilePath, - OICompiler::Config& compilerConfig, - OICodeGen::Config& generatorConfig) { +std::optional> processConfigFile( + const std::string& configFilePath, std::map featureMap, + OICompiler::Config& compilerConfig, OICodeGen::Config& generatorConfig) { fs::path configDirectory = fs::path(configFilePath).remove_filename(); toml::table config; @@ -42,7 +42,29 @@ bool processConfigFile(const std::string& configFilePath, } catch (const toml::parse_error& ex) { LOG(ERROR) << "processConfigFileToml: " << configFilePath << " : " << ex.description(); - return false; + return {}; + } + + if (toml::array* features = config["features"].as_array()) { + for (auto&& el : *features) { + auto* featureStr = el.as_string(); + if (!featureStr) { + LOG(ERROR) << "enabled features must be strings"; + return {}; + } + + if (auto f = featureFromStr(featureStr->get()); + f != Feature::UnknownFeature) { + // Inserts element(s) into the container, if the container doesn't + // already contain an element with an equivalent key. Hence prefer + // command line enabling/disabling. + featureMap.insert({f, true}); + } else { + LOG(ERROR) << "unrecognised feature: " << featureStr->get() + << " specified in config"; + return {}; + } + } } if (toml::table* types = config["types"].as_table()) { @@ -111,7 +133,7 @@ bool processConfigFile(const std::string& configFilePath, auto* type = (*ignore)["type"].as_string(); if (!type) { LOG(ERROR) << "Config entry 'ignore' must specify a type"; - return false; + return {}; } auto* members = (*ignore)["members"].as_array(); @@ -129,7 +151,13 @@ bool processConfigFile(const std::string& configFilePath, } } - return true; + std::set featuresSet; + for (auto [k, v] : featureMap) { + if (v) { + featuresSet.insert(k); + } + } + return featuresSet; } } // namespace OIUtils diff --git a/src/OIUtils.h b/src/OIUtils.h index 1f7d1ac..5759ff7 100644 --- a/src/OIUtils.h +++ b/src/OIUtils.h @@ -15,13 +15,17 @@ */ #pragma once +#include +#include + +#include "Features.h" #include "OICodeGen.h" #include "OICompiler.h" namespace OIUtils { -bool processConfigFile(const std::string& configFilePath, - OICompiler::Config& compilerConfig, - OICodeGen::Config& generatorConfig); +std::optional> processConfigFile( + const std::string& configFilePath, std::map featureMap, + OICompiler::Config& compilerConfig, OICodeGen::Config& generatorConfig); } // namespace OIUtils diff --git a/src/TreeBuilder.cpp b/src/TreeBuilder.cpp index ec7735a..44f2120 100644 --- a/src/TreeBuilder.cpp +++ b/src/TreeBuilder.cpp @@ -53,6 +53,9 @@ enum class TrackPointerTag : uint64_t { TreeBuilder::TreeBuilder(Config c) : config{std::move(c)} { buffer = std::make_unique(); + chaseRawPointers = config.features.contains(Feature::ChaseRawPointers); + genPaddingStats = config.features.contains(Feature::GenPaddingStats); + auto testdbPath = "/tmp/testdb_" + std::to_string(getpid()); if (auto status = rocksdb::DestroyDB(testdbPath, {}); !status.ok()) { LOG(FATAL) << "RocksDB error while destroying database: " @@ -414,7 +417,7 @@ TreeBuilder::Node TreeBuilder::process(NodeID id, Variable variable) { if (!variable.isStubbed) { switch (drgn_type_kind(variable.type)) { case DRGN_TYPE_POINTER: - if (config.chaseRawPointers) { + if (chaseRawPointers) { // Pointers to incomplete types are stubbed out // See OICodeGen::enumeratePointerType if (th->knownDummyTypeList.contains(variable.type)) { @@ -529,7 +532,7 @@ TreeBuilder::Node TreeBuilder::process(NodeID id, Variable variable) { break; } - if (config.genPaddingStats) { + if (genPaddingStats) { auto entry = paddedStructs->find(node.typeName); if (entry != paddedStructs->end()) { entry->second.instancesCnt++; diff --git a/src/TreeBuilder.h b/src/TreeBuilder.h index ec8c63b..a27fd82 100644 --- a/src/TreeBuilder.h +++ b/src/TreeBuilder.h @@ -18,11 +18,13 @@ #include #include #include +#include #include #include #include #include "Common.h" +#include "Features.h" // The rocksdb includes are extremely heavy and bloat compile times, // so we just forward-declare `DB` to avoid making other compile units @@ -39,9 +41,8 @@ class TreeBuilder { struct Config { // Don't set default values for the config so the user gets // an "unitialized field" warning if he missed any. + std::set features; bool logAllStructs; - bool chaseRawPointers; - bool genPaddingStats; bool dumpDataSegment; std::optional jsonPath; }; @@ -66,6 +67,9 @@ class TreeBuilder { const std::vector* oidData = nullptr; std::map* paddedStructs = nullptr; + bool genPaddingStats; + bool chaseRawPointers; + /* * The RocksDB output needs versioning so they are imported correctly in * Scuba. Version 1 had no concept of versioning and no header. diff --git a/test/integration/gen_tests.py b/test/integration/gen_tests.py index 8f22e16..435d190 100644 --- a/test/integration/gen_tests.py +++ b/test/integration/gen_tests.py @@ -270,13 +270,16 @@ def add_oid_integration_test(f, config, case_name, case): cli_options = ( "{" + ", ".join(f'"{option}"' for option in case.get("cli_options", ())) + "}" ) - config_extra = case.get("config", "") + + config_prefix = case.get("config_prefix", "") + config_suffix = case.get("config_suffix", "") f.write( f"\n" f"TEST_F(OidIntegration, {case_str}) {{\n" f"{generate_skip(case, 'oid')}" - f' std::string configOptions = R"--({config_extra})--";\n' + f' std::string configPrefix = R"--({config_prefix})--";\n' + f' std::string configSuffix = R"--({config_suffix})--";\n' f" ba::io_context ctx;\n" f" auto [target, oid] = runOidOnProcess(\n" f" {{\n" @@ -285,7 +288,7 @@ def add_oid_integration_test(f, config, case_name, case): f' .scriptSource = "{probe_str}",\n' f" }},\n" f" {cli_options},\n" - f" std::move(configOptions));\n" + f" std::move(configPrefix), std::move(configSuffix));\n" f" ASSERT_EQ(exit_code(oid), {exit_code});\n" f" EXPECT_EQ(target.proc.running(), true);\n" ) @@ -341,18 +344,20 @@ def add_oil_integration_test(f, config, case_name, case): if case.get("oil_disable", False): return - config_extra = case.get("config", "") + config_prefix = case.get("config_prefix", "") + config_suffix = case.get("config_suffix", "") f.write( f"\n" f"TEST_F(OilIntegration, {case_str}) {{\n" f"{generate_skip(case, 'oil')}" - f' std::string configOptions = R"--({config_extra})--";\n' + f' std::string configPrefix = R"--({config_prefix})--";\n' + f' std::string configSuffix = R"--({config_suffix})--";\n' f" ba::io_context ctx;\n" f" auto target = runOilTarget({{\n" f" .ctx = ctx,\n" f' .targetArgs = "oil {case_str}",\n' - f" }}, std::move(configOptions));\n\n" + f" }}, std::move(configPrefix), std::move(configSuffix));\n\n" f" ASSERT_EQ(exit_code(target), {exit_code});\n" f"\n" f" bpt::ptree result_json;\n" diff --git a/test/integration/ignored.toml b/test/integration/ignored.toml index 83821f5..7276374 100644 --- a/test/integration/ignored.toml +++ b/test/integration/ignored.toml @@ -18,7 +18,7 @@ definitions = ''' "The 3rd member of the struct Bar" }; """ - config = """ + config_suffix = """ [[codegen.ignore]] type = "Foo" members = ["a"] diff --git a/test/integration/inheritance_polymorphic.toml b/test/integration/inheritance_polymorphic.toml index 1050548..1a975c7 100644 --- a/test/integration/inheritance_polymorphic.toml +++ b/test/integration/inheritance_polymorphic.toml @@ -136,3 +136,23 @@ definitions = ''' {"name":"vec_b", "staticSize":24, "dynamicSize":12, "length":3, "capacity":3, "elementStaticSize":4}, {"name":"int_c", "staticSize":4, "dynamicSize":0} ]}]''' + [cases.feature_flag] + oil_skip = "Polymorphic inheritance disabled in OIL" + cli_options = ["-fpolymorphic-inheritance"] + param_types = ["const B&"] + arg_types = ["C"] + setup = ''' + C c; + c.vec_b = {1,2,3}; + return c; + ''' + expect_json = '''[{ + "typeName":"C", + "staticSize":48, + "dynamicSize":12, + "members":[ + {"staticSize":8, "dynamicSize":0}, + {"name":"int_a", "staticSize":4, "dynamicSize":0}, + {"name":"vec_b", "staticSize":24, "dynamicSize":12, "length":3, "capacity":3, "elementStaticSize":4}, + {"name":"int_c", "staticSize":4, "dynamicSize":0} + ]}]''' diff --git a/test/integration/pointers.toml b/test/integration/pointers.toml index d58d96b..e01fd08 100644 --- a/test/integration/pointers.toml +++ b/test/integration/pointers.toml @@ -238,3 +238,47 @@ definitions = ''' {"staticSize":8, "dynamicSize":0, "pointer":0}, {"staticSize":8, "dynamicSize":0, "NOT": {"pointer":0}} ]}]''' + [cases.feature_flag] + oil_disable = "oil can't chase raw pointers safely" + param_types = ["const std::vector&"] + setup = "return {{new int(1), nullptr, new int(3)}};" + cli_options = ["-fchase-raw-pointers"] + expect_json = '''[{ + "staticSize":24, + "dynamicSize":32, + "length":3, + "capacity":3, + "elementStaticSize":8, + "members":[ + {"staticSize":8, "dynamicSize":4, "NOT": {"pointer":0}}, + {"staticSize":8, "dynamicSize":0, "pointer":0}, + {"staticSize":8, "dynamicSize":4, "NOT": {"pointer":0}} + ]}]''' + [cases.feature_flag_disabled] + param_types = ["const PrimitivePtrs&"] + setup = "return PrimitivePtrs{0, new int(0), new int(0)};" + cli_options = ["-fchase-raw-pointers", "-Fchase-raw-pointers"] + expect_json = '''[{ + "staticSize":24, + "dynamicSize":0, + "members":[ + {"name":"a", "staticSize":4, "dynamicSize":0}, + {"name":"b", "staticSize":8, "dynamicSize":0}, + {"name":"c", "staticSize":8, "dynamicSize":0} + ]}]''' + [cases.feature_config] + oil_disable = "oil can't chase raw pointers safely" + param_types = ["const std::vector&"] + setup = "return {{new int(1), nullptr, new int(3)}};" + config_prefix = 'features = ["chase-raw-pointers"]' + expect_json = '''[{ + "staticSize":24, + "dynamicSize":32, + "length":3, + "capacity":3, + "elementStaticSize":8, + "members":[ + {"staticSize":8, "dynamicSize":4, "NOT": {"pointer":0}}, + {"staticSize":8, "dynamicSize":0, "pointer":0}, + {"staticSize":8, "dynamicSize":4, "NOT": {"pointer":0}} + ]}]''' diff --git a/test/integration/runner_common.cpp b/test/integration/runner_common.cpp index a44b518..bd63cd3 100644 --- a/test/integration/runner_common.cpp +++ b/test/integration/runner_common.cpp @@ -122,9 +122,10 @@ int IntegrationBase::exit_code(Proc& proc) { return proc.proc.exit_code(); } -fs::path IntegrationBase::createCustomConfig(const std::string& extraConfig) { +fs::path IntegrationBase::createCustomConfig(const std::string& prefix, + const std::string& suffix) { // If no extra config provided, return the config path unaltered. - if (extraConfig.empty()) { + if (prefix.empty() && suffix.empty()) { return configFile; } @@ -157,9 +158,17 @@ fs::path IntegrationBase::createCustomConfig(const std::string& extraConfig) { } std::ofstream customConfig(customConfigFile, std::ios_base::app); + if (!prefix.empty()) { + customConfig << "\n\n# Test custom config start\n\n"; + customConfig << prefix; + customConfig << "\n\n# Test custom config end\n\n"; + } customConfig << config; - customConfig << "\n\n# Test custom config\n\n"; - customConfig << extraConfig; + if (!suffix.empty()) { + customConfig << "\n\n# Test custom config start\n\n"; + customConfig << suffix; + customConfig << "\n\n# Test custom config end\n\n"; + } return customConfigFile; } @@ -170,7 +179,8 @@ std::string OidIntegration::TmpDirStr() { OidProc OidIntegration::runOidOnProcess(OidOpts opts, std::vector extra_args, - std::string extra_config) { + std::string configPrefix, + std::string configSuffix) { // Binary paths are populated by CMake std::string targetExe = std::string(TARGET_EXE_PATH) + " " + opts.targetArgs + " 1000"; @@ -195,7 +205,7 @@ OidProc OidIntegration::runOidOnProcess(OidOpts opts, std::ofstream touch(segconfigPath); } - fs::path thisConfig = createCustomConfig(extra_config); + fs::path thisConfig = createCustomConfig(configPrefix, configSuffix); // Keep PID as the last argument to make it easier for users to directly copy // and modify the command from the verbose mode output. @@ -337,8 +347,9 @@ std::string OilIntegration::TmpDirStr() { return std::string("/tmp/oil-integration-XXXXXX"); } -Proc OilIntegration::runOilTarget(OidOpts opts, std::string extra_config) { - fs::path thisConfig = createCustomConfig(extra_config); +Proc OilIntegration::runOilTarget(OidOpts opts, std::string configPrefix, + std::string configSuffix) { + fs::path thisConfig = createCustomConfig(configPrefix, configSuffix); std::string targetExe = std::string(TARGET_EXE_PATH) + " " + opts.targetArgs + " " + thisConfig.string(); diff --git a/test/integration/runner_common.h b/test/integration/runner_common.h index 4c88f8f..cd38a99 100644 --- a/test/integration/runner_common.h +++ b/test/integration/runner_common.h @@ -38,7 +38,8 @@ class IntegrationBase : public ::testing::Test { void TearDown() override; void SetUp() override; int exit_code(Proc& proc); - std::filesystem::path createCustomConfig(const std::string& extra); + std::filesystem::path createCustomConfig(const std::string& prefix, + const std::string& suffix); std::filesystem::path workingDir; @@ -51,7 +52,7 @@ class OidIntegration : public IntegrationBase { std::string TmpDirStr() override; OidProc runOidOnProcess(OidOpts opts, std::vector extra_args, - std::string extra_config); + std::string configPrefix, std::string configSuffix); /* * compare_json @@ -69,5 +70,6 @@ class OilIntegration : public IntegrationBase { protected: std::string TmpDirStr() override; - Proc runOilTarget(OidOpts opts, std::string extra_config); + Proc runOilTarget(OidOpts opts, std::string configPrefix, + std::string configSuffix); }; diff --git a/test/integration/thrift_isset.toml b/test/integration/thrift_isset.toml index ee9d21a..3a914c0 100644 --- a/test/integration/thrift_isset.toml +++ b/test/integration/thrift_isset.toml @@ -225,3 +225,24 @@ namespace cpp2 { {"name":"__fbthrift_field_e", "staticSize":4, "NOT":"isset"}, {"name":"__isset", "staticSize":3} ]}]''' + + [cases.feature_flag] + param_types = ["const cpp2::MyThriftStructBoxed&"] + setup = ''' + cpp2::MyThriftStructBoxed ret; + ret.d_ref() = 1; + ret.e_ref() = 1; + return ret; + ''' + cli_options = ["-fcapture-thrift-isset"] + expect_json = '''[{ + "staticSize":32, + "dynamicSize":0, + "members":[ + {"name":"__fbthrift_field_a", "staticSize":8, "NOT":"isset"}, + {"name":"__fbthrift_field_b", "staticSize":8, "NOT":"isset"}, + {"name":"__fbthrift_field_c", "staticSize":4, "isset":false}, + {"name":"__fbthrift_field_d", "staticSize":4, "isset":true}, + {"name":"__fbthrift_field_e", "staticSize":4, "isset":true}, + {"name":"__isset", "staticSize":3} + ]}]''' diff --git a/tools/OITB.cpp b/tools/OITB.cpp index 3212cf3..b66432c 100644 --- a/tools/OITB.cpp +++ b/tools/OITB.cpp @@ -30,6 +30,8 @@ namespace fs = std::filesystem; +using namespace ObjectIntrospection; + constexpr static OIOpts opts{ OIOpt{'h', "help", no_argument, nullptr, "Print this message and exit"}, OIOpt{'a', "log-all-structs", no_argument, nullptr, @@ -118,8 +120,10 @@ static std::ostream& operator<<(std::ostream& out, TreeBuilder::Config tbc) { out << "TreeBuilde::Config = ["; out << "\n logAllStructs = " << tbc.logAllStructs; - out << "\n chaseRawPointers = " << tbc.chaseRawPointers; - out << "\n genPaddingStats = " << tbc.genPaddingStats; + out << "\n chaseRawPointers = " + << tbc.features.contains(Feature::ChaseRawPointers); + out << "\n genPaddingStats = " + << tbc.features.contains(Feature::GenPaddingStats); out << "\n dumpDataSegment = " << tbc.dumpDataSegment; out << "\n jsonPath = " << (tbc.jsonPath ? *tbc.jsonPath : "NONE"); out << "\n]\n"; @@ -134,9 +138,8 @@ int main(int argc, char* argv[]) { /* Reflects `oid`'s defaults for TreeBuilder::Config */ TreeBuilder::Config tbConfig{ + .features = {Feature::PackStructs, Feature::GenPaddingStats}, .logAllStructs = true, - .chaseRawPointers = false, - .genPaddingStats = true, .dumpDataSegment = false, .jsonPath = std::nullopt, }; @@ -154,10 +157,10 @@ int main(int argc, char* argv[]) { true; // Weird that we're setting it to true, again... break; case 'n': - tbConfig.chaseRawPointers = true; + tbConfig.features.insert(Feature::ChaseRawPointers); break; case 'w': - tbConfig.genPaddingStats = false; + tbConfig.features.erase(Feature::GenPaddingStats); break; case 'J': tbConfig.jsonPath = optarg ? optarg : "oid_out.json"; @@ -186,7 +189,7 @@ int main(int argc, char* argv[]) { TreeBuilder typeTree(tbConfig); - if (tbConfig.genPaddingStats) { + if (tbConfig.features.contains(Feature::GenPaddingStats)) { LOG(INFO) << "Setting-up PaddingHunter..."; typeTree.setPaddedStructs(&paddingInfos); }