From c4b5cc94ad69e4fd5c489b539276370e049764fb Mon Sep 17 00:00:00 2001 From: wayne Date: Tue, 23 Jan 2024 15:25:25 +0000 Subject: [PATCH] don't panic if naga parsing of shader source fails (#5034) * naga: glsl parser should return singular ParseError similar to wgsl * wgpu: treat glsl the same as wgsl when creating ShaderModule * naga: update glsl parser tests to use new ParseError type * naga: glsl ParseError errors field should be public * wgpu-core: add 'glsl' feature * fix some minor bugs in glsl parse error refactor * naga/wgpu/wgpu-core: improve spirv parse error handling * wgpu-core: feature gate use of glsl and spv naga modules * wgpu: enable wgpu-core glsl and spirv features when appropriate * obey clippy * naga: derive Clone in Type * naga: don't feature gate Clone derivation for Type * obey cargo fmt * wgpu-core: use bytemuck instead of zerocopy * wgpu-core: apply suggested edit * wgpu-core: no need to borrow spirv code * Update wgpu/src/backend/wgpu_core.rs Co-authored-by: Alphyr <47725341+a1phyr@users.noreply.github.com> --------- Co-authored-by: Alphyr <47725341+a1phyr@users.noreply.github.com> --- Cargo.lock | 1 + naga-cli/src/bin/naga.rs | 23 ++------ naga/src/front/glsl/error.rs | 63 +++++++++++++++++++- naga/src/front/glsl/mod.rs | 8 +-- naga/src/front/glsl/parser_tests.rs | 90 +++++++++++++++++------------ naga/src/front/glsl/token.rs | 2 +- naga/src/front/spv/error.rs | 25 ++++++++ naga/src/lib.rs | 6 +- wgpu-core/Cargo.toml | 7 +++ wgpu-core/src/device/global.rs | 8 +++ wgpu-core/src/device/resource.rs | 28 ++++++++- wgpu-core/src/pipeline.rs | 26 +++++++++ wgpu/Cargo.toml | 4 +- wgpu/src/backend/wgpu_core.rs | 10 +--- 14 files changed, 221 insertions(+), 80 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c493eb6a8..0a0500838 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3991,6 +3991,7 @@ dependencies = [ "arrayvec 0.7.4", "bit-vec", "bitflags 2.4.2", + "bytemuck", "cfg_aliases", "codespan-reporting", "indexmap", diff --git a/naga-cli/src/bin/naga.rs b/naga-cli/src/bin/naga.rs index 8960866b3..7a3a0f260 100644 --- a/naga-cli/src/bin/naga.rs +++ b/naga-cli/src/bin/naga.rs @@ -488,9 +488,10 @@ fn parse_input( }, &input, ) - .unwrap_or_else(|errors| { - let filename = input_path.file_name().and_then(std::ffi::OsStr::to_str); - emit_glsl_parser_error(errors, filename.unwrap_or("glsl"), &input); + .unwrap_or_else(|error| { + let filename = input_path.file_name().and_then(std::ffi::OsStr::to_str).unwrap_or("glsl"); + let mut writer = StandardStream::stderr(ColorChoice::Auto); + error.emit_to_writer_with_path(&mut writer, &input, filename); std::process::exit(1); }), Some(input), @@ -719,22 +720,6 @@ use codespan_reporting::{ }; use naga::WithSpan; -pub fn emit_glsl_parser_error(errors: Vec, filename: &str, source: &str) { - let files = SimpleFile::new(filename, source); - let config = codespan_reporting::term::Config::default(); - let writer = StandardStream::stderr(ColorChoice::Auto); - - for err in errors { - let mut diagnostic = Diagnostic::error().with_message(err.kind.to_string()); - - if let Some(range) = err.meta.to_range() { - diagnostic = diagnostic.with_labels(vec![Label::primary((), range)]); - } - - term::emit(&mut writer.lock(), &config, &files, &diagnostic).expect("cannot write error"); - } -} - pub fn emit_annotated_error(ann_err: &WithSpan, filename: &str, source: &str) { let files = SimpleFile::new(filename, source); let config = codespan_reporting::term::Config::default(); diff --git a/naga/src/front/glsl/error.rs b/naga/src/front/glsl/error.rs index d6e358668..bd16ee30b 100644 --- a/naga/src/front/glsl/error.rs +++ b/naga/src/front/glsl/error.rs @@ -1,7 +1,11 @@ use super::token::TokenValue; use crate::{proc::ConstantEvaluatorError, Span}; +use codespan_reporting::diagnostic::{Diagnostic, Label}; +use codespan_reporting::files::SimpleFile; +use codespan_reporting::term; use pp_rs::token::PreprocessorError; use std::borrow::Cow; +use termcolor::{NoColor, WriteColor}; use thiserror::Error; fn join_with_comma(list: &[ExpectedToken]) -> String { @@ -18,7 +22,7 @@ fn join_with_comma(list: &[ExpectedToken]) -> String { } /// One of the expected tokens returned in [`InvalidToken`](ErrorKind::InvalidToken). -#[derive(Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub enum ExpectedToken { /// A specific token was expected. Token(TokenValue), @@ -55,7 +59,7 @@ impl std::fmt::Display for ExpectedToken { } /// Information about the cause of an error. -#[derive(Debug, Error)] +#[derive(Clone, Debug, Error)] #[cfg_attr(test, derive(PartialEq))] pub enum ErrorKind { /// Whilst parsing as encountered an unexpected EOF. @@ -123,7 +127,7 @@ impl From for ErrorKind { } /// Error returned during shader parsing. -#[derive(Debug, Error)] +#[derive(Clone, Debug, Error)] #[error("{kind}")] #[cfg_attr(test, derive(PartialEq))] pub struct Error { @@ -132,3 +136,56 @@ pub struct Error { /// Holds information about the range of the source code where the error happened. pub meta: Span, } + +/// A collection of errors returned during shader parsing. +#[derive(Clone, Debug)] +#[cfg_attr(test, derive(PartialEq))] +pub struct ParseError { + pub errors: Vec, +} + +impl ParseError { + pub fn emit_to_writer(&self, writer: &mut impl WriteColor, source: &str) { + self.emit_to_writer_with_path(writer, source, "glsl"); + } + + pub fn emit_to_writer_with_path(&self, writer: &mut impl WriteColor, source: &str, path: &str) { + let path = path.to_string(); + let files = SimpleFile::new(path, source); + let config = codespan_reporting::term::Config::default(); + + for err in &self.errors { + let mut diagnostic = Diagnostic::error().with_message(err.kind.to_string()); + + if let Some(range) = err.meta.to_range() { + diagnostic = diagnostic.with_labels(vec![Label::primary((), range)]); + } + + term::emit(writer, &config, &files, &diagnostic).expect("cannot write error"); + } + } + + pub fn emit_to_string(&self, source: &str) -> String { + let mut writer = NoColor::new(Vec::new()); + self.emit_to_writer(&mut writer, source); + String::from_utf8(writer.into_inner()).unwrap() + } +} + +impl std::fmt::Display for ParseError { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + self.errors.iter().try_for_each(|e| write!(f, "{e:?}")) + } +} + +impl std::error::Error for ParseError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + None + } +} + +impl From> for ParseError { + fn from(errors: Vec) -> Self { + Self { errors } + } +} diff --git a/naga/src/front/glsl/mod.rs b/naga/src/front/glsl/mod.rs index 49624a943..395111d97 100644 --- a/naga/src/front/glsl/mod.rs +++ b/naga/src/front/glsl/mod.rs @@ -13,7 +13,7 @@ To begin, take a look at the documentation for the [`Frontend`]. */ pub use ast::{Precision, Profile}; -pub use error::{Error, ErrorKind, ExpectedToken}; +pub use error::{Error, ErrorKind, ExpectedToken, ParseError}; pub use token::TokenValue; use crate::{proc::Layouter, FastHashMap, FastHashSet, Handle, Module, ShaderStage, Span, Type}; @@ -196,7 +196,7 @@ impl Frontend { &mut self, options: &Options, source: &str, - ) -> std::result::Result> { + ) -> std::result::Result { self.reset(options.stage); let lexer = lex::Lexer::new(source, &options.defines); @@ -207,12 +207,12 @@ impl Frontend { if self.errors.is_empty() { Ok(module) } else { - Err(std::mem::take(&mut self.errors)) + Err(std::mem::take(&mut self.errors).into()) } } Err(e) => { self.errors.push(e); - Err(std::mem::take(&mut self.errors)) + Err(std::mem::take(&mut self.errors).into()) } } } diff --git a/naga/src/front/glsl/parser_tests.rs b/naga/src/front/glsl/parser_tests.rs index 0f4fbab22..259052cd2 100644 --- a/naga/src/front/glsl/parser_tests.rs +++ b/naga/src/front/glsl/parser_tests.rs @@ -1,7 +1,7 @@ use super::{ ast::Profile, error::ExpectedToken, - error::{Error, ErrorKind}, + error::{Error, ErrorKind, ParseError}, token::TokenValue, Frontend, Options, Span, }; @@ -21,10 +21,12 @@ fn version() { ) .err() .unwrap(), - vec![Error { - kind: ErrorKind::InvalidVersion(99000), - meta: Span::new(9, 14) - }], + ParseError { + errors: vec![Error { + kind: ErrorKind::InvalidVersion(99000), + meta: Span::new(9, 14) + }], + }, ); assert_eq!( @@ -35,10 +37,12 @@ fn version() { ) .err() .unwrap(), - vec![Error { - kind: ErrorKind::InvalidVersion(449), - meta: Span::new(9, 12) - }] + ParseError { + errors: vec![Error { + kind: ErrorKind::InvalidVersion(449), + meta: Span::new(9, 12) + }] + }, ); assert_eq!( @@ -49,10 +53,12 @@ fn version() { ) .err() .unwrap(), - vec![Error { - kind: ErrorKind::InvalidProfile("smart".into()), - meta: Span::new(13, 18), - }] + ParseError { + errors: vec![Error { + kind: ErrorKind::InvalidProfile("smart".into()), + meta: Span::new(13, 18), + }] + }, ); assert_eq!( @@ -63,19 +69,21 @@ fn version() { ) .err() .unwrap(), - vec![ - Error { - kind: ErrorKind::PreprocessorError(PreprocessorError::UnexpectedHash,), - meta: Span::new(27, 28), - }, - Error { - kind: ErrorKind::InvalidToken( - TokenValue::Identifier("version".into()), - vec![ExpectedToken::Eof] - ), - meta: Span::new(28, 35) - } - ] + ParseError { + errors: vec![ + Error { + kind: ErrorKind::PreprocessorError(PreprocessorError::UnexpectedHash,), + meta: Span::new(27, 28), + }, + Error { + kind: ErrorKind::InvalidToken( + TokenValue::Identifier("version".into()), + vec![ExpectedToken::Eof] + ), + meta: Span::new(28, 35) + } + ] + }, ); // valid versions @@ -447,10 +455,12 @@ fn functions() { ) .err() .unwrap(), - vec![Error { - kind: ErrorKind::SemanticError("Function already defined".into()), - meta: Span::new(134, 152), - }] + ParseError { + errors: vec![Error { + kind: ErrorKind::SemanticError("Function already defined".into()), + meta: Span::new(134, 152), + }] + }, ); println!(); @@ -626,10 +636,12 @@ fn implicit_conversions() { ) .err() .unwrap(), - vec![Error { - kind: ErrorKind::SemanticError("Unknown function \'test\'".into()), - meta: Span::new(156, 165), - }] + ParseError { + errors: vec![Error { + kind: ErrorKind::SemanticError("Unknown function \'test\'".into()), + meta: Span::new(156, 165), + }] + }, ); assert_eq!( @@ -648,10 +660,12 @@ fn implicit_conversions() { ) .err() .unwrap(), - vec![Error { - kind: ErrorKind::SemanticError("Ambiguous best function for \'test\'".into()), - meta: Span::new(158, 165), - }] + ParseError { + errors: vec![Error { + kind: ErrorKind::SemanticError("Ambiguous best function for \'test\'".into()), + meta: Span::new(158, 165), + }] + } ); } diff --git a/naga/src/front/glsl/token.rs b/naga/src/front/glsl/token.rs index 4c3b5d4a2..303723a27 100644 --- a/naga/src/front/glsl/token.rs +++ b/naga/src/front/glsl/token.rs @@ -20,7 +20,7 @@ pub struct Token { /// /// This type is exported since it's returned in the /// [`InvalidToken`](super::ErrorKind::InvalidToken) error. -#[derive(Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub enum TokenValue { Identifier(String), diff --git a/naga/src/front/spv/error.rs b/naga/src/front/spv/error.rs index 2f9bf2d1b..af025636c 100644 --- a/naga/src/front/spv/error.rs +++ b/naga/src/front/spv/error.rs @@ -1,5 +1,9 @@ use super::ModuleState; use crate::arena::Handle; +use codespan_reporting::diagnostic::Diagnostic; +use codespan_reporting::files::SimpleFile; +use codespan_reporting::term; +use termcolor::{NoColor, WriteColor}; #[derive(Debug, thiserror::Error)] pub enum Error { @@ -127,3 +131,24 @@ pub enum Error { )] NonBindingArrayOfImageOrSamplers, } + +impl Error { + pub fn emit_to_writer(&self, writer: &mut impl WriteColor, source: &str) { + self.emit_to_writer_with_path(writer, source, "glsl"); + } + + pub fn emit_to_writer_with_path(&self, writer: &mut impl WriteColor, source: &str, path: &str) { + let path = path.to_string(); + let files = SimpleFile::new(path, source); + let config = codespan_reporting::term::Config::default(); + let diagnostic = Diagnostic::error().with_message(format!("{self:?}")); + + term::emit(writer, &config, &files, &diagnostic).expect("cannot write error"); + } + + pub fn emit_to_string(&self, source: &str) -> String { + let mut writer = NoColor::new(Vec::new()); + self.emit_to_writer(&mut writer, source); + String::from_utf8(writer.into_inner()).unwrap() + } +} diff --git a/naga/src/lib.rs b/naga/src/lib.rs index bfd8359d8..5c63e7db4 100644 --- a/naga/src/lib.rs +++ b/naga/src/lib.rs @@ -687,8 +687,7 @@ pub enum ImageClass { } /// A data type declared in the module. -#[derive(Debug, Eq, Hash, PartialEq)] -#[cfg_attr(feature = "clone", derive(Clone))] +#[derive(Clone, Debug, Eq, Hash, PartialEq)] #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] @@ -700,8 +699,7 @@ pub struct Type { } /// Enum with additional information, depending on the kind of type. -#[derive(Debug, Eq, Hash, PartialEq)] -#[cfg_attr(feature = "clone", derive(Clone))] +#[derive(Clone, Debug, Eq, Hash, PartialEq)] #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] diff --git a/wgpu-core/Cargo.toml b/wgpu-core/Cargo.toml index 28edac35b..95507bd7e 100644 --- a/wgpu-core/Cargo.toml +++ b/wgpu-core/Cargo.toml @@ -59,6 +59,12 @@ serial-pass = ["serde", "wgt/serde", "arrayvec/serde"] ## Enable `ShaderModuleSource::Wgsl` wgsl = ["naga/wgsl-in"] +## Enable `ShaderModuleSource::Glsl` +glsl = ["naga/glsl-in"] + +## Enable `ShaderModuleSource::SpirV` +spirv = ["naga/spv-in", "dep:bytemuck"] + ## Implement `Send` and `Sync` on Wasm, but only if atomics are not enabled. ## ## WebGL/WebGPU objects can not be shared between threads. @@ -93,6 +99,7 @@ dx12 = ["hal/dx12"] arrayvec = "0.7" bit-vec = "0.6" bitflags = "2" +bytemuck = { version = "1.14", optional = true } codespan-reporting = "0.11" indexmap = "2" log = "0.4" diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index f7cfc1da1..54d8ed61d 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -1189,6 +1189,14 @@ impl Global { pipeline::ShaderModuleSource::Wgsl(ref code) => { trace.make_binary("wgsl", code.as_bytes()) } + #[cfg(feature = "glsl")] + pipeline::ShaderModuleSource::Glsl(ref code, _) => { + trace.make_binary("glsl", code.as_bytes()) + } + #[cfg(feature = "spirv")] + pipeline::ShaderModuleSource::SpirV(ref code, _) => { + trace.make_binary("spirv", bytemuck::cast_slice::(code)) + } pipeline::ShaderModuleSource::Naga(ref module) => { let string = ron::ser::to_string_pretty(module, ron::ser::PrettyConfig::default()) diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index 352398156..b5de57f80 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -1309,7 +1309,7 @@ impl Device { let (module, source) = match source { #[cfg(feature = "wgsl")] pipeline::ShaderModuleSource::Wgsl(code) => { - profiling::scope!("naga::wgsl::parse_str"); + profiling::scope!("naga::front::wgsl::parse_str"); let module = naga::front::wgsl::parse_str(&code).map_err(|inner| { pipeline::CreateShaderModuleError::Parsing(pipeline::ShaderError { source: code.to_string(), @@ -1319,6 +1319,32 @@ impl Device { })?; (Cow::Owned(module), code.into_owned()) } + #[cfg(feature = "spirv")] + pipeline::ShaderModuleSource::SpirV(spv, options) => { + let parser = naga::front::spv::Frontend::new(spv.iter().cloned(), &options); + profiling::scope!("naga::front::spv::Frontend"); + let module = parser.parse().map_err(|inner| { + pipeline::CreateShaderModuleError::ParsingSpirV(pipeline::ShaderError { + source: String::new(), + label: desc.label.as_ref().map(|l| l.to_string()), + inner: Box::new(inner), + }) + })?; + (Cow::Owned(module), String::new()) + } + #[cfg(feature = "glsl")] + pipeline::ShaderModuleSource::Glsl(code, options) => { + let mut parser = naga::front::glsl::Frontend::default(); + profiling::scope!("naga::front::glsl::Frontend.parse"); + let module = parser.parse(&options, &code).map_err(|inner| { + pipeline::CreateShaderModuleError::ParsingGlsl(pipeline::ShaderError { + source: code.to_string(), + label: desc.label.as_ref().map(|l| l.to_string()), + inner: Box::new(inner), + }) + })?; + (Cow::Owned(module), code.into_owned()) + } pipeline::ShaderModuleSource::Naga(module) => (module, String::new()), pipeline::ShaderModuleSource::Dummy(_) => panic!("found `ShaderModuleSource::Dummy`"), }; diff --git a/wgpu-core/src/pipeline.rs b/wgpu-core/src/pipeline.rs index 6e2998235..1dc2d1eff 100644 --- a/wgpu-core/src/pipeline.rs +++ b/wgpu-core/src/pipeline.rs @@ -26,6 +26,10 @@ pub(crate) struct LateSizedBufferGroup { pub enum ShaderModuleSource<'a> { #[cfg(feature = "wgsl")] Wgsl(Cow<'a, str>), + #[cfg(feature = "glsl")] + Glsl(Cow<'a, str>, naga::front::glsl::Options), + #[cfg(feature = "spirv")] + SpirV(Cow<'a, [u32]>, naga::front::spv::Options), Naga(Cow<'static, naga::Module>), /// Dummy variant because `Naga` doesn't have a lifetime and without enough active features it /// could be the last one active. @@ -103,6 +107,22 @@ impl fmt::Display for ShaderError { write!(f, "\nShader '{label}' parsing {string}") } } +#[cfg(feature = "glsl")] +impl fmt::Display for ShaderError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let label = self.label.as_deref().unwrap_or_default(); + let string = self.inner.emit_to_string(&self.source); + write!(f, "\nShader '{label}' parsing {string}") + } +} +#[cfg(feature = "spirv")] +impl fmt::Display for ShaderError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let label = self.label.as_deref().unwrap_or_default(); + let string = self.inner.emit_to_string(&self.source); + write!(f, "\nShader '{label}' parsing {string}") + } +} impl fmt::Display for ShaderError> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { use codespan_reporting::{ @@ -151,6 +171,12 @@ pub enum CreateShaderModuleError { #[cfg(feature = "wgsl")] #[error(transparent)] Parsing(#[from] ShaderError), + #[cfg(feature = "glsl")] + #[error(transparent)] + ParsingGlsl(#[from] ShaderError), + #[cfg(feature = "spirv")] + #[error(transparent)] + ParsingSpirV(#[from] ShaderError), #[error("Failed to generate the backend-specific code")] Generation, #[error(transparent)] diff --git a/wgpu/Cargo.toml b/wgpu/Cargo.toml index 813bc71f5..cc97e4d24 100644 --- a/wgpu/Cargo.toml +++ b/wgpu/Cargo.toml @@ -56,10 +56,10 @@ webgl = ["hal", "wgc/gles"] # -------------------------------------------------------------------- ## Enable accepting SPIR-V shaders as input. -spirv = ["naga/spv-in"] +spirv = ["naga/spv-in", "wgc/spirv"] ## Enable accepting GLSL shaders as input. -glsl = ["naga/glsl-in"] +glsl = ["naga/glsl-in", "wgc/glsl"] ## Enable accepting WGSL shaders as input. wgsl = ["wgc?/wgsl"] diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index ddc133dd1..62f619d64 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -844,9 +844,7 @@ impl crate::Context for ContextWgpuCore { strict_capabilities: true, block_ctx_dump_prefix: None, }; - let parser = naga::front::spv::Frontend::new(spv.iter().cloned(), &options); - let module = parser.parse().unwrap(); - wgc::pipeline::ShaderModuleSource::Naga(Owned(module)) + wgc::pipeline::ShaderModuleSource::SpirV(Borrowed(spv), options) } #[cfg(feature = "glsl")] ShaderSource::Glsl { @@ -854,12 +852,8 @@ impl crate::Context for ContextWgpuCore { stage, defines, } => { - // Parse the given shader code and store its representation. let options = naga::front::glsl::Options { stage, defines }; - let mut parser = naga::front::glsl::Frontend::default(); - let module = parser.parse(&options, shader).unwrap(); - - wgc::pipeline::ShaderModuleSource::Naga(Owned(module)) + wgc::pipeline::ShaderModuleSource::Glsl(Borrowed(shader), options) } #[cfg(feature = "wgsl")] ShaderSource::Wgsl(ref code) => wgc::pipeline::ShaderModuleSource::Wgsl(Borrowed(code)),