From 6e74b1448bf8d94d73ef3134aa9e8bf36e51a2aa Mon Sep 17 00:00:00 2001 From: Maksim Denisov Date: Wed, 2 Oct 2024 16:03:45 +0200 Subject: Enable bugprone-exception-escape check --- .clang-tidy | 2 +- .../analysed_target/target_graph_information.cpp | 2 +- .../analysed_target/target_graph_information.hpp | 2 +- .../build_engine/expression/expression_ptr.cpp | 2 +- .../build_engine/expression/expression_ptr.hpp | 2 +- .../build_engine/target_map/result_map.hpp | 3 +- src/buildtool/common/action_description.hpp | 2 +- src/buildtool/common/artifact.hpp | 2 +- src/buildtool/common/artifact_description.cpp | 64 +++++++++------------- src/buildtool/common/artifact_description.hpp | 2 +- .../execution_api/common/tree_reader_utils.cpp | 29 ++++++---- src/buildtool/execution_api/git/git_api.hpp | 61 ++++++++++----------- .../execution_engine/executor/executor.hpp | 2 +- src/buildtool/serve_api/remote/target_client.cpp | 16 +++--- src/buildtool/storage/backend_description.cpp | 15 ++++- src/other_tools/just_mr/setup_utils.cpp | 2 +- src/other_tools/just_mr/setup_utils.hpp | 2 +- 17 files changed, 106 insertions(+), 104 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index ca95fb99..1c167b8d 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -2,7 +2,7 @@ FormatStyle: Google Checks: >- *,-abseil-*,-altera-*,-android-*,-boost-*,-cert-*,-darwin-*,-fuchsia-*,-linuxkernel-*,-llvm-*,-llvmlibc-*,-mpi-*,-objc-*,-zircon-* WarningsAsErrors: >- - bugprone-*,-bugprone-easily-swappable-parameters,-bugprone-unchecked-optional-access,-bugprone-empty-catch,-bugprone-exception-escape, + bugprone-*,-bugprone-easily-swappable-parameters,-bugprone-unchecked-optional-access,-bugprone-empty-catch, clang-analyzer-*,-clang-analyzer-cplusplus.NewDeleteLeaks,-clang-analyzer-cplusplus.StringChecker, clang-diagnostic-*,-clang-diagnostic-unused-command-line-argument, hicpp-*, diff --git a/src/buildtool/build_engine/analysed_target/target_graph_information.cpp b/src/buildtool/build_engine/analysed_target/target_graph_information.cpp index dfd333ac..24ba3090 100644 --- a/src/buildtool/build_engine/analysed_target/target_graph_information.cpp +++ b/src/buildtool/build_engine/analysed_target/target_graph_information.cpp @@ -37,7 +37,7 @@ auto NodesToString(std::vector const& } // namespace -auto TargetGraphInformation::DepsToJson() const noexcept -> nlohmann::json { +auto TargetGraphInformation::DepsToJson() const -> nlohmann::json { auto result = nlohmann::json::object(); result["declared"] = NodesToString(direct_); result["implicit"] = NodesToString(implicit_); diff --git a/src/buildtool/build_engine/analysed_target/target_graph_information.hpp b/src/buildtool/build_engine/analysed_target/target_graph_information.hpp index 7b94a91a..f07dfd00 100644 --- a/src/buildtool/build_engine/analysed_target/target_graph_information.hpp +++ b/src/buildtool/build_engine/analysed_target/target_graph_information.hpp @@ -44,7 +44,7 @@ class TargetGraphInformation { [[nodiscard]] auto NodeString() const noexcept -> std::optional; - [[nodiscard]] auto DepsToJson() const noexcept -> nlohmann::json; + [[nodiscard]] auto DepsToJson() const -> nlohmann::json; private: BuildMaps::Target::ConfiguredTargetPtr node_; diff --git a/src/buildtool/build_engine/expression/expression_ptr.cpp b/src/buildtool/build_engine/expression/expression_ptr.cpp index 68b45c6f..0d3ce649 100644 --- a/src/buildtool/build_engine/expression/expression_ptr.cpp +++ b/src/buildtool/build_engine/expression/expression_ptr.cpp @@ -76,7 +76,7 @@ auto ExpressionPtr::ToIdentifier() const noexcept -> std::string { return ptr_ ? ptr_->ToIdentifier() : std::string{}; } -auto ExpressionPtr::ToJson() const noexcept -> nlohmann::json { +auto ExpressionPtr::ToJson() const -> nlohmann::json { return ptr_ ? ptr_->ToJson() : nlohmann::json::object(); } diff --git a/src/buildtool/build_engine/expression/expression_ptr.hpp b/src/buildtool/build_engine/expression/expression_ptr.hpp index b1f698c6..c1ab6066 100644 --- a/src/buildtool/build_engine/expression/expression_ptr.hpp +++ b/src/buildtool/build_engine/expression/expression_ptr.hpp @@ -90,7 +90,7 @@ class ExpressionPtr { [[nodiscard]] auto IsCacheable() const noexcept -> bool; [[nodiscard]] auto ToIdentifier() const noexcept -> std::string; - [[nodiscard]] auto ToJson() const noexcept -> nlohmann::json; + [[nodiscard]] auto ToJson() const -> nlohmann::json; using linked_map_t = LinkedMap; [[nodiscard]] auto IsNotNull() const noexcept -> bool; diff --git a/src/buildtool/build_engine/target_map/result_map.hpp b/src/buildtool/build_engine/target_map/result_map.hpp index 9d7787bc..b73cdd48 100644 --- a/src/buildtool/build_engine/target_map/result_map.hpp +++ b/src/buildtool/build_engine/target_map/result_map.hpp @@ -131,8 +131,7 @@ class ResultTargetMap { return all_exports; } - [[nodiscard]] auto ConfiguredTargetsGraph() const noexcept - -> nlohmann::json { + [[nodiscard]] auto ConfiguredTargetsGraph() const -> nlohmann::json { auto result = nlohmann::json::object(); for (auto const& i : targets_) { for (auto const& it : i) { diff --git a/src/buildtool/common/action_description.hpp b/src/buildtool/common/action_description.hpp index f4756688..e51552ad 100644 --- a/src/buildtool/common/action_description.hpp +++ b/src/buildtool/common/action_description.hpp @@ -196,7 +196,7 @@ class ActionDescription { return action_.Id(); } - [[nodiscard]] auto ToJson() const noexcept -> nlohmann::json { + [[nodiscard]] auto ToJson() const -> nlohmann::json { auto json = nlohmann::json{{"command", action_.Command()}}; if (not output_files_.empty()) { json["output"] = output_files_; diff --git a/src/buildtool/common/artifact.hpp b/src/buildtool/common/artifact.hpp index a933433e..1ef169d3 100644 --- a/src/buildtool/common/artifact.hpp +++ b/src/buildtool/common/artifact.hpp @@ -64,7 +64,7 @@ class Artifact { // Create JSON of the form '{"id": "hash", "size": x, "file_type": "f"}' // As the failed property is only internal to a run, discard it. - [[nodiscard]] auto ToJson() const noexcept -> nlohmann::json { + [[nodiscard]] auto ToJson() const -> nlohmann::json { return {{"id", digest.hash()}, {"size", digest.size()}, {"file_type", std::string{ToChar(type)}}}; diff --git a/src/buildtool/common/artifact_description.cpp b/src/buildtool/common/artifact_description.cpp index e5a67155..b1451491 100644 --- a/src/buildtool/common/artifact_description.cpp +++ b/src/buildtool/common/artifact_description.cpp @@ -24,19 +24,19 @@ namespace { [[nodiscard]] auto DescribeLocalArtifact(std::filesystem::path const& src_path, - std::string const& repository) noexcept + std::string const& repository) -> nlohmann::json; -[[nodiscard]] auto DescribeKnownArtifact( - std::string const& blob_id, - std::size_t size, - ObjectType type = ObjectType::File) noexcept -> nlohmann::json; +[[nodiscard]] auto DescribeKnownArtifact(std::string const& blob_id, + std::size_t size, + ObjectType type = ObjectType::File) + -> nlohmann::json; [[nodiscard]] auto DescribeActionArtifact(std::string const& action_id, - std::string const& out_path) noexcept + std::string const& out_path) -> nlohmann::json; -[[nodiscard]] auto DescribeTreeArtifact(std::string const& tree_id) noexcept +[[nodiscard]] auto DescribeTreeArtifact(std::string const& tree_id) -> nlohmann::json; [[nodiscard]] auto CreateLocalArtifactDescription(nlohmann::json const& data) @@ -129,30 +129,23 @@ auto ArtifactDescription::FromJson(HashFunction::Type hash_type, return std::nullopt; } -auto ArtifactDescription::ToJson() const noexcept -> nlohmann::json { - try { - if (std::holds_alternative(data_)) { - auto const& [path, repo] = std::get(data_); - return DescribeLocalArtifact(path.string(), repo); - } - if (std::holds_alternative(data_)) { - auto const& [digest, file_type, _] = std::get(data_); - return DescribeKnownArtifact( - digest.hash(), digest.size(), file_type); - } - if (std::holds_alternative(data_)) { - auto const& [action_id, path] = std::get(data_); - return DescribeActionArtifact(action_id, path); - } - if (std::holds_alternative(data_)) { - return DescribeTreeArtifact(std::get(data_)); - } - Logger::Log(LogLevel::Error, "Internal error, unknown artifact type"); - } catch (std::exception const& ex) { - Logger::Log(LogLevel::Error, - "Serializing to JSON failed with error:\n{}", - ex.what()); +auto ArtifactDescription::ToJson() const -> nlohmann::json { + if (std::holds_alternative(data_)) { + auto const& [path, repo] = std::get(data_); + return DescribeLocalArtifact(path.string(), repo); + } + if (std::holds_alternative(data_)) { + auto const& [digest, file_type, _] = std::get(data_); + return DescribeKnownArtifact(digest.hash(), digest.size(), file_type); + } + if (std::holds_alternative(data_)) { + auto const& [action_id, path] = std::get(data_); + return DescribeActionArtifact(action_id, path); + } + if (std::holds_alternative(data_)) { + return DescribeTreeArtifact(std::get(data_)); } + Logger::Log(LogLevel::Error, "Internal error, unknown artifact type"); Ensures(false); // unreachable return {}; } @@ -210,8 +203,7 @@ auto ArtifactDescription::ComputeId(nlohmann::json const& desc) noexcept namespace { auto DescribeLocalArtifact(std::filesystem::path const& src_path, - std::string const& repository) noexcept - -> nlohmann::json { + std::string const& repository) -> nlohmann::json { return { {"type", "LOCAL"}, {"data", {{"path", src_path.string()}, {"repository", repository}}}}; @@ -219,7 +211,7 @@ auto DescribeLocalArtifact(std::filesystem::path const& src_path, auto DescribeKnownArtifact(std::string const& blob_id, std::size_t size, - ObjectType type) noexcept -> nlohmann::json { + ObjectType type) -> nlohmann::json { std::string const typestr{ToChar(type)}; return { {"type", "KNOWN"}, @@ -227,14 +219,12 @@ auto DescribeKnownArtifact(std::string const& blob_id, } auto DescribeActionArtifact(std::string const& action_id, - std::string const& out_path) noexcept - -> nlohmann::json { + std::string const& out_path) -> nlohmann::json { return {{"type", "ACTION"}, {"data", {{"id", action_id}, {"path", out_path}}}}; } -auto DescribeTreeArtifact(std::string const& tree_id) noexcept - -> nlohmann::json { +auto DescribeTreeArtifact(std::string const& tree_id) -> nlohmann::json { return {{"type", "TREE"}, {"data", {{"id", tree_id}}}}; } diff --git a/src/buildtool/common/artifact_description.hpp b/src/buildtool/common/artifact_description.hpp index c63a2dec..1858e41e 100644 --- a/src/buildtool/common/artifact_description.hpp +++ b/src/buildtool/common/artifact_description.hpp @@ -70,7 +70,7 @@ class ArtifactDescription final { nlohmann::json const& json) noexcept -> std::optional; - [[nodiscard]] auto ToJson() const noexcept -> nlohmann::json; + [[nodiscard]] auto ToJson() const -> nlohmann::json; [[nodiscard]] auto ToArtifact() const noexcept -> Artifact; diff --git a/src/buildtool/execution_api/common/tree_reader_utils.cpp b/src/buildtool/execution_api/common/tree_reader_utils.cpp index a04b9300..5a55c059 100644 --- a/src/buildtool/execution_api/common/tree_reader_utils.cpp +++ b/src/buildtool/execution_api/common/tree_reader_utils.cpp @@ -64,7 +64,7 @@ namespace { } template -[[nodiscard]] auto TreeToString(TTree const& entries) noexcept +[[nodiscard]] auto TreeToString(TTree const& entries) -> std::optional { auto json = nlohmann::json::object(); TreeReaderUtils::InfoStoreFunc store_infos = @@ -78,14 +78,7 @@ template }; if (TreeReaderUtils::ReadObjectInfos(entries, store_infos)) { - try { - return json.dump(2) + "\n"; - } catch (std::exception const& ex) { - Logger::Log(LogLevel::Error, - "dumping Directory to string failed with:\n{}", - ex.what()); - return std::nullopt; - } + return json.dump(2) + "\n"; } Logger::Log(LogLevel::Error, "reading object infos from Directory failed"); return std::nullopt; @@ -164,11 +157,25 @@ auto TreeReaderUtils::ReadObjectInfos(GitRepo::tree_entries_t const& entries, auto TreeReaderUtils::DirectoryToString(bazel_re::Directory const& dir) noexcept -> std::optional { - return TreeToString(dir); + try { + return TreeToString(dir); + } catch (const std::exception& e) { + Logger::Log(LogLevel::Error, + "An error occurred while reading bazel:re::Directory:\n", + e.what()); + return std::nullopt; + } } auto TreeReaderUtils::GitTreeToString( GitRepo::tree_entries_t const& entries) noexcept -> std::optional { - return TreeToString(entries); + try { + return TreeToString(entries); + } catch (const std::exception& e) { + Logger::Log(LogLevel::Error, + "An error occurred while reading git tree:\n{}", + e.what()); + return std::nullopt; + } } diff --git a/src/buildtool/execution_api/git/git_api.hpp b/src/buildtool/execution_api/git/git_api.hpp index ed28f237..1c7edca0 100644 --- a/src/buildtool/execution_api/git/git_api.hpp +++ b/src/buildtool/execution_api/git/git_api.hpp @@ -115,8 +115,9 @@ class GitApi final : public IExecutionApi { return false; } for (std::size_t i{}; i < artifacts_info.size(); ++i) { - auto fd = fds[i]; auto const& info = artifacts_info[i]; + + std::string content; if (IsTreeObject(info.type) and not raw_tree) { auto tree = repo_config_->ReadTreeFromGitCAS(info.digest.hash()); @@ -126,27 +127,22 @@ class GitApi final : public IExecutionApi { info.digest.hash()); return false; } - auto json = nlohmann::json::object(); - for (auto const& [path, entry] : *tree) { - auto digest = ToArtifactDigest(*entry); - if (not digest) { - return false; + + try { + auto json = nlohmann::json::object(); + for (auto const& [path, entry] : *tree) { + auto digest = ToArtifactDigest(*entry); + if (not digest) { + return false; + } + json[path] = + Artifact::ObjectInfo{.digest = *std::move(digest), + .type = entry->Type(), + .failed = false} + .ToString(/*size_unknown*/ true); } - json[path] = - Artifact::ObjectInfo{.digest = *std::move(digest), - .type = entry->Type(), - .failed = false} - .ToString(/*size_unknown*/ true); - } - auto msg = json.dump(2) + "\n"; - if (gsl::owner out = fdopen(fd, "wb")) { // NOLINT - std::fwrite(msg.data(), 1, msg.size(), out); - std::fclose(out); - } - else { - Logger::Log(LogLevel::Error, - "dumping to file descriptor {} failed.", - fd); + content = json.dump(2) + "\n"; + } catch (...) { return false; } } @@ -159,17 +155,18 @@ class GitApi final : public IExecutionApi { info.digest.hash()); return false; } - auto msg = *blob; - if (gsl::owner out = fdopen(fd, "wb")) { // NOLINT - std::fwrite(msg.data(), 1, msg.size(), out); - std::fclose(out); - } - else { - Logger::Log(LogLevel::Error, - "dumping to file descriptor {} failed.", - fd); - return false; - } + content = *std::move(blob); + } + + if (gsl::owner out = fdopen(fds[i], "wb")) { // NOLINT + std::fwrite(content.data(), 1, content.size(), out); + std::fclose(out); + } + else { + Logger::Log(LogLevel::Error, + "dumping to file descriptor {} failed.", + fds[i]); + return false; } } return true; diff --git a/src/buildtool/execution_engine/executor/executor.hpp b/src/buildtool/execution_engine/executor/executor.hpp index 0018cafc..3a416cfb 100644 --- a/src/buildtool/execution_engine/executor/executor.hpp +++ b/src/buildtool/execution_engine/executor/executor.hpp @@ -934,7 +934,7 @@ class Rebuilder { logger, artifact, context_.repo_config, *context_.apis); } - [[nodiscard]] auto DumpFlakyActions() const noexcept -> nlohmann::json { + [[nodiscard]] auto DumpFlakyActions() const -> nlohmann::json { std::unique_lock lock{m_}; auto actions = nlohmann::json::object(); for (auto const& [action_id, outputs] : flaky_actions_) { diff --git a/src/buildtool/serve_api/remote/target_client.cpp b/src/buildtool/serve_api/remote/target_client.cpp index f339172f..972b9708 100644 --- a/src/buildtool/serve_api/remote/target_client.cpp +++ b/src/buildtool/serve_api/remote/target_client.cpp @@ -91,14 +91,16 @@ auto TargetClient::ServeTarget(const TargetCacheKey& key, // add dispatch information to request, while ensuring blob is uploaded // to remote cas - auto dispatch_list = nlohmann::json::array(); + std::optional dispatch_digest; try { + auto dispatch_list = nlohmann::json::array(); for (auto const& [props, endpoint] : exec_config_.dispatch) { auto entry = nlohmann::json::array(); entry.push_back(nlohmann::json(props)); entry.push_back(endpoint.ToJson()); dispatch_list.push_back(entry); } + dispatch_digest = storage_.CAS().StoreBlob(dispatch_list.dump(2)); } catch (std::exception const& ex) { return serve_target_result_t{ std::in_place_index<1>, @@ -106,14 +108,12 @@ auto TargetClient::ServeTarget(const TargetCacheKey& key, ex.what())}; } - auto dispatch_dgst = storage_.CAS().StoreBlob(dispatch_list.dump(2)); - if (not dispatch_dgst) { + if (not dispatch_digest) { return serve_target_result_t{ - std::in_place_index<1>, - fmt::format("Failed to store blob {} to local cas", - dispatch_list.dump(2))}; + std::in_place_index<1>, "Failed to add dispatch info to local cas"}; } - auto const dispatch_info = Artifact::ObjectInfo{.digest = *dispatch_dgst, + + auto const dispatch_info = Artifact::ObjectInfo{.digest = *dispatch_digest, .type = ObjectType::File}; if (not apis_.local->RetrieveToCas({dispatch_info}, *apis_.remote)) { return serve_target_result_t{ @@ -122,7 +122,7 @@ auto TargetClient::ServeTarget(const TargetCacheKey& key, dispatch_info.ToString())}; } (*request.mutable_dispatch_info()) = - ArtifactDigestFactory::ToBazel(*dispatch_dgst); + ArtifactDigestFactory::ToBazel(*dispatch_digest); // call rpc grpc::ClientContext context; diff --git a/src/buildtool/storage/backend_description.cpp b/src/buildtool/storage/backend_description.cpp index 456f79c6..c5f790aa 100644 --- a/src/buildtool/storage/backend_description.cpp +++ b/src/buildtool/storage/backend_description.cpp @@ -23,9 +23,18 @@ auto DescribeBackend(std::optional const& address, ExecutionProperties const& properties, std::vector const& dispatch) noexcept -> expected { - auto description = nlohmann::json{ - {"remote_address", address ? address->ToJson() : nlohmann::json{}}, - {"platform_properties", properties}}; + nlohmann::json description; + try { + description["remote_address"] = + address ? address->ToJson() : nlohmann::json{}; + description["platform_properties"] = properties; + } catch (std::exception const& e) { + return unexpected{ + fmt::format("Failed to serialize remote address and " + "platform_properties:\n{}", + e.what())}; + } + if (not dispatch.empty()) { try { // only add the dispatch list, if not empty, so that keys remain diff --git a/src/other_tools/just_mr/setup_utils.cpp b/src/other_tools/just_mr/setup_utils.cpp index 4aa710c5..777343f5 100644 --- a/src/other_tools/just_mr/setup_utils.cpp +++ b/src/other_tools/just_mr/setup_utils.cpp @@ -126,7 +126,7 @@ void DefaultReachableRepositories( auto ReadConfiguration( std::optional const& config_file_opt, - std::optional const& absent_file_opt) noexcept + std::optional const& absent_file_opt) -> std::shared_ptr { if (not config_file_opt) { Logger::Log(LogLevel::Error, "Cannot find repository configuration."); diff --git a/src/other_tools/just_mr/setup_utils.hpp b/src/other_tools/just_mr/setup_utils.hpp index e29df4a6..c96db5a3 100644 --- a/src/other_tools/just_mr/setup_utils.hpp +++ b/src/other_tools/just_mr/setup_utils.hpp @@ -77,7 +77,7 @@ void DefaultReachableRepositories( /// \brief Read in a just-mr configuration file. [[nodiscard]] auto ReadConfiguration( std::optional const& config_file_opt, - std::optional const& absent_file_opt) noexcept + std::optional const& absent_file_opt) -> std::shared_ptr; [[nodiscard]] auto CreateAuthConfig( -- cgit v1.2.3