[naga] Forbid cycles between global expressions and types.

naga/src/valid/handles.rs

@@ -44,36 +44,89 @@ impl super::Validator {
             ref diagnostic_filter_leaf,
         } = module;
 
-        // NOTE: Types being first is important. All other forms of validation depend on this.
-        for (this_handle, ty) in types.iter() {
-            match ty.inner {
-                crate::TypeInner::Scalar { .. }
-                | crate::TypeInner::Vector { .. }
-                | crate::TypeInner::Matrix { .. }
-                | crate::TypeInner::ValuePointer { .. }
-                | crate::TypeInner::Atomic { .. }
-                | crate::TypeInner::Image { .. }
-                | crate::TypeInner::Sampler { .. }
-                | crate::TypeInner::AccelerationStructure
-                | crate::TypeInner::RayQuery => (),
-                crate::TypeInner::Pointer { base, space: _ } => {
-                    this_handle.check_dep(base)?;
-                }
-                crate::TypeInner::Array { base, .. }
-                | crate::TypeInner::BindingArray { base, .. } => {
-                    this_handle.check_dep(base)?;
-                }
-                crate::TypeInner::Struct {
-                    ref members,
-                    span: _,
-                } => {
-                    this_handle.check_dep_iter(members.iter().map(|m| m.ty))?;
+        // Because types can refer to global expressions and vice versa, to
+        // ensure the overall structure is free of cycles, we must traverse them
+        // both in tandem.
+        //
+        // Try to visit all types and global expressions in an order such that
+        // each item refers only to previously visited items. If we succeed,
+        // that shows that there cannot be any cycles, since walking any edge
+        // advances you towards the beginning of the visiting order.
+        //
+        // Validate all the handles in types and expressions as we traverse the
+        // arenas.
+        let mut global_exprs_iter = global_expressions.iter().peekable();
+        for (th, t) in types.iter() {
+            // Imagine the `for` loop and `global_exprs_iter` as two fingers
+            // walking the type and global expression arenas. They don't visit
+            // elements at the same rate: sometimes one processes a bunch of
+            // elements while the other one stays still. But at each point, they
+            // check that the two ranges of elements they've visited only refer
+            // to other elements in those ranges.
+            //
+            // For brevity, we'll say 'handles behind `global_exprs_iter`' to
+            // mean handles that have already been produced by
+            // `global_exprs_iter`. Once `global_exprs_iter` returns `None`, all
+            // global expression handles are 'behind' it.
+            //
+            // At this point:
+            //
+            // - All types visited by prior iterations (that is, before
+            //   `th`/`t`) refer only to expressions behind `global_exprs_iter`.
+            //
+            //   On the first iteration, this is obviously true: there are no
+            //   prior iterations, and `global_exprs_iter` hasn't produced
+            //   anything yet. At the bottom of the loop, we'll claim that it's
+            //   true for `th`/`t` as well, so the condition remains true when
+            //   we advance to the next type.
+            //
+            // - All expressions behind `global_exprs_iter` refer only to
+            //   previously visited types.
+            //
+            //   Again, trivially true at the start, and we'll show it's true
+            //   about each expression that `global_exprs_iter` produces.
+            //
+            // Once we also check that arena elements only refer to prior
+            // elements in that arena, we can see that `th`/`t` does not
+            // participate in a cycle: it only refers to previously visited
+            // types and expressions behind `global_exprs_iter`, and none of
+            // those refer to `th`/`t`, because they passed the same checks
+            // before we reached `th`/`t`.
+            if let Some(max_expr) = Self::validate_type_handles((th, t))? {
+                max_expr.check_valid_for(global_expressions)?;
+                // Since `t` refers to `max_expr`, if we want our invariants to
+                // remain true, we must advance `global_exprs_iter` beyond
+                // `max_expr`.
+                while let Some((eh, e)) = global_exprs_iter.next_if(|&(eh, _)| eh <= max_expr) {
+                    if let Some(max_type) =
+                        Self::validate_const_expression_handles((eh, e), constants, overrides)?
+                    {
+                        // Show that `eh` refers only to previously visited types.
+                        th.check_dep(max_type)?;
+                    }
+                    // We've advanced `global_exprs_iter` past `eh` already. But
+                    // since we now know that `eh` refers only to previously
+                    // visited types, it is again true that all expressions
+                    // behind `global_exprs_iter` refer only to previously
+                    // visited types. So we can continue to the next expression.
                 }
             }
+
+            // Here we know that if `th` refers to any expressions at all,
+            // `max_expr` is the latest one. And we know that `max_expr` is
+            // behind `global_exprs_iter`. So `th` refers only to expressions
+            // behind `global_exprs_iter`, and the invariants will still be
+            // true on the next iteration.
         }
 
-        for handle_and_expr in global_expressions.iter() {
-            Self::validate_const_expression_handles(handle_and_expr, constants, overrides, types)?;
+        // Since we also enforced the usual intra-arena rules that expressions
+        // refer only to prior expressions, expressions can only form cycles if
+        // they include types. But we've shown that all types are acyclic, so
+        // all expressions must be acyclic as well.
+        //
+        // Validate the remaining expressions normally.
+        for handle_and_expr in global_exprs_iter {
+            Self::validate_const_expression_handles(handle_and_expr, constants, overrides)?;
         }
 
         let validate_type = |handle| Self::validate_type_handle(handle, types);
@@ -239,38 +292,86 @@ impl super::Validator {
         handle.check_valid_for(functions).map(|_| ())
     }
 
+    /// Validate all handles that occur in `ty`, whose handle is `handle`.
+    ///
+    /// If `ty` refers to any expressions, return the highest-indexed expression
+    /// handle that it uses. This is used for detecting cycles between the
+    /// expression and type arenas.
+    fn validate_type_handles(
+        (handle, ty): (Handle<crate::Type>, &crate::Type),
+    ) -> Result<Option<Handle<crate::Expression>>, InvalidHandleError> {
+        let max_expr = match ty.inner {
+            crate::TypeInner::Scalar { .. }
+            | crate::TypeInner::Vector { .. }
+            | crate::TypeInner::Matrix { .. }
+            | crate::TypeInner::ValuePointer { .. }
+            | crate::TypeInner::Atomic { .. }
+            | crate::TypeInner::Image { .. }
+            | crate::TypeInner::Sampler { .. }
+            | crate::TypeInner::AccelerationStructure
+            | crate::TypeInner::RayQuery => None,
+            crate::TypeInner::Pointer { base, space: _ } => {
+                handle.check_dep(base)?;
+                None
+            }
+            crate::TypeInner::Array { base, size, .. }
+            | crate::TypeInner::BindingArray { base, size, .. } => {
+                handle.check_dep(base)?;
+                match size {
+                    crate::ArraySize::Pending(pending) => match pending {
+                        crate::PendingArraySize::Expression(expr) => Some(expr),
+                        crate::PendingArraySize::Override(_) => None,
+                    },
+                    crate::ArraySize::Constant(_) | crate::ArraySize::Dynamic => None,
+                }
+            }
+            crate::TypeInner::Struct {
+                ref members,
+                span: _,
+            } => {
+                handle.check_dep_iter(members.iter().map(|m| m.ty))?;
+                None
+            }
+        };
+
+        Ok(max_expr)
+    }
+
+    /// Validate all handles that occur in `expression`, whose handle is `handle`.
+    ///
+    /// If `expression` refers to any `Type`s, return the highest-indexed type
+    /// handle that it uses. This is used for detecting cycles between the
+    /// expression and type arenas.
     fn validate_const_expression_handles(
         (handle, expression): (Handle<crate::Expression>, &crate::Expression),
         constants: &Arena<crate::Constant>,
         overrides: &Arena<crate::Override>,
-        types: &UniqueArena<crate::Type>,
-    ) -> Result<(), InvalidHandleError> {
+    ) -> Result<Option<Handle<crate::Type>>, InvalidHandleError> {
         let validate_constant = |handle| Self::validate_constant_handle(handle, constants);
         let validate_override = |handle| Self::validate_override_handle(handle, overrides);
-        let validate_type = |handle| Self::validate_type_handle(handle, types);
 
-        match *expression {
-            crate::Expression::Literal(_) => {}
+        let max_type = match *expression {
+            crate::Expression::Literal(_) => None,
             crate::Expression::Constant(constant) => {
                 validate_constant(constant)?;
                 handle.check_dep(constants[constant].init)?;
+                None
             }
             crate::Expression::Override(override_) => {
                 validate_override(override_)?;
                 if let Some(init) = overrides[override_].init {
                     handle.check_dep(init)?;
                 }
+                None
             }
-            crate::Expression::ZeroValue(ty) => {
-                validate_type(ty)?;
-            }
+            crate::Expression::ZeroValue(ty) => Some(ty),
             crate::Expression::Compose { ty, ref components } => {
-                validate_type(ty)?;
                 handle.check_dep_iter(components.iter().copied())?;
+                Some(ty)
             }
-            _ => {}
-        }
-        Ok(())
+            _ => None,
+        };
+        Ok(max_type)
     }
 
     #[allow(clippy::too_many_arguments)]
@@ -783,8 +884,47 @@ fn constant_deps() {
             handle_and_expr,
             &constants,
             &overrides,
-            &types,
         )
         .is_err());
     }
 }
+
+#[test]
+fn override_deps() {
+    use super::Validator;
+    use crate::{ArraySize, Expression, PendingArraySize, Scalar, Span, Type, TypeInner};
+
+    let nowhere = Span::default();
+
+    let mut m = crate::Module::default();
+
+    let ty_u32 = m.types.insert(
+        Type {
+            name: Some("u32".to_string()),
+            inner: TypeInner::Scalar(Scalar::U32),
+        },
+        nowhere,
+    );
+    let ex_zero = m
+        .global_expressions
+        .append(Expression::ZeroValue(ty_u32), nowhere);
+    let ty_arr = m.types.insert(
+        Type {
+            name: Some("bad_array".to_string()),
+            inner: TypeInner::Array {
+                base: ty_u32,
+                size: ArraySize::Pending(PendingArraySize::Expression(ex_zero)),
+                stride: 4,
+            },
+        },
+        nowhere,
+    );
+
+    // Everything should be okay now.
+    assert!(Validator::validate_module_handles(&m).is_ok());
+
+    // Mutate `ex_zero`'s type to `ty_arr`, introducing a cycle.
+    // Validation should catch the cycle.
+    m.global_expressions[ex_zero] = Expression::ZeroValue(ty_arr);
+    assert!(Validator::validate_module_handles(&m).is_err());
+}