From 1c636fd8450f3cc062c0cdd4be33bbc9ddc884df Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 12 Mar 2024 11:01:40 -0700 Subject: [PATCH 1/7] [Impeller] started asserting test runners have fatal validations --- impeller/base/BUILD.gn | 5 ++++- impeller/base/validation.cc | 13 ++++++++++++- impeller/base/validation.h | 4 ++++ impeller/base/validation_unittests.cc | 18 ++++++++++++++++++ 4 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 impeller/base/validation_unittests.cc diff --git a/impeller/base/BUILD.gn b/impeller/base/BUILD.gn index c021a96e355c0..57397fbf97c4d 100644 --- a/impeller/base/BUILD.gn +++ b/impeller/base/BUILD.gn @@ -32,7 +32,10 @@ impeller_component("base") { impeller_component("base_unittests") { testonly = true - sources = [ "base_unittests.cc" ] + sources = [ + "base_unittests.cc", + "validation_unittests.cc", + ] deps = [ ":base", "//flutter/testing", diff --git a/impeller/base/validation.cc b/impeller/base/validation.cc index d68aa6d888bc6..5f816ea8c326e 100644 --- a/impeller/base/validation.cc +++ b/impeller/base/validation.cc @@ -11,7 +11,7 @@ namespace impeller { static std::atomic_int32_t sValidationLogsDisabledCount = 0; -static std::atomic_int32_t sValidationLogsAreFatal = 0; +static std::atomic_int32_t sValidationLogsAreFatal = 1; void ImpellerValidationErrorsSetFatal(bool fatal) { sValidationLogsAreFatal = fatal; @@ -59,4 +59,15 @@ void ImpellerValidationBreak(const char* message) { #endif // IMPELLER_ENABLE_VALIDATION } +bool ImpellerValidationIsFatal() { + return sValidationLogsAreFatal; +} + +bool ImpellerValidationIsEnabled() { +#ifdef IMPELLER_ENABLE_VALIDATION + return true; +#else + return false; +#endif +} } // namespace impeller diff --git a/impeller/base/validation.h b/impeller/base/validation.h index a015686c06851..c00c9fd78760c 100644 --- a/impeller/base/validation.h +++ b/impeller/base/validation.h @@ -39,6 +39,10 @@ void ImpellerValidationBreak(const char* message); void ImpellerValidationErrorsSetFatal(bool fatal); +bool ImpellerValidationIsFatal(); + +bool ImpellerValidationIsEnabled(); + struct ScopedValidationDisable { ScopedValidationDisable(); diff --git a/impeller/base/validation_unittests.cc b/impeller/base/validation_unittests.cc new file mode 100644 index 0000000000000..afb43dc741cd0 --- /dev/null +++ b/impeller/base/validation_unittests.cc @@ -0,0 +1,18 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/testing/testing.h" +#include "impeller/base/validation.h" + +namespace impeller { + +TEST(ValidationTest, IsFatal) { + EXPECT_TRUE(ImpellerValidationIsFatal()); +} + +TEST(ValidationTest, IsEnabled) { + EXPECT_TRUE(ImpellerValidationIsEnabled()); +} + +} // namespace impeller From 6a733252173b2be9868d140a1655f12da49ac95f Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 12 Mar 2024 12:54:28 -0700 Subject: [PATCH 2/7] added assert to golden test runner --- impeller/golden_tests/main.cc | 11 +++++++++++ impeller/tools/impeller.gni | 2 ++ 2 files changed, 13 insertions(+) diff --git a/impeller/golden_tests/main.cc b/impeller/golden_tests/main.cc index e1ed2a8c4eccc..158ac944daaa0 100644 --- a/impeller/golden_tests/main.cc +++ b/impeller/golden_tests/main.cc @@ -8,6 +8,7 @@ #include "flutter/fml/build_config.h" #include "flutter/fml/command_line.h" #include "flutter/fml/logging.h" +#include "flutter/impeller/base/validation.h" #include "flutter/impeller/golden_tests/golden_digest.h" #include "flutter/impeller/golden_tests/working_directory.h" #include "gtest/gtest.h" @@ -24,6 +25,16 @@ void print_usage() { } } // namespace +namespace impeller { +TEST(ValidationTest, IsFatal) { + EXPECT_TRUE(ImpellerValidationIsFatal()); +} + +TEST(ValidationTest, IsEnabled) { + EXPECT_TRUE(ImpellerValidationIsEnabled()); +} +} // namespace impeller + int main(int argc, char** argv) { fml::InstallCrashHandler(); testing::InitGoogleTest(&argc, argv); diff --git a/impeller/tools/impeller.gni b/impeller/tools/impeller.gni index 326f48c93f131..4953857a86a84 100644 --- a/impeller/tools/impeller.gni +++ b/impeller/tools/impeller.gni @@ -54,6 +54,8 @@ declare_args() { } declare_args() { + impeller_enable_validation = impeller_debug + # Wether to build and include the validation layers. impeller_enable_vulkan_validation_layers = impeller_enable_vulkan && flutter_runtime_mode == "debug" && From d5eb464aabe087340a87c3f2f25d247abea37b9d Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 12 Mar 2024 13:16:21 -0700 Subject: [PATCH 3/7] moved the golden test runner to the profile build --- ci/builders/mac_host_engine.json | 38 +++++++++++++++++++++++--------- impeller/tools/impeller.gni | 2 -- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/ci/builders/mac_host_engine.json b/ci/builders/mac_host_engine.json index 8998f67ac928c..f824cc61e8b68 100644 --- a/ci/builders/mac_host_engine.json +++ b/ci/builders/mac_host_engine.json @@ -143,12 +143,6 @@ "cpu=x86", "mac_model=Macmini8,1" ], - "dependencies": [ - { - "dependency": "goldctl", - "version": "git_revision:720a542f6fe4f92922c3b8f0fdcc4d2ac6bb83cd" - } - ], "gclient_variables": { "download_android_deps": false }, @@ -169,7 +163,6 @@ "flutter/build/archives:archive_gen_snapshot", "flutter/build/archives:artifacts", "flutter/build/dart:copy_dart_sdk", - "flutter/impeller/golden_tests:impeller_golden_tests", "flutter/shell/platform/darwin/macos:zip_macos_flutter_framework", "flutter/tools/font_subset", "flutter:unittests" @@ -183,13 +176,13 @@ "tests": [ { "language": "python3", - "name": "Impeller-golden, dart and engine tests for host_release", + "name": "dart and engine tests for host_release", "script": "flutter/testing/run_tests.py", "parameters": [ "--variant", "host_release", "--type", - "dart,dart-host,engine,impeller-golden" + "dart,dart-host,engine" ] } ] @@ -259,6 +252,12 @@ "os=Mac-13", "cpu=x86" ], + "dependencies": [ + { + "dependency": "goldctl", + "version": "git_revision:720a542f6fe4f92922c3b8f0fdcc4d2ac6bb83cd" + } + ], "gclient_variables": { "download_android_deps": false }, @@ -269,13 +268,17 @@ "--runtime-mode", "profile", "--no-lto", - "--prebuilt-dart-sdk" + "--prebuilt-dart-sdk", + "--enable-impeller-vulkan", + "--enable-impeller-opengles", + "--use-glfw-swiftshader" ], "name": "mac_profile_arm64", "ninja": { "config": "mac_profile_arm64", "targets": [ "flutter/build/archives:artifacts", + "flutter/impeller/golden_tests:impeller_golden_tests", "flutter/shell/platform/darwin/macos:zip_macos_flutter_framework" ] }, @@ -283,7 +286,20 @@ "$flutter/osx_sdk": { "sdk_version": "15a240d" } - } + }, + "tests": [ + { + "language": "python3", + "name": "Impeller-golden", + "script": "flutter/testing/run_tests.py", + "parameters": [ + "--variant", + "host_profile", + "--type", + "impeller-golden" + ] + } + ] }, { "archives": [ diff --git a/impeller/tools/impeller.gni b/impeller/tools/impeller.gni index 4953857a86a84..326f48c93f131 100644 --- a/impeller/tools/impeller.gni +++ b/impeller/tools/impeller.gni @@ -54,8 +54,6 @@ declare_args() { } declare_args() { - impeller_enable_validation = impeller_debug - # Wether to build and include the validation layers. impeller_enable_vulkan_validation_layers = impeller_enable_vulkan && flutter_runtime_mode == "debug" && From 416c19716addb25eb78b2d1b7512f1fa46c5f4b3 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 12 Mar 2024 13:22:44 -0700 Subject: [PATCH 4/7] Revert "moved the golden test runner to the profile build" This reverts commit d5eb464aabe087340a87c3f2f25d247abea37b9d. --- ci/builders/mac_host_engine.json | 38 +++++++++----------------------- impeller/tools/impeller.gni | 2 ++ 2 files changed, 13 insertions(+), 27 deletions(-) diff --git a/ci/builders/mac_host_engine.json b/ci/builders/mac_host_engine.json index f824cc61e8b68..8998f67ac928c 100644 --- a/ci/builders/mac_host_engine.json +++ b/ci/builders/mac_host_engine.json @@ -143,6 +143,12 @@ "cpu=x86", "mac_model=Macmini8,1" ], + "dependencies": [ + { + "dependency": "goldctl", + "version": "git_revision:720a542f6fe4f92922c3b8f0fdcc4d2ac6bb83cd" + } + ], "gclient_variables": { "download_android_deps": false }, @@ -163,6 +169,7 @@ "flutter/build/archives:archive_gen_snapshot", "flutter/build/archives:artifacts", "flutter/build/dart:copy_dart_sdk", + "flutter/impeller/golden_tests:impeller_golden_tests", "flutter/shell/platform/darwin/macos:zip_macos_flutter_framework", "flutter/tools/font_subset", "flutter:unittests" @@ -176,13 +183,13 @@ "tests": [ { "language": "python3", - "name": "dart and engine tests for host_release", + "name": "Impeller-golden, dart and engine tests for host_release", "script": "flutter/testing/run_tests.py", "parameters": [ "--variant", "host_release", "--type", - "dart,dart-host,engine" + "dart,dart-host,engine,impeller-golden" ] } ] @@ -252,12 +259,6 @@ "os=Mac-13", "cpu=x86" ], - "dependencies": [ - { - "dependency": "goldctl", - "version": "git_revision:720a542f6fe4f92922c3b8f0fdcc4d2ac6bb83cd" - } - ], "gclient_variables": { "download_android_deps": false }, @@ -268,17 +269,13 @@ "--runtime-mode", "profile", "--no-lto", - "--prebuilt-dart-sdk", - "--enable-impeller-vulkan", - "--enable-impeller-opengles", - "--use-glfw-swiftshader" + "--prebuilt-dart-sdk" ], "name": "mac_profile_arm64", "ninja": { "config": "mac_profile_arm64", "targets": [ "flutter/build/archives:artifacts", - "flutter/impeller/golden_tests:impeller_golden_tests", "flutter/shell/platform/darwin/macos:zip_macos_flutter_framework" ] }, @@ -286,20 +283,7 @@ "$flutter/osx_sdk": { "sdk_version": "15a240d" } - }, - "tests": [ - { - "language": "python3", - "name": "Impeller-golden", - "script": "flutter/testing/run_tests.py", - "parameters": [ - "--variant", - "host_profile", - "--type", - "impeller-golden" - ] - } - ] + } }, { "archives": [ diff --git a/impeller/tools/impeller.gni b/impeller/tools/impeller.gni index 326f48c93f131..4953857a86a84 100644 --- a/impeller/tools/impeller.gni +++ b/impeller/tools/impeller.gni @@ -54,6 +54,8 @@ declare_args() { } declare_args() { + impeller_enable_validation = impeller_debug + # Wether to build and include the validation layers. impeller_enable_vulkan_validation_layers = impeller_enable_vulkan && flutter_runtime_mode == "debug" && From ff9972964e52e9ae6dfc005ce8e7c36da9fd57ae Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 12 Mar 2024 13:34:08 -0700 Subject: [PATCH 5/7] just removed conditional compilation for validation --- impeller/base/BUILD.gn | 5 +---- impeller/base/validation.cc | 11 ++--------- impeller/base/validation.h | 10 +--------- impeller/base/validation_unittests.cc | 18 ------------------ impeller/golden_tests/main.cc | 7 ++----- 5 files changed, 6 insertions(+), 45 deletions(-) delete mode 100644 impeller/base/validation_unittests.cc diff --git a/impeller/base/BUILD.gn b/impeller/base/BUILD.gn index 57397fbf97c4d..c021a96e355c0 100644 --- a/impeller/base/BUILD.gn +++ b/impeller/base/BUILD.gn @@ -32,10 +32,7 @@ impeller_component("base") { impeller_component("base_unittests") { testonly = true - sources = [ - "base_unittests.cc", - "validation_unittests.cc", - ] + sources = [ "base_unittests.cc" ] deps = [ ":base", "//flutter/testing", diff --git a/impeller/base/validation.cc b/impeller/base/validation.cc index 5f816ea8c326e..aef78bb2b9933 100644 --- a/impeller/base/validation.cc +++ b/impeller/base/validation.cc @@ -11,7 +11,7 @@ namespace impeller { static std::atomic_int32_t sValidationLogsDisabledCount = 0; -static std::atomic_int32_t sValidationLogsAreFatal = 1; +static std::atomic_int32_t sValidationLogsAreFatal = 0; void ImpellerValidationErrorsSetFatal(bool fatal) { sValidationLogsAreFatal = fatal; @@ -59,15 +59,8 @@ void ImpellerValidationBreak(const char* message) { #endif // IMPELLER_ENABLE_VALIDATION } -bool ImpellerValidationIsFatal() { +bool ImpellerValidationErrorsAreFatal() { return sValidationLogsAreFatal; } -bool ImpellerValidationIsEnabled() { -#ifdef IMPELLER_ENABLE_VALIDATION - return true; -#else - return false; -#endif -} } // namespace impeller diff --git a/impeller/base/validation.h b/impeller/base/validation.h index c00c9fd78760c..6d3ca3a52d19f 100644 --- a/impeller/base/validation.h +++ b/impeller/base/validation.h @@ -5,12 +5,6 @@ #ifndef FLUTTER_IMPELLER_BASE_VALIDATION_H_ #define FLUTTER_IMPELLER_BASE_VALIDATION_H_ -#ifndef IMPELLER_ENABLE_VALIDATION -#ifdef IMPELLER_DEBUG -#define IMPELLER_ENABLE_VALIDATION 1 -#endif -#endif - #include namespace impeller { @@ -39,9 +33,7 @@ void ImpellerValidationBreak(const char* message); void ImpellerValidationErrorsSetFatal(bool fatal); -bool ImpellerValidationIsFatal(); - -bool ImpellerValidationIsEnabled(); +bool ImpellerValidationErrorsAreFatal(); struct ScopedValidationDisable { ScopedValidationDisable(); diff --git a/impeller/base/validation_unittests.cc b/impeller/base/validation_unittests.cc deleted file mode 100644 index afb43dc741cd0..0000000000000 --- a/impeller/base/validation_unittests.cc +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "flutter/testing/testing.h" -#include "impeller/base/validation.h" - -namespace impeller { - -TEST(ValidationTest, IsFatal) { - EXPECT_TRUE(ImpellerValidationIsFatal()); -} - -TEST(ValidationTest, IsEnabled) { - EXPECT_TRUE(ImpellerValidationIsEnabled()); -} - -} // namespace impeller diff --git a/impeller/golden_tests/main.cc b/impeller/golden_tests/main.cc index 158ac944daaa0..4791f3818a9f0 100644 --- a/impeller/golden_tests/main.cc +++ b/impeller/golden_tests/main.cc @@ -27,15 +27,12 @@ void print_usage() { namespace impeller { TEST(ValidationTest, IsFatal) { - EXPECT_TRUE(ImpellerValidationIsFatal()); -} - -TEST(ValidationTest, IsEnabled) { - EXPECT_TRUE(ImpellerValidationIsEnabled()); + EXPECT_TRUE(ImpellerValidationErrorsAreFatal()); } } // namespace impeller int main(int argc, char** argv) { + impeller::ImpellerValidationErrorsSetFatal(true); fml::InstallCrashHandler(); testing::InitGoogleTest(&argc, argv); fml::CommandLine cmd = fml::CommandLineFromPlatformOrArgcArgv(argc, argv); From d7c09bd78bd2b576a15a433a2487936e3f27220d Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 12 Mar 2024 13:36:01 -0700 Subject: [PATCH 6/7] removed stray arg --- impeller/tools/impeller.gni | 2 -- 1 file changed, 2 deletions(-) diff --git a/impeller/tools/impeller.gni b/impeller/tools/impeller.gni index 4953857a86a84..326f48c93f131 100644 --- a/impeller/tools/impeller.gni +++ b/impeller/tools/impeller.gni @@ -54,8 +54,6 @@ declare_args() { } declare_args() { - impeller_enable_validation = impeller_debug - # Wether to build and include the validation layers. impeller_enable_vulkan_validation_layers = impeller_enable_vulkan && flutter_runtime_mode == "debug" && From 2678a138b45d2d93fc45ad38b50b3eb6cbc87b3a Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 12 Mar 2024 14:10:10 -0700 Subject: [PATCH 7/7] cleaned up output --- impeller/base/validation.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/impeller/base/validation.cc b/impeller/base/validation.cc index aef78bb2b9933..d95a84c811c89 100644 --- a/impeller/base/validation.cc +++ b/impeller/base/validation.cc @@ -46,17 +46,18 @@ std::ostream& ValidationLog::GetStream() { } void ImpellerValidationBreak(const char* message) { -// Nothing to do. Exists for the debugger. -#ifdef IMPELLER_ENABLE_VALIDATION std::stringstream stream; +#if FLUTTER_RELEASE + stream << "Impeller validation: " << message; +#else stream << "Break on '" << __FUNCTION__ << "' to inspect point of failure: " << message; +#endif if (sValidationLogsAreFatal > 0) { FML_LOG(FATAL) << stream.str(); } else { FML_LOG(ERROR) << stream.str(); } -#endif // IMPELLER_ENABLE_VALIDATION } bool ImpellerValidationErrorsAreFatal() {