From 3a9a55eec6e8fa06d1c6b3201cc719b8295cd4ea Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Thu, 18 Feb 2021 16:33:38 +0000 Subject: [PATCH] ICE macros: Use '<<' for error message Lessens the friction of using these macros. Also allows you to add a message to TINT_UNREACHABLE(), which you couldn't do before. Change-Id: Ida4d63ec96e1d99af71503e8b80d7a5a712e6a47 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/42020 Commit-Queue: Ben Clayton Reviewed-by: dan sinclair --- src/ast/module.cc | 4 +-- src/clone_context.h | 2 +- src/debug.cc | 18 +++++----- src/debug.h | 59 +++++++++++++++++++++++-------- src/intrinsic_table.cc | 16 ++++----- src/transform/hlsl.cc | 5 ++- src/writer/hlsl/generator_impl.cc | 18 +++++----- src/writer/msl/generator_impl.cc | 4 +-- 8 files changed, 78 insertions(+), 48 deletions(-) diff --git a/src/ast/module.cc b/src/ast/module.cc index 996ecc91a7..91b8fdb327 100644 --- a/src/ast/module.cc +++ b/src/ast/module.cc @@ -45,7 +45,7 @@ Module::Module(const Source& source, std::vector global_decls) global_variables_.push_back(var); } else { diag::List diagnostics; - TINT_ICE(diagnostics, "Unknown global declaration type"); + TINT_ICE(diagnostics) << "Unknown global declaration type"; } } } @@ -108,7 +108,7 @@ void Module::Copy(CloneContext* ctx, const Module* src) { } else if (auto* var = decl->As()) { AddGlobalVariable(ctx->Clone(var)); } else { - TINT_ICE(ctx->dst->Diagnostics(), "Unknown global declaration type"); + TINT_ICE(ctx->dst->Diagnostics()) << "Unknown global declaration type"; } } } diff --git a/src/clone_context.h b/src/clone_context.h index a533bff267..e1b4da0205 100644 --- a/src/clone_context.h +++ b/src/clone_context.h @@ -307,7 +307,7 @@ class CloneContext { TO* CheckedCast(FROM* obj) { TO* cast = obj->template As(); if (!cast) { - TINT_ICE(Diagnostics(), "Cloned object was not of the expected type"); + TINT_ICE(Diagnostics()) << "Cloned object was not of the expected type"; } return cast; } diff --git a/src/debug.cc b/src/debug.cc index b6d9b0182a..c33c7de527 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -64,19 +64,21 @@ void SetInternalCompilerErrorReporter(InternalCompilerErrorReporter* reporter) { ice_reporter = reporter; } -void InternalCompilerError(const char* filepath, - size_t line, - const std::string& msg, - diag::List& diagnostics) { - auto* file = new Source::File(filepath, ""); +InternalCompilerError::InternalCompilerError(const char* file, + size_t line, + diag::List& diagnostics) + : file_(file), line_(line), diagnostics_(diagnostics) {} + +InternalCompilerError::~InternalCompilerError() { + auto* file = new Source::File(file_, ""); SourceFileToDelete::Get().Add(file); - Source source{Source::Range{Source::Location{line}}, file}; - diagnostics.add_ice(msg, source); + Source source{Source::Range{Source::Location{line_}}, file}; + diagnostics_.add_ice(msg_.str(), source); if (ice_reporter) { - ice_reporter(diagnostics); + ice_reporter(diagnostics_); } } diff --git a/src/debug.h b/src/debug.h index 3e7e7de6f6..ba3aed18e9 100644 --- a/src/debug.h +++ b/src/debug.h @@ -15,7 +15,8 @@ #ifndef SRC_DEBUG_H_ #define SRC_DEBUG_H_ -#include +#include +#include #include "src/diagnostic/diagnostic.h" #include "src/diagnostic/formatter.h" @@ -37,17 +38,42 @@ void FreeInternalCompilerErrors(); /// @param reporter the error reporter void SetInternalCompilerErrorReporter(InternalCompilerErrorReporter* reporter); -/// InternalCompilerError adds the internal compiler error message to the -/// diagnostics list, and then calls the InternalCompilerErrorReporter if one is -/// set. -/// @param file the file containing the ICE -/// @param line the line containing the ICE -/// @param msg the ICE message -/// @param diagnostics the list of diagnostics to append the ICE message to -void InternalCompilerError(const char* file, - size_t line, - const std::string& msg, - diag::List& diagnostics); +/// InternalCompilerError is a helper for reporting internal compiler errors. +/// Construct the InternalCompilerError with the source location of the ICE +/// fault and append any error details with the `<<` operator. +/// When the InternalCompilerError is destructed, the concatenated error message +/// is appended to the diagnostics list with the severity of +/// tint::diag::Severity::InternalCompilerError, and if a +/// InternalCompilerErrorReporter is set, then it is called with the diagnostic +/// list. +class InternalCompilerError { + public: + /// Constructor + /// @param file the file containing the ICE + /// @param line the line containing the ICE + /// @param diagnostics the list of diagnostics to append the ICE message to + InternalCompilerError(const char* file, size_t line, diag::List& diagnostics); + + /// Destructor. + /// Adds the internal compiler error message to the diagnostics list, and then + /// calls the InternalCompilerErrorReporter if one is set. + ~InternalCompilerError(); + + /// Appends `arg` to the ICE message. + /// @param arg the argument to append to the ICE message + /// @returns this object so calls can be chained + template + InternalCompilerError& operator<<(T&& arg) { + msg_ << std::forward(arg); + return *this; + } + + private: + char const* const file_; + size_t const line_; + diag::List& diagnostics_; + std::stringstream msg_; +}; } // namespace tint @@ -56,14 +82,17 @@ void InternalCompilerError(const char* file, /// InternalCompilerErrorReporter with the full diagnostic list if a reporter is /// set. /// The ICE message contains the callsite's file and line. -#define TINT_ICE(diagnostics, msg) \ - tint::InternalCompilerError(__FILE__, __LINE__, msg, diagnostics) +/// Use the `<<` operator to append an error message to the ICE. +#define TINT_ICE(diagnostics) \ + tint::InternalCompilerError(__FILE__, __LINE__, diagnostics) /// TINT_UNREACHABLE() is a macro for appending a "TINT_UNREACHABLE" /// internal compiler error message to the diagnostics list `diagnostics`, and /// calling the InternalCompilerErrorReporter with the full diagnostic list if a /// reporter is set. /// The ICE message contains the callsite's file and line. -#define TINT_UNREACHABLE(diagnostics) TINT_ICE(diagnostics, "TINT_UNREACHABLE") +/// Use the `<<` operator to append an error message to the ICE. +#define TINT_UNREACHABLE(diagnostics) \ + TINT_ICE(diagnostics) << "TINT_UNREACHABLE " #endif // SRC_DEBUG_H_ diff --git a/src/intrinsic_table.cc b/src/intrinsic_table.cc index 04757218ec..34550b1e86 100644 --- a/src/intrinsic_table.cc +++ b/src/intrinsic_table.cc @@ -1448,20 +1448,20 @@ semantic::Intrinsic* Impl::Overload::Match(ProgramBuilder& builder, if (type_it == matcher_state.open_types.end()) { // We have an overload that claims to have matched, but didn't actually // resolve the open type. This is a bug that needs fixing. - TINT_ICE(diagnostics, "IntrinsicTable overload matched for " + - CallSignature(builder, intrinsic, args) + - ", but didn't resolve the open type " + - str(open_type)); + TINT_ICE(diagnostics) + << "IntrinsicTable overload matched for " + << CallSignature(builder, intrinsic, args) + << ", but didn't resolve the open type " << str(open_type); return nullptr; } auto* resolved_type = type_it->second; if (resolved_type == nullptr) { // We have an overload that claims to have matched, but has a nullptr // resolved open type. This is a bug that needs fixing. - TINT_ICE(diagnostics, "IntrinsicTable overload matched for " + - CallSignature(builder, intrinsic, args) + - ", but open type " + str(open_type) + - " is nullptr"); + TINT_ICE(diagnostics) + << "IntrinsicTable overload matched for " + << CallSignature(builder, intrinsic, args) << ", but open type " + << str(open_type) << " is nullptr"; return nullptr; } if (!matcher->Match(matcher_state, resolved_type)) { diff --git a/src/transform/hlsl.cc b/src/transform/hlsl.cc index b0ce373c1b..af67bcc383 100644 --- a/src/transform/hlsl.cc +++ b/src/transform/hlsl.cc @@ -57,9 +57,8 @@ void Hlsl::PromoteArrayInitializerToConstVar(CloneContext& ctx) const { if (auto* src_init = src_node->As()) { auto* src_sem_expr = ctx.src->Sem().Get(src_init); if (!src_sem_expr) { - TINT_ICE( - ctx.dst->Diagnostics(), - "ast::TypeConstructorExpression has no semantic expression node"); + TINT_ICE(ctx.dst->Diagnostics()) + << "ast::TypeConstructorExpression has no semantic expression node"; continue; } auto* src_sem_stmt = src_sem_expr->Stmt(); diff --git a/src/writer/hlsl/generator_impl.cc b/src/writer/hlsl/generator_impl.cc index d8ee19d0db..9a60d21f3f 100644 --- a/src/writer/hlsl/generator_impl.cc +++ b/src/writer/hlsl/generator_impl.cc @@ -799,7 +799,7 @@ bool GeneratorImpl::EmitTextureCall(std::ostream& pre, case semantic::IntrinsicType::kTextureDimensions: switch (texture_type->dim()) { case type::TextureDimension::kNone: - diagnostics_.add_error("texture dimension is kNone"); + TINT_ICE(diagnostics_) << "texture dimension is kNone"; return false; case type::TextureDimension::k1d: num_dimensions = 1; @@ -835,7 +835,7 @@ bool GeneratorImpl::EmitTextureCall(std::ostream& pre, case semantic::IntrinsicType::kTextureNumLayers: switch (texture_type->dim()) { default: - diagnostics_.add_error("texture dimension is not arrayed"); + TINT_ICE(diagnostics_) << "texture dimension is not arrayed"; return false; case type::TextureDimension::k1dArray: num_dimensions = 2; @@ -852,7 +852,8 @@ bool GeneratorImpl::EmitTextureCall(std::ostream& pre, add_mip_level_in = true; switch (texture_type->dim()) { default: - diagnostics_.add_error("texture dimension does not support mips"); + TINT_ICE(diagnostics_) + << "texture dimension does not support mips"; return false; case type::TextureDimension::k2d: case type::TextureDimension::kCube: @@ -870,8 +871,8 @@ bool GeneratorImpl::EmitTextureCall(std::ostream& pre, case semantic::IntrinsicType::kTextureNumSamples: switch (texture_type->dim()) { default: - diagnostics_.add_error( - "texture dimension does not support multisampling"); + TINT_ICE(diagnostics_) + << "texture dimension does not support multisampling"; return false; case type::TextureDimension::k2d: num_dimensions = 3; @@ -884,7 +885,7 @@ bool GeneratorImpl::EmitTextureCall(std::ostream& pre, } break; default: - diagnostics_.add_error("unexpected intrinsic"); + TINT_ICE(diagnostics_) << "unexpected intrinsic"; return false; } @@ -2468,9 +2469,8 @@ bool GeneratorImpl::EmitType(std::ostream& out, if (auto* st = tex->As()) { auto* component = image_format_to_rwtexture_type(st->image_format()); if (component == nullptr) { - diagnostics_.add_error( - "Unsupported StorageTexture ImageFormat: " + - std::to_string(static_cast(st->image_format()))); + TINT_ICE(diagnostics_) << "Unsupported StorageTexture ImageFormat: " + << static_cast(st->image_format()); return false; } out << "<" << component << ">"; diff --git a/src/writer/msl/generator_impl.cc b/src/writer/msl/generator_impl.cc index ac835bdab5..f21598f1b1 100644 --- a/src/writer/msl/generator_impl.cc +++ b/src/writer/msl/generator_impl.cc @@ -699,8 +699,8 @@ bool GeneratorImpl::EmitTextureCall(ast::CallExpression* expr, out_ << ".write("; break; default: - TINT_ICE(diagnostics_, "Unhandled texture intrinsic '" + - std::string(intrinsic->str()) + "'"); + TINT_UNREACHABLE(diagnostics_) + << "Unhandled texture intrinsic '" << intrinsic->str() << "'"; return false; }