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

@@ -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
 )

include/oi/oi-jit.h

@@ -34,7 +34,7 @@ class OILibraryImpl;
 namespace oi {
 
 struct GeneratorOptions {
-  std::filesystem::path configFilePath;
+  std::vector<std::filesystem::path> configFilePaths;
   std::filesystem::path sourceFileDumpPath;
   int debugLevel = 0;
 };

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 <glog/logging.h>
 
@@ -27,16 +27,48 @@
 
 namespace fs = std::filesystem;
 
-namespace oi::detail::utils {
+namespace oi::detail::config {
 
 using namespace std::literals;
 
-std::optional<FeatureSet> processConfigFile(
-    const std::string& configFilePath,
+namespace {
+
+std::optional<FeatureSet> processConfigFile(const std::string& configFilePath,
+                                            OICompiler::Config& compilerConfig,
+                                            OICodeGen::Config& generatorConfig);
+
+}
+
+std::optional<FeatureSet> processConfigFiles(
+    std::span<const fs::path> configFilePaths,
     std::map<Feature, bool> 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<FeatureSet> 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<FeatureSet> 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<FeatureSet> 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<FeatureSet> 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

oi/Config.h

@@ -15,18 +15,21 @@
  */
 #pragma once
 
+#include <filesystem>
 #include <optional>
 #include <set>
+#include <span>
 
 #include "oi/Features.h"
 #include "oi/OICodeGen.h"
 #include "oi/OICompiler.h"
 
-namespace oi::detail::utils {
+namespace oi::detail::config {
 
-std::optional<FeatureSet> processConfigFile(const std::string& configFilePath,
+std::optional<FeatureSet> processConfigFiles(
+    std::span<const std::filesystem::path> configFilePaths,
     std::map<Feature, bool> featureMap,
     OICompiler::Config& compilerConfig,
     OICodeGen::Config& generatorConfig);
 
-}  // namespace oi::detail::utils
+}  // namespace oi::detail::config

oi/EnumBitset.h

@@ -47,6 +47,11 @@ class EnumBitset {
     return bitset.none();
   }
 
+  EnumBitset<T, N>& operator|=(const EnumBitset<T, N>& that) {
+    bitset |= that.bitset;
+    return *this;
+  }
+
  private:
   BitsetType bitset;
 };

oi/OID.cpp

@@ -23,17 +23,19 @@
 #include <filesystem>
 #include <iostream>
 #include <map>
+#include <string>
+#include <vector>
 
 extern "C" {
 #include <getopt.h>
 #include <libgen.h>
 }
 
+#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<fs::path> 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,7 +671,7 @@ int main(int argc, char* argv[]) {
       .jsonPath = jsonPath,
   };
 
-  auto featureSet = utils::processConfigFile(oidConfig.configFile, features,
+  auto featureSet = config::processConfigFiles(oidConfig.configFiles, features,
                                                compilerConfig, codeGenConfig);
   if (!featureSet) {
     return ExitStatus::UsageError;

oi/OIDebugger.cpp

@@ -45,11 +45,11 @@ extern "C" {
 #include <glog/logging.h>
 
 #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"

oi/OIGenerator.cpp

@@ -26,9 +26,9 @@
 #include <variant>
 
 #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<fs::path>{configFilePath},
+                                 featuresMap, compilerConfig, generatorConfig);
   if (!features) {
     LOG(ERROR) << "failed to process config file";
     return -1;

oi/OILibraryImpl.cpp

@@ -25,9 +25,9 @@
 #include <fstream>
 #include <stdexcept>
 
+#include "oi/Config.h"
 #include "oi/DrgnUtils.h"
 #include "oi/Headers.h"
-#include "oi/OIUtils.h"
 
 namespace oi::detail {
 namespace {
@@ -94,7 +94,7 @@ std::pair<void*, const exporters::inst::Inst&> OILibraryImpl::init() {
 
 void OILibraryImpl::processConfigFile() {
   auto features =
-      utils::processConfigFile(opts_.configFilePath, requestedFeatures_,
+      config::processConfigFiles(opts_.configFilePaths, requestedFeatures_,
                                  compilerConfig_, generatorConfig_);
   if (!features)
     throw std::runtime_error("failed to process configuration");

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<std::filesystem::path> 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 <boost/property_tree/json_parser.hpp>\n"
             "#include <boost/property_tree/ptree.hpp>\n"
+            "#include <filesystem>\n"
             "#include <fstream>\n"
             "#include <gmock/gmock.h>\n"
             "#include <gtest/gtest.h>\n"
-            "#include <string>\n"
             "#include <sstream>\n"
+            "#include <string>\n"
+            "#include <vector>\n"
             '#include "runner_common.h"\n'
             "\n"
             "namespace ba = boost::asio;\n"

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<std::filesystem::path> 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<decltype(el)>) {
-          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<decltype(el)>) {
-            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;

test/integration/runner_common.h

@@ -9,6 +9,7 @@
 #include <future>
 #include <iostream>
 #include <string>
+#include <string_view>
 
 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<std::filesystem::path> writeCustomConfig(
+      std::string_view filePrefix, std::string_view content);
 
   std::filesystem::path workingDir;