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

@@ -3991,6 +3991,7 @@ dependencies = [
  "arrayvec 0.7.4",
  "bit-vec",
  "bitflags 2.4.2",
+ "bytemuck",
  "cfg_aliases",
  "codespan-reporting",
  "indexmap",

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<naga::front::glsl::Error>, 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<E: Error>(ann_err: &WithSpan<E>, filename: &str, source: &str) {
     let files = SimpleFile::new(filename, source);
     let config = codespan_reporting::term::Config::default();

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<ConstantEvaluatorError> 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<Error>,
+}
+
+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<Vec<Error>> for ParseError {
+    fn from(errors: Vec<Error>) -> Self {
+        Self { errors }
+    }
+}

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<Module, Vec<Error>> {
+    ) -> std::result::Result<Module, ParseError> {
         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())
             }
         }
     }

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),
+            }]
+        }
     );
 }
 

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),
 

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()
+    }
+}

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))]

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"

wgpu-core/src/device/global.rs

@@ -1189,6 +1189,14 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                     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::<u32, u8>(code))
+                    }
                     pipeline::ShaderModuleSource::Naga(ref module) => {
                         let string =
                             ron::ser::to_string_pretty(module, ron::ser::PrettyConfig::default())

wgpu-core/src/device/resource.rs

@@ -1309,7 +1309,7 @@ impl<A: HalApi> Device<A> {
         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<A: HalApi> Device<A> {
                 })?;
                 (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`"),
         };

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<naga::front::wgsl::ParseError> {
         write!(f, "\nShader '{label}' parsing {string}")
     }
 }
+#[cfg(feature = "glsl")]
+impl fmt::Display for ShaderError<naga::front::glsl::ParseError> {
+    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<naga::front::spv::Error> {
+    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<naga::WithSpan<naga::valid::ValidationError>> {
     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<naga::front::wgsl::ParseError>),
+    #[cfg(feature = "glsl")]
+    #[error(transparent)]
+    ParsingGlsl(#[from] ShaderError<naga::front::glsl::ParseError>),
+    #[cfg(feature = "spirv")]
+    #[error(transparent)]
+    ParsingSpirV(#[from] ShaderError<naga::front::spv::Error>),
     #[error("Failed to generate the backend-specific code")]
     Generation,
     #[error(transparent)]

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"]

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)),