wgpu icon indicating copy to clipboard operation
wgpu copied to clipboard

Naga doesn't check that subexpressions have been emitted.

Open jimblandy opened this issue 1 month ago • 2 comments

Naga doesn't check that each subexpression of an expression used by a statement has been covered by an Emit statement.

When added to naga/tests/validation.rs, the following test should pass, but it fails:

/// Validation should reject expressions that refer to un-emitted
/// subexpressions.
#[test]
fn emit_subexpressions() {
    fn variant(
        emit: bool,
    ) -> Result<naga::valid::ModuleInfo, naga::WithSpan<naga::valid::ValidationError>> {
        let span = naga::Span::default();
        let mut module = Module::default();
        let ty_u32 = module.types.insert(
            Type {
                name: Some("u32".into()),
                inner: TypeInner::Scalar(Scalar::U32),
            },
            span,
        );
        let var_private = module.global_variables.append(
            naga::GlobalVariable {
                name: Some("private".into()),
                space: naga::AddressSpace::Private,
                binding: None,
                ty: ty_u32,
                init: None,
            },
            span,
        );

        let mut fun = Function::default();

        // These expressions are pre-emit, so they don't need to be
        // covered by any `Emit` statement.
        let ex_var = fun.expressions.append(Expression::GlobalVariable(var_private), span);
        let ex_42 = fun.expressions.append(Expression::Literal(naga::Literal::U32(42)), span);

        // This expression is neither pre-emit nor used directly by a
        // statement. We want to test whether validation notices when
        // it is not covered by an `Emit` statement.
        let ex_add = fun.expressions.append(Expression::Binary {
            op: naga::BinaryOperator::Add,
            left: ex_42,
            right: ex_42,
        }, span);

        // This expression is used directly by the statement, so if
        // it's not covered by an `Emit`, then validation will catch
        // that.
        let ex_mul = fun.expressions.append(Expression::Binary {
            op: naga::BinaryOperator::Multiply,
            left: ex_add,
            right: ex_add,
        }, span);

        if emit {
            // This `Emit` covers all expressions properly.
            fun.body.push(naga::Statement::Emit(naga::Range::new_from_bounds(ex_add, ex_mul)), span);
        } else {
            // This `Emit` covers `ex_mul` but not its subexpression `ex_add`.
            fun.body.push(naga::Statement::Emit(naga::Range::new_from_bounds(ex_mul, ex_mul)), span);
        }
        fun.body.push(naga::Statement::Store {
            pointer: ex_var,
            value: ex_mul,
        }, span);

        module.functions.append(fun, span);

        valid::Validator::new(
            valid::ValidationFlags::default(),
            valid::Capabilities::all(),
        )
        .validate(&module)
    }

    variant(true).expect("module should validate");
    variant(false).expect_err("validation should notice un-emitted subexpression");
}

jimblandy avatar Jun 01 '24 00:06 jimblandy