From d3eb71c53c40b0e134774f76fc4c66b4f5763db9 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Sun, 15 Dec 2019 10:06:48 -0500 Subject: [PATCH 1/4] tools: API boosting support for rewriting elaborated types. This PR introduces API boosting support for ElaboratedTypeLoc, one of a number of Clang AST nodes that we need to analyze and rewrite to move to the latest type according to the type database. The approach taken is to generate a replacements YAML file suitable for consumption by clang-apply-replacements; this is how clang-tidy works internally. Replacements are a series of patch like file transformation operations, Clang has nice support for building replacement sets and serializing to YAML. This PR also starts to split out api_booster into more modules (just some utils to start with), introduces some more unit tests and a golden C++ source file test framework. Risk level: Low Testing: New unit and e2e golden tests added. Signed-off-by: Harvey Tuch --- test/tools/type_whisperer/api_type_db_test.cc | 26 ++- tools/api_boost/api_boost.py | 91 ++++++---- tools/api_boost/api_boost_test.py | 61 +++++++ tools/api_boost/testdata/BUILD | 15 ++ tools/api_boost/testdata/elaborated_type.cc | 6 + .../testdata/elaborated_type.cc.gold | 6 + tools/check_format.py | 8 +- tools/clang_tools/api_booster/BUILD | 22 ++- tools/clang_tools/api_booster/main.cc | 155 +++++++++++++----- .../clang_tools/api_booster/proto_cxx_utils.h | 43 +++++ .../api_booster/proto_cxx_utils_test.cc | 24 +++ tools/clang_tools/support/clang_tools.bzl | 15 +- tools/type_whisperer/api_type_db.cc | 15 +- tools/type_whisperer/api_type_db.h | 14 +- 14 files changed, 404 insertions(+), 97 deletions(-) create mode 100755 tools/api_boost/api_boost_test.py create mode 100644 tools/api_boost/testdata/BUILD create mode 100644 tools/api_boost/testdata/elaborated_type.cc create mode 100644 tools/api_boost/testdata/elaborated_type.cc.gold create mode 100644 tools/clang_tools/api_booster/proto_cxx_utils.h create mode 100644 tools/clang_tools/api_booster/proto_cxx_utils_test.cc diff --git a/test/tools/type_whisperer/api_type_db_test.cc b/test/tools/type_whisperer/api_type_db_test.cc index e125b8d67806..972eb4e44287 100644 --- a/test/tools/type_whisperer/api_type_db_test.cc +++ b/test/tools/type_whisperer/api_type_db_test.cc @@ -6,14 +6,28 @@ namespace Tools { namespace TypeWhisperer { namespace { -TEST(ApiTypeDb, GetProtoPathForTypeUnknown) { - const auto unknown_type_path = ApiTypeDb::getProtoPathForType("foo"); - EXPECT_EQ(absl::nullopt, unknown_type_path); +// Validate that ApiTypeDb::getLatestTypeInformation returns nullopt when no +// type information exists. +TEST(ApiTypeDb, GetLatestTypeInformationForTypeUnknown) { + const auto unknown_type_information = ApiTypeDb::getLatestTypeInformation("foo"); + EXPECT_EQ(absl::nullopt, unknown_type_information); } -TEST(ApiTypeDb, GetProtoPathForTypeKnown) { - const auto known_type_path = ApiTypeDb::getProtoPathForType("envoy.type.Int64Range"); - EXPECT_EQ("envoy/type/range.proto", *known_type_path); +// Validate that ApiTypeDb::getLatestTypeInformation fetches the latest type +// information when an upgrade occurs. +TEST(ApiTypeDb, GetLatestTypeInformationForTypeKnownUpgraded) { + const auto known_type_information = ApiTypeDb::getLatestTypeInformation("envoy.type.Int64Range"); + EXPECT_EQ("envoy.type.v3alpha.Int64Range", known_type_information->type_name_); + EXPECT_EQ("envoy/type/v3alpha/range.proto", known_type_information->proto_path_); +} + +// Validate that ApiTypeDb::getLatestTypeInformation is idempotent when no +// upgrade occurs. +TEST(ApiTypeDb, GetLatestTypeInformationForTypeKnownNoUpgrade) { + const auto known_type_information = + ApiTypeDb::getLatestTypeInformation("envoy.type.v3alpha.Int64Range"); + EXPECT_EQ("envoy.type.v3alpha.Int64Range", known_type_information->type_name_); + EXPECT_EQ("envoy/type/v3alpha/range.proto", known_type_information->proto_path_); } } // namespace diff --git a/tools/api_boost/api_boost.py b/tools/api_boost/api_boost.py index 4bcd7b4b17d5..30d447a97946 100755 --- a/tools/api_boost/api_boost.py +++ b/tools/api_boost/api_boost.py @@ -26,6 +26,12 @@ API_INCLUDE_REGEX = re.compile('#include "(envoy/.*)/[^/]+\.pb\.(validate\.)?h"') +# Obtain the directory containing a path prefix, e.g. ./foo/bar.txt is ./foo, +# ./foo/ba is ./foo, ./foo/bar/ is ./foo/bar. +def PrefixDirectory(path_prefix): + return path_prefix if os.path.isdir(path_prefix) else os.path.dirname(path_prefix) + + # Update a C++ file to the latest API. def ApiBoostFile(llvm_include_path, debug_log, path): print('Processing %s' % path) @@ -44,17 +50,22 @@ def ApiBoostFile(llvm_include_path, debug_log, path): if debug_log: print(result.stderr.decode('utf-8')) - # Consume stdout containing the list of inferred API headers. We don't have - # rewrite capabilities yet in the API booster, so we rewrite here in Python - # below. - inferred_api_includes = sorted(set(result.stdout.decode('utf-8').splitlines())) + # Consume stdout containing the list of inferred API headers. + return sorted(set(result.stdout.decode('utf-8').splitlines())) + +# Rewrite API includes to the inferred headers. Currently this is handled +# outside of the clang-ast-replacements. In theory we could either integrate +# with this or with clang-include-fixer, but it's pretty simply to handle as done +# below, we have more control over special casing as well, so ¯\_(ツ)_/¯. +def RewriteIncludes(args): + path, api_includes = args # We just dump the inferred API header includes at the start of the #includes # in the file and remove all the present API header includes. This does not # match Envoy style; we rely on later invocations of fix_format.sh to take # care of this alignment. output_lines = [] - include_lines = ['#include "%s"' % f for f in inferred_api_includes] + include_lines = ['#include "%s"' % f for f in api_includes] input_text = pathlib.Path(path).read_text() for line in input_text.splitlines(): if include_lines and line.startswith('#include'): @@ -66,35 +77,29 @@ def ApiBoostFile(llvm_include_path, debug_log, path): if re.match(API_INCLUDE_REGEX, line) and 'envoy/service/auth/v2alpha' not in line: continue output_lines.append(line) - - # Write to temporary file. We can't overwrite in place as we're executing - # concurrently with other ApiBoostFile() invocations that might need the file - # we're writing to. - pathlib.Path(path + TMP_SWP_SUFFIX).write_text('\n'.join(output_lines) + '\n') - - -# Replace the original file with the temporary file created by ApiBoostFile() -# for a given path. -def SwapTmpFile(path): - pathlib.Path(path + TMP_SWP_SUFFIX).rename(path) + # Rewrite file. + pathlib.Path(path).write_text('\n'.join(output_lines) + '\n') # Update the Envoy source tree the latest API. -def ApiBoostTree(args): +def ApiBoostTree(target_paths, + generate_compilation_database=False, + build_api_booster=False, + debug_log=False): + dep_build_targets = ['//%s/...' % PrefixDirectory(prefix) for prefix in target_paths] + # Optional setup of state. We need the compilation database and api_booster # tool in place before we can start boosting. - if args.generate_compilation_database: - sp.run(['./tools/gen_compilation_database.py', '--run_bazel_build', '--include_headers'], + if generate_compilation_database: + sp.run(['./tools/gen_compilation_database.py', '--run_bazel_build', '--include_headers'] + + dep_build_targets, check=True) - if args.build_api_booster: + if build_api_booster: # Similar to gen_compilation_database.py, we only need the cc_library for # setup. The long term fix for this is in # https://github.com/bazelbuild/bazel/issues/9578. - dep_build_targets = [ - '//source/...', - '//test/...', - ] + # # Figure out some cc_libraries that cover most of our external deps. This is # the same logic as in gen_compilation_database.py. query = 'kind(cc_library, {})'.format(' union '.join(dep_build_targets)) @@ -104,7 +109,7 @@ def ApiBoostTree(args): query = 'attr("tags", "compilation_db_dep", {})'.format(' union '.join(dep_build_targets)) dep_lib_build_targets.extend(sp.check_output(['bazel', 'query', query]).decode().splitlines()) extra_api_booster_args = [] - if args.debug_log: + if debug_log: extra_api_booster_args.append('--copt=-DENABLE_DEBUG_LOG') # Slightly easier to debug when we build api_booster on its own. @@ -132,22 +137,35 @@ def ApiBoostTree(args): # Determine the files in the target dirs eligible for API boosting, based on # known files in the compilation database. - paths = set([]) + file_paths = set([]) for entry in json.loads(pathlib.Path('compile_commands.json').read_text()): file_path = entry['file'] - if any(file_path.startswith(prefix) for prefix in args.paths): - paths.add(file_path) + if any(file_path.startswith(prefix) for prefix in target_paths): + file_paths.add(file_path) # The API boosting is file local, so this is trivially parallelizable, use # multiprocessing pool with default worker pool sized to cpu_count(), since # this is CPU bound. - with mp.Pool() as p: - # We need two phases, to ensure that any dependency on files being modified - # in one thread on consumed transitive headers on the other thread isn't an - # issue. This also ensures that we complete all analysis error free before - # any mutation takes place. - p.map(functools.partial(ApiBoostFile, llvm_include_path, args.debug_log), paths) - p.map(SwapTmpFile, paths) + try: + with mp.Pool() as p: + # We need multiple phases, to ensure that any dependency on files being modified + # in one thread on consumed transitive headers on the other thread isn't an + # issue. This also ensures that we complete all analysis error free before + # any mutation takes place. + api_includes = p.map(functools.partial(ApiBoostFile, llvm_include_path, debug_log), + file_paths) + # Apply Clang replacements before header fixups, since the replacements + # are all relative to the original file. + for prefix in target_paths: + sp.run(['clang-apply-replacements', PrefixDirectory(prefix)], check=True) + # Fixup headers. + p.map(RewriteIncludes, zip(file_paths, api_includes)) + finally: + # Cleanup any stray **/*.clang-replacements.yaml. + for prefix in target_paths: + clang_replacements = pathlib.Path(PrefixDirectory(prefix)).glob('**/*.yaml') + for path in clang_replacements: + path.unlink() if __name__ == '__main__': @@ -157,4 +175,5 @@ def ApiBoostTree(args): parser.add_argument('--debug_log', action='store_true') parser.add_argument('paths', nargs='*', default=['source', 'test', 'include']) args = parser.parse_args() - ApiBoostTree(args) + ApiBoostTree(args.paths, args.generate_compilation_database, args.build_api_booster, + args.debug_log) diff --git a/tools/api_boost/api_boost_test.py b/tools/api_boost/api_boost_test.py new file mode 100755 index 000000000000..ef6154db9096 --- /dev/null +++ b/tools/api_boost/api_boost_test.py @@ -0,0 +1,61 @@ +#!/usr/bin/env python3 + +# Golden C++ source tests for API boosting. This is effectively a test for the +# combination of api_boost.py, the Clang libtooling-based +# tools/clang_tools/api_booster, as well as the type whisperer and API type +# database. + +import logging +import os +import pathlib +import shutil +import subprocess +import sys +import tempfile + +import api_boost + +# List of test in the form [(file_name, explanation)] +TESTS = [ + ('elaborated_type.cc', 'ElaboratedTypeLoc type upgrades'), +] + +TESTDATA_PATH = 'tools/api_boost/testdata' + + +def Diff(some_path, other_path): + result = subprocess.run(['diff', '-u', some_path, other_path], capture_output=True) + if result.returncode == 0: + return None + return result.stdout.decode('utf-8') + result.stderr.decode('utf-8') + + +if __name__ == '__main__': + # Accumulated error messages. + logging.basicConfig(format='%(message)s') + messages = [] + + # Run API booster against test artifacts in a directory relative to workspace. + # We use a temporary copy as the API booster does in-place rewriting. + with tempfile.TemporaryDirectory(dir=pathlib.Path.cwd()) as path: + # Setup temporary tree. + shutil.copy(os.path.join(TESTDATA_PATH, 'BUILD'), path) + for filename, _ in TESTS: + shutil.copy(os.path.join(TESTDATA_PATH, filename), path) + + # Run API booster. + api_boost.ApiBoostTree([str(pathlib.Path(path).relative_to(pathlib.Path.cwd()))], + generate_compilation_database=True, + build_api_booster=True, + debug_log=True) + + # Validate output against golden files. + for filename, description in TESTS: + delta = Diff(os.path.join(TESTDATA_PATH, filename + '.gold'), os.path.join(path, filename)) + if delta is not None: + messages.append('Non-empty diff for %s (%s):\n%s\n' % (filename, description, delta)) + + if len(messages) > 0: + logging.error('FAILED:\n{}'.format('\n'.join(messages))) + sys.exit(1) + logging.warning('PASS') diff --git a/tools/api_boost/testdata/BUILD b/tools/api_boost/testdata/BUILD new file mode 100644 index 000000000000..aba697c0b06e --- /dev/null +++ b/tools/api_boost/testdata/BUILD @@ -0,0 +1,15 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "elaborated_type", + srcs = ["elaborated_type.cc"], + deps = ["@envoy_api//envoy/config/overload/v2alpha:pkg_cc_proto"], +) diff --git a/tools/api_boost/testdata/elaborated_type.cc b/tools/api_boost/testdata/elaborated_type.cc new file mode 100644 index 000000000000..765aedf7db5a --- /dev/null +++ b/tools/api_boost/testdata/elaborated_type.cc @@ -0,0 +1,6 @@ +#include "envoy/config/overload/v2alpha/overload.pb.h" + +class ThresholdTriggerImpl { +public: + ThresholdTriggerImpl(const envoy::config::overload::v2alpha::ThresholdTrigger& /*config*/) {} +}; diff --git a/tools/api_boost/testdata/elaborated_type.cc.gold b/tools/api_boost/testdata/elaborated_type.cc.gold new file mode 100644 index 000000000000..fdcb9f756e6e --- /dev/null +++ b/tools/api_boost/testdata/elaborated_type.cc.gold @@ -0,0 +1,6 @@ +#include "envoy/config/overload/v3alpha/overload.pb.h" + +class ThresholdTriggerImpl { +public: + ThresholdTriggerImpl(const envoy::config::overload::v3alpha::ThresholdTrigger& /*config*/) {} +}; diff --git a/tools/check_format.py b/tools/check_format.py index a7bb5daf7426..bf059adec994 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -71,7 +71,8 @@ "./source/extensions/filters/http/squash/squash_filter.h", "./source/extensions/filters/http/squash/squash_filter.cc", "./source/server/http/admin.h", "./source/server/http/admin.cc", - "./tools/clang_tools/api_booster/main.cc") + "./tools/clang_tools/api_booster/main.cc", + "./tools/clang_tools/api_booster/proto_cxx_utils.h") # Only one C++ file should instantiate grpc_init GRPC_INIT_WHITELIST = ("./source/common/grpc/google_grpc_context.cc") @@ -860,7 +861,10 @@ def checkErrorMessages(error_messages): target_path = args.target_path envoy_build_rule_check = not args.skip_envoy_build_rule_check namespace_check = args.namespace_check - namespace_check_excluded_paths = args.namespace_check_excluded_paths + namespace_check_excluded_paths = args.namespace_check_excluded_paths + [ + "./tools/api_boost/testdata/", + "./tools/clang_tools/", + ] build_fixer_check_excluded_paths = args.build_fixer_check_excluded_paths + [ "./bazel/external/", "./bazel/toolchains/", diff --git a/tools/clang_tools/api_booster/BUILD b/tools/clang_tools/api_booster/BUILD index 53872e22279b..727b3e6b1c4b 100644 --- a/tools/clang_tools/api_booster/BUILD +++ b/tools/clang_tools/api_booster/BUILD @@ -1,14 +1,32 @@ -load("//clang_tools/support:clang_tools.bzl", "envoy_clang_tools_cc_binary") +load( + "//clang_tools/support:clang_tools.bzl", + "clang_tools_cc_binary", + "clang_tools_cc_library", + "clang_tools_cc_test", +) licenses(["notice"]) # Apache 2 -envoy_clang_tools_cc_binary( +clang_tools_cc_binary( name = "api_booster", srcs = ["main.cc"], deps = [ + ":proto_cxx_utils_lib", "@clang_tools//:clang_astmatchers", "@clang_tools//:clang_basic", "@clang_tools//:clang_tooling", "@envoy//tools/type_whisperer:api_type_db_lib", ], ) + +clang_tools_cc_library( + name = "proto_cxx_utils_lib", + hdrs = ["proto_cxx_utils.h"], + deps = ["@com_google_absl//absl/strings"], +) + +clang_tools_cc_test( + name = "proto_cxx_utils_test", + srcs = ["proto_cxx_utils_test.cc"], + deps = [":proto_cxx_utils_lib"], +) diff --git a/tools/clang_tools/api_booster/main.cc b/tools/clang_tools/api_booster/main.cc index ab45b7ba5ea5..7a24c8bb7a46 100644 --- a/tools/clang_tools/api_booster/main.cc +++ b/tools/clang_tools/api_booster/main.cc @@ -6,6 +6,7 @@ // // NOLINT(namespace-envoy) +#include #include #include #include @@ -15,15 +16,18 @@ #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Frontend/FrontendActions.h" #include "clang/Tooling/CommonOptionsParser.h" -#include "clang/Tooling/Tooling.h" +#include "clang/Tooling/Core/Replacement.h" +#include "clang/Tooling/Refactoring.h" +#include "clang/Tooling/ReplacementsYaml.h" // Declares llvm::cl::extrahelp. #include "llvm/Support/CommandLine.h" +#include "proto_cxx_utils.h" + #include "tools/type_whisperer/api_type_db.h" -#include "absl/strings/str_join.h" -#include "absl/strings/str_split.h" +#include "absl/strings/str_cat.h" // Enable to see debug log messages. #ifdef ENABLE_DEBUG_LOG @@ -35,19 +39,30 @@ #define DEBUG_LOG(s) #endif +using namespace Envoy::Tools::TypeWhisperer; + +namespace ApiBooster { + class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback, public clang::tooling::SourceFileCallbacks { public: + ApiBooster(std::map& replacements) + : replacements_(replacements) {} + // AST match callback. - void run(const clang::ast_matchers::MatchFinder::MatchResult& result) override { - // If we have a match on type, we should track the corresponding .pb.h. - if (const clang::TypeLoc* type = result.Nodes.getNodeAs("type")) { + void run(const clang::ast_matchers::MatchFinder::MatchResult& match_result) override { + clang::SourceManager& source_manager = match_result.Context->getSourceManager(); + // If we have a match on type, we should track the corresponding .pb.h and + // attempt to upgrade. + if (const clang::TypeLoc* type_loc = match_result.Nodes.getNodeAs("type")) { const std::string type_name = - type->getType().getCanonicalType().getUnqualifiedType().getAsString(); - DEBUG_LOG(absl::StrCat("Matched type ", type_name)); - const auto result = getProtoPathFromCType(type_name); + type_loc->getType().getCanonicalType().getUnqualifiedType().getAsString(); + const auto result = getLatestTypeInformationFromCType(type_name); if (result) { - source_api_proto_paths_.insert(*result + ".pb.h"); + source_api_proto_paths_.insert(adjustProtoSuffix(result->proto_path_, ".pb.h")); + DEBUG_LOG(absl::StrCat("Matched type ", type_name, " ", type_loc->getTypeLocClass(), " ", + type_loc->getType()->getTypeClassName())); + addTypeLocReplacements(*type_loc, result->type_name_, source_manager); } return; } @@ -55,7 +70,8 @@ class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback, // If we have a match on a call expression, check to see if it's something // like loadFromYamlAndValidate; if so, we might need to look at the // argument type to figure out any corresponding .pb.validate.h we require. - if (const clang::CallExpr* call_expr = result.Nodes.getNodeAs("call_expr")) { + if (const clang::CallExpr* call_expr = + match_result.Nodes.getNodeAs("call_expr")) { auto* direct_callee = call_expr->getDirectCallee(); if (direct_callee != nullptr) { const std::unordered_map ValidateNameToArg = { @@ -74,9 +90,10 @@ class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback, .getCanonicalType() .getUnqualifiedType() .getAsString(); - const auto result = getProtoPathFromCType(type_name); + const auto result = getLatestTypeInformationFromCType(type_name); if (result) { - source_api_proto_paths_.insert(*result + ".pb.validate.h"); + source_api_proto_paths_.insert( + adjustProtoSuffix(result->proto_path_, ".pb.validate.h")); } } } @@ -86,7 +103,7 @@ class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback, // The last place we need to look for .pb.validate.h reference is // instantiation of FactoryBase. if (const clang::ClassTemplateSpecializationDecl* tmpl = - result.Nodes.getNodeAs("tmpl")) { + match_result.Nodes.getNodeAs("tmpl")) { const std::string tmpl_type_name = tmpl->getSpecializedTemplate() ->getInjectedClassNameSpecialization() .getCanonicalType() @@ -98,9 +115,9 @@ class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback, .getCanonicalType() .getUnqualifiedType() .getAsString(); - const auto result = getProtoPathFromCType(type_name); + const auto result = getLatestTypeInformationFromCType(type_name); if (result) { - source_api_proto_paths_.insert(*result + ".pb.validate.h"); + source_api_proto_paths_.insert(adjustProtoSuffix(result->proto_path_, ".pb.validate.h")); } } } @@ -122,38 +139,54 @@ class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback, } private: - // Convert from C++ type, e.g. envoy:config::v2::Cluster, to a proto path - // (minus the .proto suffix), e.g. envoy/config/v2/cluster. - absl::optional getProtoPathFromCType(const std::string& c_type_name) { + // Attempt to add type replacements as applicable for Envoy API types. + void addTypeLocReplacements(const clang::TypeLoc& type_loc, + const std::string& latest_proto_type_name, + const clang::SourceManager& source_manager) { + // We only support upgrading ElaboratedTypes so far. TODO(htuch): extend + // this to other AST type matches. + const clang::UnqualTypeLoc unqual_type_loc = type_loc.getUnqualifiedLoc(); + if (unqual_type_loc.getTypeLocClass() == clang::TypeLoc::Elaborated) { + clang::LangOptions lopt; + const clang::SourceLocation start = unqual_type_loc.getSourceRange().getBegin(); + const clang::SourceLocation end = clang::Lexer::getLocForEndOfToken( + unqual_type_loc.getSourceRange().getEnd(), 0, source_manager, lopt); + const size_t length = source_manager.getFileOffset(end) - source_manager.getFileOffset(start); + clang::tooling::Replacement type_replacement( + source_manager, start, length, ProtoCxxUtils::protoToCxxType(latest_proto_type_name)); + llvm::Error error = replacements_[type_replacement.getFilePath()].add(type_replacement); + if (error) { + std::cerr << " Replacement insertion error: " << llvm::toString(std::move(error)) + << std::endl; + } else { + std::cerr << " Replacement added: " << type_replacement.toString() << std::endl; + } + } + } + + // Remove .proto from a path, apply specified suffix instead. + std::string adjustProtoSuffix(absl::string_view proto_path, absl::string_view suffix) { + return absl::StrCat(proto_path.substr(0, proto_path.size() - 6), suffix); + } + + // Obtain the latest type information for a given from C++ type, e.g. envoy:config::v2::Cluster, + // from the API type database. + absl::optional + getLatestTypeInformationFromCType(const std::string& c_type_name) { // Ignore compound or non-API types. - // TODO(htuch): without compound types, this is only an under-approximation - // of the types. Add proper logic to destructor compound types. + // TODO(htuch): this is all super hacky and not really right, we should be + // removing qualifiers etc. to get to the underlying type name. const std::string type_name = std::regex_replace(c_type_name, std::regex("^(class|enum) "), ""); if (!absl::StartsWith(type_name, "envoy::") || absl::StrContains(type_name, " ")) { return {}; } - - // Convert from C++ to a qualified proto type. This is fairly hacky stuff, - // we're essentially reversing the conventions that the protobuf C++ - // compiler is using, e.g. replacing _ and :: with . as needed, guessing - // that a Case suffix implies some enum switching. - const std::string dotted_path = std::regex_replace(type_name, std::regex("::"), "."); - std::vector frags = absl::StrSplit(dotted_path, '.'); - for (std::string& frag : frags) { - if (!frag.empty() && isupper(frag[0])) { - frag = std::regex_replace(frag, std::regex("_"), "."); - } - } - if (absl::EndsWith(frags.back(), "Case")) { - frags.pop_back(); - } - const std::string proto_type_name = absl::StrJoin(frags, "."); + const std::string proto_type_name = ProtoCxxUtils::cxxToProtoType(type_name); // Use API type database to map from proto type to path. - auto result = Envoy::Tools::TypeWhisperer::ApiTypeDb::getProtoPathForType(proto_type_name); + auto result = ApiTypeDb::getLatestTypeInformation(proto_type_name); if (result) { // Remove the .proto extension. - return result->substr(0, result->size() - 6); + return result; } else if (!absl::StartsWith(proto_type_name, "envoy.HotRestart") && !absl::StartsWith(proto_type_name, "envoy.RouterCheckToolSchema") && !absl::StartsWith(proto_type_name, "envoy.test") && @@ -170,18 +203,22 @@ class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback, // Set of inferred .pb[.validate].h, updated as the AST matcher callbacks above fire. std::set source_api_proto_paths_; + // Map from source file to replacements. + std::map& replacements_; }; +} // namespace ApiBooster + int main(int argc, const char** argv) { // Apply a custom category to all command-line options so that they are the // only ones displayed. llvm::cl::OptionCategory api_booster_tool_category("api-booster options"); - clang::tooling::CommonOptionsParser OptionsParser(argc, argv, api_booster_tool_category); - clang::tooling::ClangTool Tool(OptionsParser.getCompilations(), - OptionsParser.getSourcePathList()); + clang::tooling::CommonOptionsParser optionsParser(argc, argv, api_booster_tool_category); + clang::tooling::RefactoringTool tool(optionsParser.getCompilations(), + optionsParser.getSourcePathList()); - ApiBooster api_booster; + ApiBooster::ApiBooster api_booster(tool.getReplacements()); clang::ast_matchers::MatchFinder finder; // Match on all mentions of types in the AST. @@ -199,5 +236,35 @@ int main(int argc, const char** argv) { auto tmpl_matcher = clang::ast_matchers::classTemplateSpecializationDecl().bind("tmpl"); finder.addMatcher(tmpl_matcher, &api_booster); - return Tool.run(newFrontendActionFactory(&finder, &api_booster).get()); + // Apply ApiBooster to AST matches. This will generate a set of replacements in + // tool.getReplacements(). + const int run_result = tool.run(newFrontendActionFactory(&finder, &api_booster).get()); + if (run_result != 0) { + std::cerr << "Exiting with non-zero result " << run_result << std::endl; + return run_result; + } + + // Serialize replacements to
.clang-replacements.yaml. + // These are suitable for consuming by clang-apply-replacements. + for (const auto& file_replacement : tool.getReplacements()) { + // Populate TranslationUnitReplacements from file replacements (this is what + // there exists llvm::yaml serialization support for). + clang::tooling::TranslationUnitReplacements tu_replacements; + tu_replacements.MainSourceFile = file_replacement.first; + for (const auto& r : file_replacement.second) { + tu_replacements.Replacements.push_back(r); + DEBUG_LOG(r.toString()); + } + // Serialize TranslationUnitReplacements to YAML. + std::string yaml_content; + llvm::raw_string_ostream yaml_content_stream(yaml_content); + llvm::yaml::Output yaml(yaml_content_stream); + yaml << tu_replacements; + // Write to
.clang-replacements.yaml. + std::ofstream serialized_replacement_file(tu_replacements.MainSourceFile + + ".clang-replacements.yaml"); + serialized_replacement_file << yaml_content_stream.str(); + } + + return 0; } diff --git a/tools/clang_tools/api_booster/proto_cxx_utils.h b/tools/clang_tools/api_booster/proto_cxx_utils.h new file mode 100644 index 000000000000..26f1f8aaf912 --- /dev/null +++ b/tools/clang_tools/api_booster/proto_cxx_utils.h @@ -0,0 +1,43 @@ +#pragma once + +#include + +#include "absl/strings/str_join.h" +#include "absl/strings/str_split.h" + +namespace ApiBooster { + +// Protobuf C++ code generation hackery. This is where the utilities that map +// between C++ and protobuf types, enum constants and identifiers live. Most of +// this is heuristic and needs to match whatever the protobuf compiler does. +class ProtoCxxUtils { +public: + // Convert from a C++ type, e.g. foo::bar::v2, to a protobuf type, e.g. + // foo.bar.v2. + static std::string cxxToProtoType(const std::string& cxx_type_name) { + // Convert from C++ to a qualified proto type. This is fairly hacky stuff, + // we're essentially reversing the conventions that the protobuf C++ + // compiler is using, e.g. replacing _ and :: with . as needed, guessing + // that a Case suffix implies some enum switching. + const std::string dotted_path = std::regex_replace(cxx_type_name, std::regex("::"), "."); + std::vector frags = absl::StrSplit(dotted_path, '.'); + for (std::string& frag : frags) { + if (!frag.empty() && isupper(frag[0])) { + frag = std::regex_replace(frag, std::regex("_"), "."); + } + } + if (absl::EndsWith(frags.back(), "Case")) { + frags.pop_back(); + } + return absl::StrJoin(frags, "."); + } + + // Convert from a protobuf type, e.g. foo.bar.v2, to a C++ type, e.g. + // foo::bar::v2. + static std::string protoToCxxType(const std::string& proto_type_name) { + // TODO(htuch): add support for recovering foo::bar::Baz_Blah from foo.bar.Baz.Blah. + return std::regex_replace(proto_type_name, std::regex(R"(\.)"), "::"); + } +}; + +} // namespace ApiBooster diff --git a/tools/clang_tools/api_booster/proto_cxx_utils_test.cc b/tools/clang_tools/api_booster/proto_cxx_utils_test.cc new file mode 100644 index 000000000000..19a9a2b77662 --- /dev/null +++ b/tools/clang_tools/api_booster/proto_cxx_utils_test.cc @@ -0,0 +1,24 @@ +#include "gtest/gtest.h" +#include "proto_cxx_utils.h" + +namespace ApiBooster { +namespace { + +// Validate C++ to proto type name conversion. +TEST(ProtoCxxUtils, CxxToProtoType) { + EXPECT_EQ("", ProtoCxxUtils::cxxToProtoType("")); + EXPECT_EQ("foo", ProtoCxxUtils::cxxToProtoType("foo")); + EXPECT_EQ("foo.bar", ProtoCxxUtils::cxxToProtoType("foo::bar")); + EXPECT_EQ("foo.bar", ProtoCxxUtils::cxxToProtoType("foo::bar::FooCase")); + EXPECT_EQ("foo.bar.Baz.Blah", ProtoCxxUtils::cxxToProtoType("foo::bar::Baz_Blah")); +} + +// Validate proto to C++ type name conversion. +TEST(ProtoCxxUtils, ProtoToCxxType) { + EXPECT_EQ("", ProtoCxxUtils::protoToCxxType("")); + EXPECT_EQ("foo", ProtoCxxUtils::protoToCxxType("foo")); + EXPECT_EQ("foo::bar", ProtoCxxUtils::protoToCxxType("foo.bar")); +} + +} // namespace +} // namespace ApiBooster diff --git a/tools/clang_tools/support/clang_tools.bzl b/tools/clang_tools/support/clang_tools.bzl index b8c2cd41b8ab..67336a35e430 100644 --- a/tools/clang_tools/support/clang_tools.bzl +++ b/tools/clang_tools/support/clang_tools.bzl @@ -1,4 +1,4 @@ -def envoy_clang_tools_cc_binary(name, copts = [], tags = [], **kwargs): +def clang_tools_cc_binary(name, copts = [], tags = [], **kwargs): native.cc_binary( name = name, copts = copts + [ @@ -8,3 +8,16 @@ def envoy_clang_tools_cc_binary(name, copts = [], tags = [], **kwargs): tags = tags + ["manual"], **kwargs ) + +def clang_tools_cc_library(name, **kwargs): + native.cc_library( + name = name, + **kwargs + ) + +def clang_tools_cc_test(name, deps = [], **kwargs): + native.cc_test( + name = name, + deps = deps + ["@com_google_googletest//:gtest_main"], + **kwargs + ) diff --git a/tools/type_whisperer/api_type_db.cc b/tools/type_whisperer/api_type_db.cc index dce28eadffd4..bd5f5932011f 100644 --- a/tools/type_whisperer/api_type_db.cc +++ b/tools/type_whisperer/api_type_db.cc @@ -27,12 +27,17 @@ const tools::type_whisperer::TypeDb& getApiTypeDb() { } // namespace -absl::optional ApiTypeDb::getProtoPathForType(const std::string& type_name) { - auto it = getApiTypeDb().types().find(type_name); - if (it == getApiTypeDb().types().end()) { - return absl::nullopt; +absl::optional ApiTypeDb::getLatestTypeInformation(const std::string& type_name) { + absl::optional result; + std::string current_type_name = type_name; + while (true) { + auto it = getApiTypeDb().types().find(current_type_name); + if (it == getApiTypeDb().types().end()) { + return result; + } + result.emplace(current_type_name, it->second.proto_path()); + current_type_name = it->second.next_version_type_name(); } - return it->second.proto_path(); } } // namespace TypeWhisperer diff --git a/tools/type_whisperer/api_type_db.h b/tools/type_whisperer/api_type_db.h index 34d4c2808e46..a662ea4e7524 100644 --- a/tools/type_whisperer/api_type_db.h +++ b/tools/type_whisperer/api_type_db.h @@ -7,12 +7,24 @@ namespace Envoy { namespace Tools { namespace TypeWhisperer { +// C++ representation of TypeDbDescription. +struct TypeInformation { + TypeInformation(absl::string_view type_name, absl::string_view proto_path) + : type_name_(type_name), proto_path_(proto_path) {} + + // Type's name in the next major version of the API. + const std::string type_name_; + + // Path to .proto from API root. + const std::string proto_path_; +}; + // We don't expose the raw API type database to consumers, as this requires RTTI // and this may be linked in environments where this is not available (e.g. // libtooling binaries). class ApiTypeDb { public: - static absl::optional getProtoPathForType(const std::string& type_name); + static absl::optional getLatestTypeInformation(const std::string& type_name); }; } // namespace TypeWhisperer From 72252cb398cbe4ff75d4b3f6c0a4f88cf659fef2 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 16 Dec 2019 21:43:47 -0500 Subject: [PATCH 2/4] Cleanup only *.clang-replacements.yaml instead of *.yaml. Signed-off-by: Harvey Tuch --- tools/api_boost/api_boost.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/api_boost/api_boost.py b/tools/api_boost/api_boost.py index 30d447a97946..c6afeedbd783 100755 --- a/tools/api_boost/api_boost.py +++ b/tools/api_boost/api_boost.py @@ -163,7 +163,8 @@ def ApiBoostTree(target_paths, finally: # Cleanup any stray **/*.clang-replacements.yaml. for prefix in target_paths: - clang_replacements = pathlib.Path(PrefixDirectory(prefix)).glob('**/*.yaml') + clang_replacements = pathlib.Path( + PrefixDirectory(prefix)).glob('**/*.clang-replacements.yaml') for path in clang_replacements: path.unlink() From d55e051bda2ad591f3f11dfc855edb937b60ef4c Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 18 Dec 2019 12:07:41 -0500 Subject: [PATCH 3/4] Some TODOs. Signed-off-by: Harvey Tuch --- tools/api_boost/api_boost.py | 2 ++ tools/clang_tools/api_booster/proto_cxx_utils.h | 2 ++ 2 files changed, 4 insertions(+) diff --git a/tools/api_boost/api_boost.py b/tools/api_boost/api_boost.py index c6afeedbd783..d2e03ca12316 100755 --- a/tools/api_boost/api_boost.py +++ b/tools/api_boost/api_boost.py @@ -152,6 +152,8 @@ def ApiBoostTree(target_paths, # in one thread on consumed transitive headers on the other thread isn't an # issue. This also ensures that we complete all analysis error free before # any mutation takes place. + # TODO(htuch): we should move to run-clang-tidy.py once the headers fixups + # are Clang-based. api_includes = p.map(functools.partial(ApiBoostFile, llvm_include_path, debug_log), file_paths) # Apply Clang replacements before header fixups, since the replacements diff --git a/tools/clang_tools/api_booster/proto_cxx_utils.h b/tools/clang_tools/api_booster/proto_cxx_utils.h index 26f1f8aaf912..ea7774eef610 100644 --- a/tools/clang_tools/api_booster/proto_cxx_utils.h +++ b/tools/clang_tools/api_booster/proto_cxx_utils.h @@ -10,6 +10,8 @@ namespace ApiBooster { // Protobuf C++ code generation hackery. This is where the utilities that map // between C++ and protobuf types, enum constants and identifiers live. Most of // this is heuristic and needs to match whatever the protobuf compiler does. +// TODO(htuch): investigate what can be done to make use of embedded proto +// descriptors in generated stubs to make these utils more robust. class ProtoCxxUtils { public: // Convert from a C++ type, e.g. foo::bar::v2, to a protobuf type, e.g. From 8994eccc17d2057b5512ba5058325cdae407d854 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 18 Dec 2019 16:57:22 -0500 Subject: [PATCH 4/4] Review feedback. Signed-off-by: Harvey Tuch --- tools/api_boost/api_boost.py | 3 --- tools/clang_tools/api_booster/main.cc | 6 +++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/tools/api_boost/api_boost.py b/tools/api_boost/api_boost.py index d2e03ca12316..9ecd2650c2e1 100755 --- a/tools/api_boost/api_boost.py +++ b/tools/api_boost/api_boost.py @@ -19,9 +19,6 @@ import re import subprocess as sp -# Temporary location of modified files. -TMP_SWP_SUFFIX = '.tmp.swp' - # Detect API #includes. API_INCLUDE_REGEX = re.compile('#include "(envoy/.*)/[^/]+\.pb\.(validate\.)?h"') diff --git a/tools/clang_tools/api_booster/main.cc b/tools/clang_tools/api_booster/main.cc index 7a24c8bb7a46..30df4978302f 100644 --- a/tools/clang_tools/api_booster/main.cc +++ b/tools/clang_tools/api_booster/main.cc @@ -214,9 +214,9 @@ int main(int argc, const char** argv) { // only ones displayed. llvm::cl::OptionCategory api_booster_tool_category("api-booster options"); - clang::tooling::CommonOptionsParser optionsParser(argc, argv, api_booster_tool_category); - clang::tooling::RefactoringTool tool(optionsParser.getCompilations(), - optionsParser.getSourcePathList()); + clang::tooling::CommonOptionsParser options_parser(argc, argv, api_booster_tool_category); + clang::tooling::RefactoringTool tool(options_parser.getCompilations(), + options_parser.getSourcePathList()); ApiBooster::ApiBooster api_booster(tool.getReplacements()); clang::ast_matchers::MatchFinder finder;