From 37991140da0a94fdd948a725ea8a6e10c0090c29 Mon Sep 17 00:00:00 2001 From: Jake Hillion Date: Fri, 29 Sep 2023 14:36:00 -0700 Subject: [PATCH] support 0 to many config files (#371) Summary: Previously OID/OIL required exactly one configuration file. This change makes it so you can supply 0 or more configuration files. 0 is useful if you have pre-generated the cache or use some sort of remote generation system. 1 is useful for the common case, where you have a configuration file that describes your entire source and use just that. More are useful if you have supplemental bits of config you wish to apply/override - see the changes to the integration test framework where we do exactly this. Test Plan: This isn't super well tested. It works for the test cases which add features via the config or enable `codegen.ignore`. - CI Reviewed By: ajor Differential Revision: D49758032 Pulled By: JakeHillion --- CMakeLists.txt | 2 +- include/oi/oi-jit.h | 2 +- oi/{OIUtils.cpp => Config.cpp} | 62 ++++++++++++++------- oi/{OIUtils.h => Config.h} | 15 +++--- oi/EnumBitset.h | 5 ++ oi/OID.cpp | 33 ++++-------- oi/OIDebugger.cpp | 2 +- oi/OIGenerator.cpp | 7 +-- oi/OILibraryImpl.cpp | 6 +-- test/integration/gen_tests.py | 20 ++++--- test/integration/runner_common.cpp | 87 +++++++++++------------------- test/integration/runner_common.h | 5 +- 12 files changed, 126 insertions(+), 120 deletions(-) rename oi/{OIUtils.cpp => Config.cpp} (86%) rename oi/{OIUtils.h => Config.h} (66%) diff --git a/CMakeLists.txt b/CMakeLists.txt index ed4040c..380fdea 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -278,11 +278,11 @@ include_directories(${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR}) add_subdirectory(oi) add_subdirectory(resources) add_library(oicore + oi/Config.cpp oi/Descs.cpp oi/Metrics.cpp oi/OICache.cpp oi/OICompiler.cpp - oi/OIUtils.cpp oi/PaddingHunter.cpp oi/Serialize.cpp ) diff --git a/include/oi/oi-jit.h b/include/oi/oi-jit.h index 4ab3c2b..5286093 100644 --- a/include/oi/oi-jit.h +++ b/include/oi/oi-jit.h @@ -34,7 +34,7 @@ class OILibraryImpl; namespace oi { struct GeneratorOptions { - std::filesystem::path configFilePath; + std::vector configFilePaths; std::filesystem::path sourceFileDumpPath; int debugLevel = 0; }; diff --git a/oi/OIUtils.cpp b/oi/Config.cpp similarity index 86% rename from oi/OIUtils.cpp rename to oi/Config.cpp index 7b99db5..a752a7e 100644 --- a/oi/OIUtils.cpp +++ b/oi/Config.cpp @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include "oi/OIUtils.h" +#include "oi/Config.h" #include @@ -27,16 +27,48 @@ namespace fs = std::filesystem; -namespace oi::detail::utils { +namespace oi::detail::config { using namespace std::literals; -std::optional processConfigFile( - const std::string& configFilePath, +namespace { + +std::optional processConfigFile(const std::string& configFilePath, + OICompiler::Config& compilerConfig, + OICodeGen::Config& generatorConfig); + +} + +std::optional processConfigFiles( + std::span configFilePaths, std::map featureMap, OICompiler::Config& compilerConfig, OICodeGen::Config& generatorConfig) { - fs::path configDirectory = fs::path(configFilePath).remove_filename(); + FeatureSet enables; + FeatureSet disables; + + for (fs::path p : configFilePaths) { + auto fs = processConfigFile(p, compilerConfig, generatorConfig); + if (!fs.has_value()) + return std::nullopt; + enables |= *fs; + } + + // Override anything from the config files with command line options + for (auto [k, v] : featureMap) { + enables[k] = v; + disables[k] = !v; + } + return handleFeatureConflicts(enables, disables); +} + +namespace { + +std::optional processConfigFile( + const std::string& configFilePath, + OICompiler::Config& compilerConfig, + OICodeGen::Config& generatorConfig) { + fs::path configDirectory = fs::path{configFilePath}.remove_filename(); toml::table config; try { @@ -47,6 +79,7 @@ std::optional processConfigFile( return {}; } + FeatureSet enabledFeatures; if (toml::array* features = config["features"].as_array()) { for (auto&& el : *features) { auto* featureStr = el.as_string(); @@ -57,10 +90,7 @@ std::optional processConfigFile( 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}); + enabledFeatures[f] = true; } else { LOG(ERROR) << "unrecognised feature: " << featureStr->get() << " specified in config"; @@ -204,16 +234,8 @@ std::optional processConfigFile( } } - FeatureSet enabledFeatures; - FeatureSet disabledFeatures; - for (auto [k, v] : featureMap) { - if (v) { - enabledFeatures[k] = true; - } else { - disabledFeatures[k] = true; - } - } - return handleFeatureConflicts(enabledFeatures, disabledFeatures); + return enabledFeatures; } -} // namespace oi::detail::utils +} // namespace +} // namespace oi::detail::config diff --git a/oi/OIUtils.h b/oi/Config.h similarity index 66% rename from oi/OIUtils.h rename to oi/Config.h index 76ce8e2..0e0f2eb 100644 --- a/oi/OIUtils.h +++ b/oi/Config.h @@ -15,18 +15,21 @@ */ #pragma once +#include #include #include +#include #include "oi/Features.h" #include "oi/OICodeGen.h" #include "oi/OICompiler.h" -namespace oi::detail::utils { +namespace oi::detail::config { -std::optional processConfigFile(const std::string& configFilePath, - std::map featureMap, - OICompiler::Config& compilerConfig, - OICodeGen::Config& generatorConfig); +std::optional processConfigFiles( + std::span configFilePaths, + std::map featureMap, + OICompiler::Config& compilerConfig, + OICodeGen::Config& generatorConfig); -} // namespace oi::detail::utils +} // namespace oi::detail::config diff --git a/oi/EnumBitset.h b/oi/EnumBitset.h index 1370a18..96b798b 100644 --- a/oi/EnumBitset.h +++ b/oi/EnumBitset.h @@ -47,6 +47,11 @@ class EnumBitset { return bitset.none(); } + EnumBitset& operator|=(const EnumBitset& that) { + bitset |= that.bitset; + return *this; + } + private: BitsetType bitset; }; diff --git a/oi/OID.cpp b/oi/OID.cpp index 9e16060..34055ee 100644 --- a/oi/OID.cpp +++ b/oi/OID.cpp @@ -23,17 +23,19 @@ #include #include #include +#include +#include extern "C" { #include #include } +#include "oi/Config.h" #include "oi/Features.h" #include "oi/Metrics.h" #include "oi/OIDebugger.h" #include "oi/OIOpts.h" -#include "oi/OIUtils.h" #include "oi/PaddingHunter.h" #include "oi/TimeUtils.h" #include "oi/TreeBuilder.h" @@ -249,7 +251,7 @@ namespace Oid { struct Config { pid_t pid; std::string debugInfoFile; - std::string configFile; + std::vector configFiles; fs::path cacheBasePath; fs::path customCodeFile; size_t dataSegSize; @@ -563,10 +565,11 @@ int main(int argc, char* argv[]) { oidConfig.compAndExit = true; break; case 'c': - oidConfig.configFile = std::string(optarg); + oidConfig.configFiles.emplace_back(optarg); - if (!fs::exists(oidConfig.configFile)) { - LOG(ERROR) << "Non existent config file: " << oidConfig.configFile; + if (!fs::exists(oidConfig.configFiles.back())) { + LOG(ERROR) << "Non existent config file: " + << oidConfig.configFiles.back(); usage(); return ExitStatus::FileNotFoundError; } @@ -630,27 +633,13 @@ int main(int argc, char* argv[]) { } } - if (oidConfig.configFile.empty()) { - oidConfig.configFile = "/usr/local/share/oi/base.oid.toml"; - - if (!fs::exists(oidConfig.configFile)) { - LOG(ERROR) << "Non existent default config file: " - << oidConfig.configFile; - usage(); - return ExitStatus::FileNotFoundError; - } - - LOG(INFO) << "Using default config file " << oidConfig.configFile; - } - if (oidConfig.pid != 0 && !oidConfig.debugInfoFile.empty()) { LOG(INFO) << "'-p' and '-b' are mutually exclusive"; usage(); return ExitStatus::UsageError; } - if ((oidConfig.pid == 0 && oidConfig.debugInfoFile.empty()) || - oidConfig.configFile.empty()) { + if ((oidConfig.pid == 0 && oidConfig.debugInfoFile.empty())) { usage(); return ExitStatus::UsageError; } @@ -682,8 +671,8 @@ int main(int argc, char* argv[]) { .jsonPath = jsonPath, }; - auto featureSet = utils::processConfigFile(oidConfig.configFile, features, - compilerConfig, codeGenConfig); + auto featureSet = config::processConfigFiles(oidConfig.configFiles, features, + compilerConfig, codeGenConfig); if (!featureSet) { return ExitStatus::UsageError; } diff --git a/oi/OIDebugger.cpp b/oi/OIDebugger.cpp index 8d9e87c..705595d 100644 --- a/oi/OIDebugger.cpp +++ b/oi/OIDebugger.cpp @@ -45,11 +45,11 @@ extern "C" { #include #include "oi/CodeGen.h" +#include "oi/Config.h" #include "oi/ContainerInfo.h" #include "oi/Headers.h" #include "oi/Metrics.h" #include "oi/OILexer.h" -#include "oi/OIUtils.h" #include "oi/PaddingHunter.h" #include "oi/Syscall.h" #include "oi/type_graph/DrgnParser.h" diff --git a/oi/OIGenerator.cpp b/oi/OIGenerator.cpp index 83a43a3..a31f63f 100644 --- a/oi/OIGenerator.cpp +++ b/oi/OIGenerator.cpp @@ -26,9 +26,9 @@ #include #include "oi/CodeGen.h" +#include "oi/Config.h" #include "oi/DrgnUtils.h" #include "oi/Headers.h" -#include "oi/OIUtils.h" namespace oi::detail { @@ -193,8 +193,9 @@ int OIGenerator::generate(fs::path& primaryObject, SymbolService& symbols) { OICompiler::Config compilerConfig{}; compilerConfig.usePIC = pic; - auto features = utils::processConfigFile(configFilePath, featuresMap, - compilerConfig, generatorConfig); + auto features = + config::processConfigFiles(std::vector{configFilePath}, + featuresMap, compilerConfig, generatorConfig); if (!features) { LOG(ERROR) << "failed to process config file"; return -1; diff --git a/oi/OILibraryImpl.cpp b/oi/OILibraryImpl.cpp index 15d47f9..bade97c 100644 --- a/oi/OILibraryImpl.cpp +++ b/oi/OILibraryImpl.cpp @@ -25,9 +25,9 @@ #include #include +#include "oi/Config.h" #include "oi/DrgnUtils.h" #include "oi/Headers.h" -#include "oi/OIUtils.h" namespace oi::detail { namespace { @@ -94,8 +94,8 @@ std::pair OILibraryImpl::init() { void OILibraryImpl::processConfigFile() { auto features = - utils::processConfigFile(opts_.configFilePath, requestedFeatures_, - compilerConfig_, generatorConfig_); + config::processConfigFiles(opts_.configFilePaths, requestedFeatures_, + compilerConfig_, generatorConfig_); if (!features) throw std::runtime_error("failed to process configuration"); diff --git a/test/integration/gen_tests.py b/test/integration/gen_tests.py index 71c8504..2dbc3ff 100644 --- a/test/integration/gen_tests.py +++ b/test/integration/gen_tests.py @@ -136,7 +136,7 @@ def add_test_setup(f, config): oil_func_body = ( f" oi::GeneratorOptions opts{{\n" - f" .configFilePath = configFile,\n" + f" .configFilePaths = configFiles,\n" f' .sourceFileDumpPath = "oil_jit_code.cpp",\n' f" .debugLevel = 3,\n" f" }};\n\n" @@ -164,11 +164,11 @@ def add_common_code(f): """ void usage(const std::string &argv0) { std::cerr << "usage: " << argv0 << " oid CASE ITERATIONS" << std::endl; - std::cerr << " " << argv0 << " oil CASE CONFIG_FILE" << std::endl; + std::cerr << " " << argv0 << " oil CASE CONFIG_FILE..." << std::endl; } int main(int argc, char *argv[]) { - if (argc != 4) { + if (argc < 4) { usage(argv[0]); return -1; } @@ -179,6 +179,10 @@ int main(int argc, char *argv[]) { int iterations = 1; if (mode == "oid") { + if (argc != 4) { + usage(argv[0]); + return -1; + } std::istringstream iss(argv[3]); iss >> iterations; if (iss.fail()) { @@ -187,7 +191,9 @@ int main(int argc, char *argv[]) { } } else if (mode == "oil") { - configFile = argv[3]; + for (int i = 3; i < argc; i++) { + configFiles.emplace_back(argv[i]); + } } else { usage(argv[0]); @@ -241,7 +247,7 @@ def gen_target(output_target_name, test_configs): f"thrift/annotation/gen-cpp2/{config['suite']}_types.h" ] add_headers(f, sorted(headers), thrift_headers) - f.write("std::string configFile;") + f.write("std::vector configFiles;") for config in test_configs: add_test_setup(f, config) @@ -415,11 +421,13 @@ def gen_runner(output_runner_name, test_configs): f.write( "#include \n" "#include \n" + "#include \n" "#include \n" "#include \n" "#include \n" - "#include \n" "#include \n" + "#include \n" + "#include \n" '#include "runner_common.h"\n' "\n" "namespace ba = boost::asio;\n" diff --git a/test/integration/runner_common.cpp b/test/integration/runner_common.cpp index cecf6fd..71b574c 100644 --- a/test/integration/runner_common.cpp +++ b/test/integration/runner_common.cpp @@ -136,55 +136,15 @@ int IntegrationBase::exit_code(Proc& proc) { return proc.proc.exit_code(); } -fs::path IntegrationBase::createCustomConfig(const std::string& prefix, - const std::string& suffix) { - // If no extra config provided, return the config path unaltered. - if (prefix.empty() && suffix.empty()) { - return configFile; - } +std::optional IntegrationBase::writeCustomConfig( + std::string_view filePrefix, std::string_view content) { + if (content.empty()) + return std::nullopt; - auto customConfigFile = workingDir / "oid.config.toml"; - auto config = toml::parse_file(configFile); - - // As relative paths are allowed, we must canonicalise the paths before - // moving the file to the temporary directory. - fs::path configDirectory = fs::path(configFile).remove_filename(); - - if (toml::table* types = config["types"].as_table()) { - if (toml::array* arr = (*types)["containers"].as_array()) { - arr->for_each([&](auto&& el) { - if constexpr (toml::is_string) { - el = configDirectory / el.get(); - } - }); - } - } - if (toml::table* headers = config["headers"].as_table()) { - for (auto& path : {"user_paths", "system_paths"}) { - if (toml::array* arr = (*headers)[path].as_array()) { - arr->for_each([&](auto&& el) { - if constexpr (toml::is_string) { - el = configDirectory / el.get(); - } - }); - } - } - } - - 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; - 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; + auto path = workingDir / (std::string{filePrefix} + ".config.toml"); + std::ofstream file{path, std::ios_base::app}; + file << content; + return path; } std::string OidIntegration::TmpDirStr() { @@ -219,8 +179,6 @@ OidProc OidIntegration::runOidOnProcess(OidOpts opts, std::ofstream touch(segconfigPath); } - 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. // clang-format off @@ -228,7 +186,6 @@ OidProc OidIntegration::runOidOnProcess(OidOpts opts, "--debug-level=3"s, "--timeout=20"s, "--dump-json"s, - "--config-file"s, thisConfig.string(), "--script-source"s, opts.scriptSource, "--mode=strict"s, "--pid"s, std::to_string(targetProcess.id()), @@ -242,6 +199,19 @@ OidProc OidIntegration::runOidOnProcess(OidOpts opts, oid_args.insert(oid_args.end(), extra_args.begin(), extra_args.end()); oid_args.insert(oid_args.end(), default_args.begin(), default_args.end()); + if (auto prefix = writeCustomConfig("prefix", configPrefix)) { + oid_args.emplace_back("--config-file"); + oid_args.emplace_back(*prefix); + } + + oid_args.emplace_back("--config-file"); + oid_args.emplace_back(configFile); + + if (auto suffix = writeCustomConfig("suffix", configSuffix)) { + oid_args.emplace_back("--config-file"); + oid_args.emplace_back(*suffix); + } + if (verbose) { std::cerr << "Running: " << targetExe << "\n"; std::cerr << "Running: " << oidExe << " "; @@ -386,10 +356,17 @@ std::string OilIntegration::TmpDirStr() { Proc OilIntegration::runOilTarget(OilOpts 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(); + std::string targetExe = + std::string(TARGET_EXE_PATH) + " " + opts.targetArgs + " "; + if (auto prefix = writeCustomConfig("prefix", configPrefix)) { + targetExe += *prefix; + targetExe += " "; + } + targetExe += configFile; + if (auto suffix = writeCustomConfig("suffix", configSuffix)) { + targetExe += " "; + targetExe += *suffix; + } if (verbose) { std::cerr << "Running: " << targetExe << std::endl; diff --git a/test/integration/runner_common.h b/test/integration/runner_common.h index f0cf4db..d2b5e57 100644 --- a/test/integration/runner_common.h +++ b/test/integration/runner_common.h @@ -9,6 +9,7 @@ #include #include #include +#include struct OidOpts { boost::asio::io_context& ctx; @@ -42,8 +43,8 @@ class IntegrationBase : public ::testing::Test { void TearDown() override; void SetUp() override; int exit_code(Proc& proc); - std::filesystem::path createCustomConfig(const std::string& prefix, - const std::string& suffix); + std::optional writeCustomConfig( + std::string_view filePrefix, std::string_view content); std::filesystem::path workingDir;