From f40c9cdf1d6e60932ec69f4a00787bb3190f9c15 Mon Sep 17 00:00:00 2001 From: wechman Date: Mon, 30 May 2022 07:56:18 +0200 Subject: [PATCH 1/5] Update changelog with information about changes to CommonSubexpressionEliminator --- Changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/Changelog.md b/Changelog.md index b37938172..4151d533a 100644 --- a/Changelog.md +++ b/Changelog.md @@ -12,6 +12,7 @@ Compiler Features: Bugfixes: * ABI Encoder: When encoding an empty string coming from storage do not add a superfluous empty slot for data. + * Common Subexpression Eliminator: Process assembly items in chunks with maximum size of 2000. It helps to avoid extremely time-consuming searches during code optimization. * Yul Optimizer: Do not remove ``returndatacopy`` in cases in which it might perform out-of-bounds reads that unconditionally revert as out-of-gas. Previously, any ``returndatacopy`` that wrote to memory that was never read from was removed without accounting for the out-of-bounds condition. From 2cea70c04fb327da0d2b718dd01f4e0ebbb1bac5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20=C5=9Aliwak?= Date: Mon, 6 Jun 2022 20:26:57 +0200 Subject: [PATCH 2/5] Skip external test benchmark diff instead of failing when previous run of the job did not succeed --- .circleci/config.yml | 9 +++- scripts/common/rest_api_helpers.py | 30 +++++++++--- scripts/externalTests/download_benchmarks.py | 48 ++++++++++++++------ 3 files changed, 66 insertions(+), 21 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index b637b5805..f99c28ee9 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1257,12 +1257,17 @@ jobs: pr_id=$(echo "$CIRCLE_PULL_REQUEST" | sed 's|\(.*\)\/||') scripts_dir=../../../scripts - "${scripts_dir}/externalTests/download_benchmarks.py" --base-of-pr "$pr_id" + # Our main goal here is to provide new benchmarks, the diff is optional. When benchmarks from + # the previous run are not available for whatever reason, we still succeed and just skip the diff. + # download_benchmarks.py exits with status 2 in that case. + if "${scripts_dir}/externalTests/download_benchmarks.py" --base-of-pr "$pr_id" || [[ $? == 2 ]]; then + echo 'export SKIP_BENCHMARK_DIFF=true' >> $BASH_ENV + fi fi - run: name: Diff benchmarks command: | - if [[ $CIRCLE_PULL_REQUEST != "" ]]; then + if [[ $CIRCLE_PULL_REQUEST != "" && $SKIP_BENCHMARK_DIFF != "true" ]]; then cd reports/externalTests/ mkdir diff/ scripts_dir=../../scripts diff --git a/scripts/common/rest_api_helpers.py b/scripts/common/rest_api_helpers.py index 6dc3d1326..4068a5e1e 100644 --- a/scripts/common/rest_api_helpers.py +++ b/scripts/common/rest_api_helpers.py @@ -11,7 +11,28 @@ import requests class APIHelperError(Exception): pass -class DataUnavailable(APIHelperError): +class JobNotSuccessful(APIHelperError): + def __init__(self, name: str, status: str): + assert status != 'success' + + self.name = name + self.status = status + self.job_finished = (status in ['failed', 'blocked']) + + if status == 'not_running': + message = f"Job {name} has not started yet." + elif status == 'blocked': + message = f"Job {name} will not run because one of its dependencies failed." + elif status == 'running': + message = f"Job {name} is still running." + elif status == 'failed': + message = f"Job {name} failed." + else: + message = f"Job {name} did not finish successfully. Current status: {status}." + + super().__init__(message) + +class JobMissing(APIHelperError): pass class InvalidResponse(APIHelperError): @@ -145,13 +166,10 @@ class CircleCI: def job(self, workflow_id: str, name: str, require_success: bool = False) -> dict: jobs = self.jobs(workflow_id) if name not in jobs: - raise DataUnavailable(f"Job {name} is not present in the workflow.") + raise JobMissing(f"Job {name} is not present in the workflow.") if require_success and jobs[name]['status'] != 'success': - raise DataUnavailable( - f"Job {name} has failed or is still running. " - f"Current status: {jobs[name]['status']}." - ) + raise JobNotSuccessful(name, jobs[name]['status']) return jobs[name] diff --git a/scripts/externalTests/download_benchmarks.py b/scripts/externalTests/download_benchmarks.py index 4ac7c0c4d..29f5fe4bc 100755 --- a/scripts/externalTests/download_benchmarks.py +++ b/scripts/externalTests/download_benchmarks.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 from argparse import ArgumentParser, Namespace +from enum import Enum, unique from pathlib import Path from typing import Mapping, Optional import sys @@ -13,10 +14,18 @@ SCRIPTS_DIR = Path(__file__).parent.parent sys.path.insert(0, str(SCRIPTS_DIR)) from common.git_helpers import git_current_branch, git_commit_hash -from common.rest_api_helpers import APIHelperError, CircleCI, Github, download_file +from common.rest_api_helpers import APIHelperError, JobNotSuccessful, CircleCI, Github, download_file # pragma pylint: enable=import-error,wrong-import-position +@unique +class Status(Enum): + OK = 0 # Benchmarks downloaded successfully + ERROR = 1 # Error in the script, bad API response, unexpected data, etc. + NO_BENCHMARK = 2 # Benchmark collector job did not finish successfully and/or benchmark artifacts are missing. + PENDING = 3 # Benchmark collector job has not finished yet. + + def process_commandline() -> Namespace: script_description = ( "Downloads benchmark results attached as artifacts to the c_ext_benchmarks job on CircleCI. " @@ -76,14 +85,16 @@ def download_benchmark_artifact( commit_hash: str, overwrite: bool, silent: bool = False -): +) -> bool: if not silent: print(f"Downloading artifact: {benchmark_name}-{branch}-{commit_hash[:8]}.json.") artifact_path = f'reports/externalTests/{benchmark_name}.json' if artifact_path not in artifacts: - raise RuntimeError(f"Missing artifact: {artifact_path}.") + if not silent: + print(f"Missing artifact: {artifact_path}.") + return False download_file( artifacts[artifact_path]['url'], @@ -91,6 +102,8 @@ def download_benchmark_artifact( overwrite, ) + return True + def download_benchmarks( branch: Optional[str], @@ -100,7 +113,7 @@ def download_benchmarks( overwrite: bool = False, debug_requests: bool = False, silent: bool = False, -): +) -> Status: github = Github('ethereum/solidity', debug_requests) circleci = CircleCI('ethereum/solidity', debug_requests) @@ -141,32 +154,41 @@ def download_benchmarks( artifacts = circleci.artifacts(int(benchmark_collector_job['job_number'])) - download_benchmark_artifact(artifacts, 'summarized-benchmarks', branch, actual_commit_hash, overwrite, silent) - download_benchmark_artifact(artifacts, 'all-benchmarks', branch, actual_commit_hash, overwrite, silent) + got_summary = download_benchmark_artifact(artifacts, 'summarized-benchmarks', branch, actual_commit_hash, overwrite, silent) + got_full = download_benchmark_artifact(artifacts, 'all-benchmarks', branch, actual_commit_hash, overwrite, silent) + + return Status.OK if got_summary and got_full else Status.NO_BENCHMARK def main(): try: options = process_commandline() - download_benchmarks( + return download_benchmarks( options.branch, options.pull_request_id, options.base_of_pr, options.ignore_commit_hash, options.overwrite, options.debug_requests, - ) - - return 0 + ).value + except JobNotSuccessful as exception: + print(f"[ERROR] {exception}", file=sys.stderr) + if not exception.job_finished: + print("Please wait for the workflow to finish and try again.", file=sys.stderr) + return Status.PENDING.value + else: + print("Benchmarks from this run of the pipeline are not available.", file=sys.stderr) + return Status.NO_BENCHMARK.value except APIHelperError as exception: print(f"[ERROR] {exception}", file=sys.stderr) - return 1 + return Status.ERROR.value except requests.exceptions.HTTPError as exception: print(f"[ERROR] {exception}", file=sys.stderr) - return 1 + return Status.ERROR.value except RuntimeError as exception: print(f"[ERROR] {exception}", file=sys.stderr) - return 1 + return Status.ERROR.value + if __name__ == '__main__': sys.exit(main()) From 9c4ea1dc682f1a7ccf0c87887e7110fa3427b9fb Mon Sep 17 00:00:00 2001 From: Marenz Date: Thu, 2 Jun 2022 16:45:25 +0200 Subject: [PATCH 3/5] lsp.py: Add missing check for non-interactive --- test/lsp.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/lsp.py b/test/lsp.py index 117b65df2..f20e93953 100755 --- a/test/lsp.py +++ b/test/lsp.py @@ -643,7 +643,7 @@ class FileTestRunner: finally: self.close_all_open_files() - def user_interaction_failed_method_test(self, testcase, actual, expected): + def user_interaction_failed_method_test(self, testcase, actual, expected) -> TestResult: actual_pretty = self.suite.replace_ranges_with_tags(actual, self.sub_dir) if expected is None: @@ -654,6 +654,9 @@ class FileTestRunner: "\nbut got:\n" + actual_pretty ) + if self.suite.non_interactive: + return self.TestResult.SuccessOrIgnored + while True: print("(u)pdate/(r)etry/(i)gnore?") user_response = getCharFromStdin() From 927da20ce4387404cb94028bf3ba94e399340bdb Mon Sep 17 00:00:00 2001 From: Christian Parpart Date: Tue, 7 Jun 2022 15:41:15 +0200 Subject: [PATCH 4/5] Adds solidity::util::unreachable() helper function. --- libsolutil/Assertions.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/libsolutil/Assertions.h b/libsolutil/Assertions.h index 4924522d6..6735cda16 100644 --- a/libsolutil/Assertions.h +++ b/libsolutil/Assertions.h @@ -40,6 +40,28 @@ namespace solidity::util #define ETH_FUNC __func__ #endif +#if defined(__GNUC__) +// GCC 4.8+, Clang, Intel and other compilers compatible with GCC (-std=c++0x or above) +[[noreturn]] inline __attribute__((always_inline)) void unreachable() +{ + __builtin_unreachable(); +} + +#elif defined(_MSC_VER) // MSVC + +[[noreturn]] __forceinline void unreachable() +{ + __assume(false); +} + +#else + +[[noreturn]] inline void unreachable() +{ + solThrow(Exception, "Unreachable"); +} +#endif + namespace assertions { From 4ae43884d0e4dd9ebfd1f4393fcd2f894f07c473 Mon Sep 17 00:00:00 2001 From: Christian Parpart Date: Tue, 7 Jun 2022 15:43:14 +0200 Subject: [PATCH 5/5] Apply a better way to annotate unreachability to the C++ compiler. --- libsmtutil/CVC4Interface.cpp | 2 +- libsmtutil/Z3Interface.cpp | 4 ++-- libsolidity/ast/ASTJsonImporter.cpp | 10 +++++----- libsolidity/parsing/Parser.cpp | 2 +- libyul/AsmJsonImporter.cpp | 4 ++-- tools/yulPhaser/Phaser.cpp | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/libsmtutil/CVC4Interface.cpp b/libsmtutil/CVC4Interface.cpp index c335b18d1..ff356c025 100644 --- a/libsmtutil/CVC4Interface.cpp +++ b/libsmtutil/CVC4Interface.cpp @@ -292,7 +292,7 @@ CVC4::Expr CVC4Interface::toCVC4Expr(Expression const& _expr) smtAssert(false); // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } CVC4::Type CVC4Interface::cvc4Sort(Sort const& _sort) diff --git a/libsmtutil/Z3Interface.cpp b/libsmtutil/Z3Interface.cpp index 74767d86c..4abeffa16 100644 --- a/libsmtutil/Z3Interface.cpp +++ b/libsmtutil/Z3Interface.cpp @@ -274,7 +274,7 @@ z3::expr Z3Interface::toZ3Expr(Expression const& _expr) smtAssert(false); // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } Expression Z3Interface::fromZ3Expr(z3::expr const& _expr) @@ -385,7 +385,7 @@ Expression Z3Interface::fromZ3Expr(z3::expr const& _expr) smtAssert(false); // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } z3::sort Z3Interface::z3Sort(Sort const& _sort) diff --git a/libsolidity/ast/ASTJsonImporter.cpp b/libsolidity/ast/ASTJsonImporter.cpp index 859a93350..4fcea9aff 100644 --- a/libsolidity/ast/ASTJsonImporter.cpp +++ b/libsolidity/ast/ASTJsonImporter.cpp @@ -231,7 +231,7 @@ ASTPointer ASTJsonImporter::convertJsonToASTNode(Json::Value const& _js astAssert(false, "Unknown type of ASTNode: " + nodeType); // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } // ============ functions to instantiate the AST-Nodes from Json-Nodes ============== @@ -1078,7 +1078,7 @@ Visibility ASTJsonImporter::visibility(Json::Value const& _node) astAssert(false, "Unknown visibility declaration"); // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } VariableDeclaration::Location ASTJsonImporter::location(Json::Value const& _node) @@ -1100,7 +1100,7 @@ VariableDeclaration::Location ASTJsonImporter::location(Json::Value const& _node astAssert(false, "Unknown location declaration"); // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } Literal::SubDenomination ASTJsonImporter::subdenomination(Json::Value const& _node) @@ -1136,7 +1136,7 @@ Literal::SubDenomination ASTJsonImporter::subdenomination(Json::Value const& _no astAssert(false, "Unknown subdenomination"); // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } StateMutability ASTJsonImporter::stateMutability(Json::Value const& _node) @@ -1156,7 +1156,7 @@ StateMutability ASTJsonImporter::stateMutability(Json::Value const& _node) astAssert(false, "Unknown stateMutability"); // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } } diff --git a/libsolidity/parsing/Parser.cpp b/libsolidity/parsing/Parser.cpp index 0299ff092..554ebf75c 100644 --- a/libsolidity/parsing/Parser.cpp +++ b/libsolidity/parsing/Parser.cpp @@ -1655,7 +1655,7 @@ ASTPointer Parser::parseSimpleStatement(ASTPointer const& } // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } bool Parser::IndexAccessedPath::empty() const diff --git a/libyul/AsmJsonImporter.cpp b/libyul/AsmJsonImporter.cpp index 1f108145f..0a88cacf8 100644 --- a/libyul/AsmJsonImporter.cpp +++ b/libyul/AsmJsonImporter.cpp @@ -112,7 +112,7 @@ Statement AsmJsonImporter::createStatement(Json::Value const& _node) yulAssert(false, "Invalid nodeType as statement"); // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } Expression AsmJsonImporter::createExpression(Json::Value const& _node) @@ -134,7 +134,7 @@ Expression AsmJsonImporter::createExpression(Json::Value const& _node) yulAssert(false, "Invalid nodeType as expression"); // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } vector AsmJsonImporter::createExpressionVector(Json::Value const& _array) diff --git a/tools/yulPhaser/Phaser.cpp b/tools/yulPhaser/Phaser.cpp index c49d9a2d8..213e6f15b 100644 --- a/tools/yulPhaser/Phaser.cpp +++ b/tools/yulPhaser/Phaser.cpp @@ -278,7 +278,7 @@ unique_ptr FitnessMetricFactory::build( } // FIXME: Workaround for spurious GCC 12.1 warning (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105794) - throw exception(); + util::unreachable(); } PopulationFactory::Options PopulationFactory::Options::fromCommandLine(po::variables_map const& _arguments)