Skip to content

Commit

Permalink
containers: add required features (#374)
Browse files Browse the repository at this point in the history
Summary:
Adds the option for required features to container definitions. These cause the container not to be passed to `DrgnParser` if that feature is not enabled during code generation. The thrift isset type does not currently work with `tree-builder-v2` and only provides benefit with `capture-thrift-isset`. This change makes sure the container is ignored if it won't be useful, allowing code generation under `tree-builder-v2`.


Test Plan: - CI

Differential Revision: D49960512

Pulled By: JakeHillion
  • Loading branch information
JakeHillion authored and facebook-github-bot committed Oct 9, 2023
1 parent 593a141 commit 4c6f232
Show file tree
Hide file tree
Showing 8 changed files with 46 additions and 2 deletions.
5 changes: 4 additions & 1 deletion oi/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,20 @@ target_link_libraries(symbol_service
dw
)

add_library(features Features.cpp)
target_link_libraries(features glog::glog)

add_library(container_info
ContainerInfo.cpp
)
target_link_libraries(container_info
features
glog::glog
toml
)

add_library(codegen
CodeGen.cpp
Features.cpp
FuncGen.cpp
OICodeGen.cpp
)
Expand Down
4 changes: 4 additions & 0 deletions oi/CodeGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,10 @@ bool CodeGen::codegenFromDrgn(struct drgn_type* drgnType, std::string& code) {

void CodeGen::registerContainer(const fs::path& path) {
auto info = std::make_unique<ContainerInfo>(path);
if (info->requiredFeatures != (config_.features & info->requiredFeatures)) {
VLOG(1) << "Skipping container (feature conflict): " << info->typeName;
return;
}
VLOG(1) << "Registered container: " << info->typeName;
containerInfos_.emplace_back(std::move(info));
}
Expand Down
13 changes: 13 additions & 0 deletions oi/ContainerInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,19 @@ ContainerInfo::ContainerInfo(const fs::path& path) {

underlyingContainerIndex = info["underlying_container_index"].value<size_t>();

if (toml::array* arr = info["required_features"].as_array()) {
arr->for_each([&](auto&& el) {
if constexpr (toml::is_string<decltype(el)>) {
oi::detail::Feature f = oi::detail::featureFromStr(*el);
if (f == oi::detail::Feature::UnknownFeature) {
LOG(WARNING) << "unknown feature in container config: " << el;
return;
}
requiredFeatures[f] = true;
}
});
}

if (!container["codegen"].is_table()) {
throw ContainerInfoError(
path, "a container info file requires a `codegen` table");
Expand Down
2 changes: 2 additions & 0 deletions oi/ContainerInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <vector>

#include "oi/ContainerTypeEnum.h"
#include "oi/Features.h"

ContainerTypeEnum containerTypeEnumFromStr(std::string& str);
const char* containerTypeEnumToStr(ContainerTypeEnum ty);
Expand Down Expand Up @@ -94,6 +95,7 @@ struct ContainerInfo {
std::optional<size_t> underlyingContainerIndex{};
std::vector<size_t> stubTemplateParams{};
bool captureKeys = false;
oi::detail::FeatureSet requiredFeatures;

Codegen codegen;

Expand Down
15 changes: 15 additions & 0 deletions oi/EnumBitset.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,26 @@ class EnumBitset {
return bitset.none();
}

bool operator==(const EnumBitset<T, N>& that) const {
return bitset == that.bitset;
}
EnumBitset<T, N>& operator|=(const EnumBitset<T, N>& that) {
bitset |= that.bitset;
return *this;
}
EnumBitset<T, N>& operator&=(const EnumBitset<T, N>& that) {
bitset &= that.bitset;
return *this;
}

private:
BitsetType bitset;
};

template <typename T, size_t N>
EnumBitset<T, N> operator&(const EnumBitset<T, N>& lhs,
const EnumBitset<T, N>& rhs) {
auto out = lhs;
out &= rhs;
return out;
}
1 change: 0 additions & 1 deletion test/integration/thrift_isset.toml
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,6 @@ namespace cpp2 {
]}]'''

[cases.no_capture]
oil_skip = 'oil does not support thrift isset yet' # https://github.com/facebookexperimental/object-introspection/issues/296
param_types = ["const cpp2::MyThriftStructBoxed&"]
setup = '''
cpp2::MyThriftStructBoxed ret;
Expand Down
7 changes: 7 additions & 0 deletions types/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ This document describes the format of the container definition files contained i
Only used for container adapters. Points OI to the template parameter
representing the underlying container to be measured.

- `required_features`

A set of feature names such as `tree-builder-v2` which must be enabled for
this container description to be included. Currently only supported with
CodeGen v2 as that's their only use case and an implementation for CodeGen v1
would be untested.

### codegen
- `decl`

Expand Down
1 change: 1 addition & 0 deletions types/thrift_isset_type.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
type_name = "apache::thrift::detail::isset_bitset"
ctype = "THRIFT_ISSET_TYPE"
header = "thrift/lib/cpp2/gen/module_types_h.h"
required_features = ["capture-thrift-isset"]

# Old:
typeName = "apache::thrift::detail::isset_bitset<"
Expand Down

0 comments on commit 4c6f232

Please sign in to comment.