From 81e55754e148eb27ec835fceebac577003df8a6b Mon Sep 17 00:00:00 2001 From: James Price Date: Tue, 24 Jan 2023 21:01:36 +0000 Subject: [PATCH] tint/resolver: Handle diagnostic directives The Resolver parses the diagnostic rule and sets the updated severity in a ScopeStack, which is stored in the Validator. Automatically generate the diagnostic rule enum and its parsing logic using intrinsics.def. Add a "chromium_unreachable_code" diagnostic rule to test this. Bug: tint:1809 Change-Id: Ia94db4321b8019f01d31a84da0fda25dfdf72f5c Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/117566 Kokoro: Kokoro Reviewed-by: Ben Clayton --- src/tint/BUILD.gn | 1 + src/tint/CMakeLists.txt | 1 + src/tint/ast/diagnostic_control.cc | 33 ++++++++ src/tint/ast/diagnostic_control.cc.tmpl | 17 +++++ src/tint/ast/diagnostic_control.h | 23 ++++++ src/tint/ast/diagnostic_control.h.tmpl | 6 ++ src/tint/ast/diagnostic_control_bench.cc | 16 ++++ src/tint/ast/diagnostic_control_bench.cc.tmpl | 2 + src/tint/ast/diagnostic_control_test.cc | 48 ++++++++++++ src/tint/ast/diagnostic_control_test.cc.tmpl | 6 ++ src/tint/intrinsics.def | 6 ++ src/tint/resolver/diagnostic_control_test.cc | 75 +++++++++++++++++++ src/tint/resolver/resolver.cc | 11 +++ src/tint/resolver/resolver.h | 5 +- src/tint/resolver/validator.cc | 31 +++++++- src/tint/resolver/validator.h | 17 +++++ 16 files changed, 293 insertions(+), 5 deletions(-) create mode 100644 src/tint/resolver/diagnostic_control_test.cc diff --git a/src/tint/BUILD.gn b/src/tint/BUILD.gn index ab972477eb..969262a251 100644 --- a/src/tint/BUILD.gn +++ b/src/tint/BUILD.gn @@ -1293,6 +1293,7 @@ if (tint_build_unittests) { "resolver/const_eval_unary_op_test.cc", "resolver/control_block_validation_test.cc", "resolver/dependency_graph_test.cc", + "resolver/diagnostic_control_test.cc", "resolver/entry_point_validation_test.cc", "resolver/evaluation_stage_test.cc", "resolver/f16_extension_test.cc", diff --git a/src/tint/CMakeLists.txt b/src/tint/CMakeLists.txt index b9837c9f2a..a6306de2f9 100644 --- a/src/tint/CMakeLists.txt +++ b/src/tint/CMakeLists.txt @@ -926,6 +926,7 @@ if(TINT_BUILD_TESTS) resolver/const_eval_unary_op_test.cc resolver/control_block_validation_test.cc resolver/dependency_graph_test.cc + resolver/diagnostic_control_test.cc resolver/entry_point_validation_test.cc resolver/evaluation_stage_test.cc resolver/f16_extension_test.cc diff --git a/src/tint/ast/diagnostic_control.cc b/src/tint/ast/diagnostic_control.cc index d83e9c2bac..48f487df2c 100644 --- a/src/tint/ast/diagnostic_control.cc +++ b/src/tint/ast/diagnostic_control.cc @@ -38,6 +38,19 @@ const DiagnosticControl* DiagnosticControl::Clone(CloneContext* ctx) const { return ctx->dst->create(src, severity, rule); } +diag::Severity ToSeverity(DiagnosticSeverity sc) { + switch (sc) { + case DiagnosticSeverity::kError: + return diag::Severity::Error; + case DiagnosticSeverity::kWarning: + return diag::Severity::Warning; + case DiagnosticSeverity::kInfo: + return diag::Severity::Note; + default: + return diag::Severity::InternalCompilerError; + } +} + /// ParseDiagnosticSeverity parses a DiagnosticSeverity from a string. /// @param str the string to parse /// @returns the parsed enum, or DiagnosticSeverity::kUndefined if the string could not be parsed. @@ -73,4 +86,24 @@ std::ostream& operator<<(std::ostream& out, DiagnosticSeverity value) { return out << ""; } +/// ParseDiagnosticRule parses a DiagnosticRule from a string. +/// @param str the string to parse +/// @returns the parsed enum, or DiagnosticRule::kUndefined if the string could not be parsed. +DiagnosticRule ParseDiagnosticRule(std::string_view str) { + if (str == "chromium_unreachable_code") { + return DiagnosticRule::kChromiumUnreachableCode; + } + return DiagnosticRule::kUndefined; +} + +std::ostream& operator<<(std::ostream& out, DiagnosticRule value) { + switch (value) { + case DiagnosticRule::kUndefined: + return out << "undefined"; + case DiagnosticRule::kChromiumUnreachableCode: + return out << "chromium_unreachable_code"; + } + return out << ""; +} + } // namespace tint::ast diff --git a/src/tint/ast/diagnostic_control.cc.tmpl b/src/tint/ast/diagnostic_control.cc.tmpl index c5d1a73d1e..c6dc463e78 100644 --- a/src/tint/ast/diagnostic_control.cc.tmpl +++ b/src/tint/ast/diagnostic_control.cc.tmpl @@ -28,8 +28,25 @@ const DiagnosticControl* DiagnosticControl::Clone(CloneContext* ctx) const { return ctx->dst->create(src, severity, rule); } +diag::Severity ToSeverity(DiagnosticSeverity sc) { + switch (sc) { + case DiagnosticSeverity::kError: + return diag::Severity::Error; + case DiagnosticSeverity::kWarning: + return diag::Severity::Warning; + case DiagnosticSeverity::kInfo: + return diag::Severity::Note; + default: + return diag::Severity::InternalCompilerError; + } +} + {{ Eval "ParseEnum" (Sem.Enum "diagnostic_severity")}} {{ Eval "EnumOStream" (Sem.Enum "diagnostic_severity")}} +{{ Eval "ParseEnum" (Sem.Enum "diagnostic_rule")}} + +{{ Eval "EnumOStream" (Sem.Enum "diagnostic_rule")}} + } // namespace tint::ast diff --git a/src/tint/ast/diagnostic_control.h b/src/tint/ast/diagnostic_control.h index f127c1fe48..2571265521 100644 --- a/src/tint/ast/diagnostic_control.h +++ b/src/tint/ast/diagnostic_control.h @@ -61,6 +61,29 @@ constexpr const char* kDiagnosticSeverityStrings[] = { "warning", }; +/// The diagnostic rule. +enum class DiagnosticRule { + kUndefined, + kChromiumUnreachableCode, +}; + +/// @param out the std::ostream to write to +/// @param value the DiagnosticRule +/// @returns `out` so calls can be chained +std::ostream& operator<<(std::ostream& out, DiagnosticRule value); + +/// ParseDiagnosticRule parses a DiagnosticRule from a string. +/// @param str the string to parse +/// @returns the parsed enum, or DiagnosticRule::kUndefined if the string could not be parsed. +DiagnosticRule ParseDiagnosticRule(std::string_view str); + +constexpr const char* kDiagnosticRuleStrings[] = { + "chromium_unreachable_code", +}; + +/// Convert a DiagnosticSeverity to the corresponding diag::Severity. +diag::Severity ToSeverity(DiagnosticSeverity sc); + /// A diagnostic control used for diagnostic directives and attributes. class DiagnosticControl : public Castable { public: diff --git a/src/tint/ast/diagnostic_control.h.tmpl b/src/tint/ast/diagnostic_control.h.tmpl index 5ddbc6df2f..5018783eed 100644 --- a/src/tint/ast/diagnostic_control.h.tmpl +++ b/src/tint/ast/diagnostic_control.h.tmpl @@ -28,6 +28,12 @@ namespace tint::ast { /// The diagnostic severity control. {{ Eval "DeclareEnum" (Sem.Enum "diagnostic_severity") }} +/// The diagnostic rule. +{{ Eval "DeclareEnum" (Sem.Enum "diagnostic_rule") }} + +/// Convert a DiagnosticSeverity to the corresponding diag::Severity. +diag::Severity ToSeverity(DiagnosticSeverity sc); + /// A diagnostic control used for diagnostic directives and attributes. class DiagnosticControl : public Castable { public: diff --git a/src/tint/ast/diagnostic_control_bench.cc b/src/tint/ast/diagnostic_control_bench.cc index d96f93b8cb..5c03ed637b 100644 --- a/src/tint/ast/diagnostic_control_bench.cc +++ b/src/tint/ast/diagnostic_control_bench.cc @@ -46,5 +46,21 @@ void DiagnosticSeverityParser(::benchmark::State& state) { BENCHMARK(DiagnosticSeverityParser); +void DiagnosticRuleParser(::benchmark::State& state) { + std::array kStrings{ + "hromium_unyeachable_code", "chrorrillmGunnreachable_c77de", "chromium_unreachable4cod00", + "chromium_unreachable_code", "chromium_unracaboo_code", "chromium_unrzzchabl_code", + "ciipp11ium_unreachable_cod", + }; + for (auto _ : state) { + for (auto& str : kStrings) { + auto result = ParseDiagnosticRule(str); + benchmark::DoNotOptimize(result); + } + } +} + +BENCHMARK(DiagnosticRuleParser); + } // namespace } // namespace tint::ast diff --git a/src/tint/ast/diagnostic_control_bench.cc.tmpl b/src/tint/ast/diagnostic_control_bench.cc.tmpl index 48058cabd7..55d3cce0de 100644 --- a/src/tint/ast/diagnostic_control_bench.cc.tmpl +++ b/src/tint/ast/diagnostic_control_bench.cc.tmpl @@ -21,5 +21,7 @@ namespace { {{ Eval "BenchmarkParseEnum" (Sem.Enum "diagnostic_severity")}} +{{ Eval "BenchmarkParseEnum" (Sem.Enum "diagnostic_rule")}} + } // namespace } // namespace tint::ast diff --git a/src/tint/ast/diagnostic_control_test.cc b/src/tint/ast/diagnostic_control_test.cc index 2dbd13952f..90e8c84ff9 100644 --- a/src/tint/ast/diagnostic_control_test.cc +++ b/src/tint/ast/diagnostic_control_test.cc @@ -100,5 +100,53 @@ INSTANTIATE_TEST_SUITE_P(ValidCases, DiagnosticSeverityPrintTest, testing::Value } // namespace diagnostic_severity_tests +namespace diagnostic_rule_tests { + +namespace parse_print_tests { + +struct Case { + const char* string; + DiagnosticRule value; +}; + +inline std::ostream& operator<<(std::ostream& out, Case c) { + return out << "'" << std::string(c.string) << "'"; +} + +static constexpr Case kValidCases[] = { + {"chromium_unreachable_code", DiagnosticRule::kChromiumUnreachableCode}, +}; + +static constexpr Case kInvalidCases[] = { + {"cXromggum_unreachable_cde", DiagnosticRule::kUndefined}, + {"chroVium_unruchble_codX", DiagnosticRule::kUndefined}, + {"chromium_3nreachable_code", DiagnosticRule::kUndefined}, +}; + +using DiagnosticRuleParseTest = testing::TestWithParam; + +TEST_P(DiagnosticRuleParseTest, Parse) { + const char* string = GetParam().string; + DiagnosticRule expect = GetParam().value; + EXPECT_EQ(expect, ParseDiagnosticRule(string)); +} + +INSTANTIATE_TEST_SUITE_P(ValidCases, DiagnosticRuleParseTest, testing::ValuesIn(kValidCases)); +INSTANTIATE_TEST_SUITE_P(InvalidCases, DiagnosticRuleParseTest, testing::ValuesIn(kInvalidCases)); + +using DiagnosticRulePrintTest = testing::TestWithParam; + +TEST_P(DiagnosticRulePrintTest, Print) { + DiagnosticRule value = GetParam().value; + const char* expect = GetParam().string; + EXPECT_EQ(expect, utils::ToString(value)); +} + +INSTANTIATE_TEST_SUITE_P(ValidCases, DiagnosticRulePrintTest, testing::ValuesIn(kValidCases)); + +} // namespace parse_print_tests + +} // namespace diagnostic_rule_tests + } // namespace } // namespace tint::ast diff --git a/src/tint/ast/diagnostic_control_test.cc.tmpl b/src/tint/ast/diagnostic_control_test.cc.tmpl index a078cceaed..2d1a2ccf0c 100644 --- a/src/tint/ast/diagnostic_control_test.cc.tmpl +++ b/src/tint/ast/diagnostic_control_test.cc.tmpl @@ -41,5 +41,11 @@ namespace diagnostic_severity_tests { } // namespace diagnostic_severity_tests +namespace diagnostic_rule_tests { + +{{ Eval "TestParsePrintEnum" (Sem.Enum "diagnostic_rule")}} + +} // namespace diagnostic_rule_tests + } // namespace } // namespace tint::ast diff --git a/src/tint/intrinsics.def b/src/tint/intrinsics.def index 05fe8c9b0a..0600bc125f 100644 --- a/src/tint/intrinsics.def +++ b/src/tint/intrinsics.def @@ -40,6 +40,12 @@ enum builtin_value { @internal point_size } +// https://gpuweb.github.io/gpuweb/wgsl/#filterable-triggering-rules +enum diagnostic_rule { + // Chromium specific rules not defined in the spec. + chromium_unreachable_code +} + // https://gpuweb.github.io/gpuweb/wgsl/#syntax-severity_control_name enum diagnostic_severity { error diff --git a/src/tint/resolver/diagnostic_control_test.cc b/src/tint/resolver/diagnostic_control_test.cc new file mode 100644 index 0000000000..b02a5e85d8 --- /dev/null +++ b/src/tint/resolver/diagnostic_control_test.cc @@ -0,0 +1,75 @@ +// Copyright 2023 The Tint Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "src/tint/resolver/resolver.h" + +#include "src/tint/resolver/resolver_test_helper.h" + +using namespace tint::number_suffixes; // NOLINT + +namespace tint::resolver { +namespace { + +using ResolverDiagnosticControlTest = ResolverTest; + +TEST_F(ResolverDiagnosticControlTest, UnreachableCode_DefaultSeverity) { + auto stmts = utils::Vector{Return(), Return()}; + Func("foo", {}, ty.void_(), stmts); + + EXPECT_TRUE(r()->Resolve()) << r()->error(); + EXPECT_EQ(r()->error(), R"(warning: code is unreachable)"); +} + +TEST_F(ResolverDiagnosticControlTest, UnreachableCode_ErrorViaDirective) { + DiagnosticDirective(ast::DiagnosticSeverity::kError, Expr("chromium_unreachable_code")); + + auto stmts = utils::Vector{Return(), Return()}; + Func("foo", {}, ty.void_(), stmts); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(error: code is unreachable)"); +} + +TEST_F(ResolverDiagnosticControlTest, UnreachableCode_WarningViaDirective) { + DiagnosticDirective(ast::DiagnosticSeverity::kWarning, Expr("chromium_unreachable_code")); + + auto stmts = utils::Vector{Return(), Return()}; + Func("foo", {}, ty.void_(), stmts); + + EXPECT_TRUE(r()->Resolve()) << r()->error(); + EXPECT_EQ(r()->error(), R"(warning: code is unreachable)"); +} + +TEST_F(ResolverDiagnosticControlTest, UnreachableCode_InfoViaDirective) { + DiagnosticDirective(ast::DiagnosticSeverity::kInfo, Expr("chromium_unreachable_code")); + + auto stmts = utils::Vector{Return(), Return()}; + Func("foo", {}, ty.void_(), stmts); + + EXPECT_TRUE(r()->Resolve()) << r()->error(); + EXPECT_EQ(r()->error(), R"(note: code is unreachable)"); +} + +TEST_F(ResolverDiagnosticControlTest, UnreachableCode_OffViaDirective) { + DiagnosticDirective(ast::DiagnosticSeverity::kOff, Expr("chromium_unreachable_code")); + + auto stmts = utils::Vector{Return(), Return()}; + Func("foo", {}, ty.void_(), stmts); + + EXPECT_TRUE(r()->Resolve()) << r()->error(); + EXPECT_TRUE(r()->error().empty()); +} + +} // namespace +} // namespace tint::resolver diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index 59e36bbd6b..b80db263a6 100644 --- a/src/tint/resolver/resolver.cc +++ b/src/tint/resolver/resolver.cc @@ -151,6 +151,7 @@ bool Resolver::ResolveInternal() { Mark(decl); if (!Switch( decl, // + [&](const ast::DiagnosticControl* dc) { return DiagnosticControl(dc); }, [&](const ast::Enable* e) { return Enable(e); }, [&](const ast::TypeDecl* td) { return TypeDecl(td); }, [&](const ast::Function* func) { return Function(func); }, @@ -3050,6 +3051,16 @@ sem::Expression* Resolver::UnaryOp(const ast::UnaryOpExpression* unary) { return sem; } +bool Resolver::DiagnosticControl(const ast::DiagnosticControl* control) { + Mark(control->rule_name); + auto rule_name = builder_->Symbols().NameFor(control->rule_name->symbol); + auto rule = ast::ParseDiagnosticRule(rule_name); + if (rule != ast::DiagnosticRule::kUndefined) { + validator_.DiagnosticFilters().Set(rule, control->severity); + } + return true; +} + bool Resolver::Enable(const ast::Enable* enable) { enabled_extensions_.Add(enable->extension); return true; diff --git a/src/tint/resolver/resolver.h b/src/tint/resolver/resolver.h index 87998854a9..d6d7ba9991 100644 --- a/src/tint/resolver/resolver.h +++ b/src/tint/resolver/resolver.h @@ -30,7 +30,6 @@ #include "src/tint/resolver/intrinsic_table.h" #include "src/tint/resolver/sem_helper.h" #include "src/tint/resolver/validator.h" -#include "src/tint/scope_stack.h" #include "src/tint/sem/binding_point.h" #include "src/tint/sem/block_statement.h" #include "src/tint/sem/function.h" @@ -262,6 +261,10 @@ class Resolver { /// @param ty the ast::Type type::Type* Type(const ast::Type* ty); + /// @param control the diagnostic control + /// @returns true on success, false on failure + bool DiagnosticControl(const ast::DiagnosticControl* control); + /// @param enable the enable declaration /// @returns the resolved extension bool Enable(const ast::Enable* enable); diff --git a/src/tint/resolver/validator.cc b/src/tint/resolver/validator.cc index 18f53fbc1d..4b769d1c37 100644 --- a/src/tint/resolver/validator.cc +++ b/src/tint/resolver/validator.cc @@ -166,7 +166,11 @@ Validator::Validator( sem_(sem), enabled_extensions_(enabled_extensions), atomic_composite_info_(atomic_composite_info), - valid_type_storage_layouts_(valid_type_storage_layouts) {} + valid_type_storage_layouts_(valid_type_storage_layouts) { + // Set default severities for filterable diagnostic rules. + diagnostic_filters_.Set(ast::DiagnosticRule::kChromiumUnreachableCode, + ast::DiagnosticSeverity::kWarning); +} Validator::~Validator() = default; @@ -182,6 +186,24 @@ void Validator::AddNote(const std::string& msg, const Source& source) const { diagnostics_.add_note(diag::System::Resolver, msg, source); } +bool Validator::AddDiagnostic(ast::DiagnosticRule rule, + const std::string& msg, + const Source& source) const { + auto severity = diagnostic_filters_.Get(rule); + if (severity != ast::DiagnosticSeverity::kOff) { + diag::Diagnostic d{}; + d.severity = ToSeverity(severity); + d.system = diag::System::Resolver; + d.source = source; + d.message = msg; + diagnostics_.add(std::move(d)); + if (severity == ast::DiagnosticSeverity::kError) { + return false; + } + } + return true; +} + // https://gpuweb.github.io/gpuweb/wgsl/#plain-types-section bool Validator::IsPlain(const type::Type* type) const { return type->is_scalar() || @@ -1358,9 +1380,10 @@ bool Validator::EvaluationStage(const sem::Expression* expr, bool Validator::Statements(utils::VectorRef stmts) const { for (auto* stmt : stmts) { if (!sem_.Get(stmt)->IsReachable()) { - /// TODO(https://github.com/gpuweb/gpuweb/issues/2378): This may need to - /// become an error. - AddWarning("code is unreachable", stmt->source); + if (!AddDiagnostic(ast::DiagnosticRule::kChromiumUnreachableCode, "code is unreachable", + stmt->source)) { + return false; + } break; } } diff --git a/src/tint/resolver/validator.h b/src/tint/resolver/validator.h index 891f02daa9..635a6ccfa7 100644 --- a/src/tint/resolver/validator.h +++ b/src/tint/resolver/validator.h @@ -22,6 +22,7 @@ #include "src/tint/ast/pipeline_stage.h" #include "src/tint/program_builder.h" #include "src/tint/resolver/sem_helper.h" +#include "src/tint/scope_stack.h" #include "src/tint/sem/evaluation_stage.h" #include "src/tint/source.h" #include "src/tint/utils/hash.h" @@ -84,6 +85,9 @@ struct TypeAndAddressSpace { } }; +/// DiagnosticFilterStack is a scoped stack of diagnostic filters. +using DiagnosticFilterStack = ScopeStack; + /// Validation logic for various ast nodes. The validations in general should /// be shallow and depend on the resolver to call on children. The validations /// also assume that sem changes have already been made. The validation checks @@ -118,6 +122,18 @@ class Validator { /// @param source the note source void AddNote(const std::string& msg, const Source& source) const; + /// Adds the given message to the diagnostics with current severity for the given rule. + /// @param rule the diagnostic trigger rule + /// @param msg the diagnostic message + /// @param source the diagnostic source + /// @returns false if the diagnostic is an error for the given trigger rule + bool AddDiagnostic(ast::DiagnosticRule rule, + const std::string& msg, + const Source& source) const; + + /// @returns the diagnostic filter stack + DiagnosticFilterStack& DiagnosticFilters() { return diagnostic_filters_; } + /// @param type the given type /// @returns true if the given type is a plain type bool IsPlain(const type::Type* type) const; @@ -525,6 +541,7 @@ class Validator { SymbolTable& symbols_; diag::List& diagnostics_; SemHelper& sem_; + DiagnosticFilterStack diagnostic_filters_; const ast::Extensions& enabled_extensions_; const utils::Hashmap& atomic_composite_info_; utils::Hashset& valid_type_storage_layouts_;