Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "duplicate identifier for field" for subtype fields #1117

Merged
merged 5 commits into from Jul 12, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 1 addition & 4 deletions crates/wasmparser/benches/benchmark.rs
Expand Up @@ -4,10 +4,7 @@ use once_cell::unsync::Lazy;
use std::fs;
use std::path::Path;
use std::path::PathBuf;
use wasmparser::{
DataKind, ElementKind, HeapType, Parser, Payload, ValType, Validator, VisitOperator,
WasmFeatures,
};
use wasmparser::{DataKind, ElementKind, Parser, Payload, Validator, VisitOperator, WasmFeatures};

/// A benchmark input.
pub struct BenchmarkInput {
Expand Down
43 changes: 36 additions & 7 deletions crates/wast/src/core/resolve/names.rs
Expand Up @@ -3,6 +3,7 @@ use crate::core::*;
use crate::names::{resolve_error, Namespace};
use crate::token::{Id, Index};
use crate::Error;
use std::collections::HashMap;

pub fn resolve<'a>(fields: &mut Vec<ModuleField<'a>>) -> Result<Resolver<'a>, Error> {
let mut resolver = Resolver::default();
Expand All @@ -25,7 +26,7 @@ pub struct Resolver<'a> {
tags: Namespace<'a>,
datas: Namespace<'a>,
elems: Namespace<'a>,
fields: Namespace<'a>,
fields: HashMap<u32, Namespace<'a>>,
type_info: Vec<TypeInfo<'a>>,
}

Expand All @@ -46,16 +47,41 @@ impl<'a> Resolver<'a> {
}

fn register_type(&mut self, ty: &Type<'a>) -> Result<(), Error> {
let type_index = self.types.register(ty.id, "type")?;

match &ty.def {
// For GC structure types we need to be sure to populate the
// field namespace here as well.
//
// The field namespace is global, but the resolved indices
// are relative to the struct they are defined in
// The field namespace is relative to the struct fields are defined in
TypeDef::Struct(r#struct) => {
for (i, field) in r#struct.fields.iter().enumerate() {
if let Some(id) = field.id {
self.fields.register_specific(id, i as u32, "field")?;
if let Some(parent_type_index) = ty
.parent
.map(|index| self.types.get_index(&index))
.flatten()
{
if let Some(parent_field_ns) = self.fields.get(&parent_type_index) {
let field_idx =
parent_field_ns.resolve(&mut Index::Id(id), "field")?;
if field_idx != i as u32 {
imikushin marked this conversation as resolved.
Show resolved Hide resolved
return Err(Error::new(
id.span(),
format!(
"field index mismatch for `{}`: expected {}, got {}",
id.name(),
field_idx,
i
),
));
}
}
}
self.fields
.entry(type_index)
.or_insert(Namespace::default())
.register_specific(id, i as u32, "field")?;
}
}
}
Expand All @@ -75,7 +101,6 @@ impl<'a> Resolver<'a> {
_ => self.type_info.push(TypeInfo::Other),
}

self.types.register(ty.id, "type")?;
Ok(())
}

Expand Down Expand Up @@ -610,8 +635,12 @@ impl<'a, 'b> ExprResolver<'a, 'b> {
}

StructSet(s) | StructGet(s) | StructGetS(s) | StructGetU(s) => {
self.resolver.resolve(&mut s.r#struct, Ns::Type)?;
self.resolver.fields.resolve(&mut s.field, "field")?;
let type_index = self.resolver.resolve(&mut s.r#struct, Ns::Type)?;
self.resolver
.fields
.get(&type_index)
.unwrap()
imikushin marked this conversation as resolved.
Show resolved Hide resolved
.resolve(&mut s.field, "field")?;
}

ArrayNewFixed(a) => {
Expand Down
20 changes: 15 additions & 5 deletions crates/wast/src/names.rs
Expand Up @@ -51,11 +51,13 @@ impl<'a> Namespace<'a> {
}

pub fn register_specific(&mut self, name: Id<'a>, index: u32, desc: &str) -> Result<(), Error> {
if let Some(_prev) = self.names.insert(name, index) {
return Err(Error::new(
name.span(),
format!("duplicate identifier for {}", desc),
));
if let Some(prev) = self.names.insert(name, index) {
if prev != index {
imikushin marked this conversation as resolved.
Show resolved Hide resolved
return Err(Error::new(
name.span(),
format!("duplicate identifier for {}", desc),
));
}
}
Ok(())
}
Expand All @@ -71,6 +73,14 @@ impl<'a> Namespace<'a> {
}
Err(resolve_error(*id, desc))
}

pub fn get_index(&self, idx: &Index<'a>) -> Option<u32> {
let id = match idx {
Index::Num(n, _) => return Some(*n),
Index::Id(id) => id,
};
self.names.get(id).copied()
}
}

pub fn resolve_error(id: Id<'_>, ns: &str) -> Error {
Expand Down
14 changes: 14 additions & 0 deletions tests/local/gc/gc-subtypes-invalid.wast
Expand Up @@ -129,3 +129,17 @@
)
"type index out of bounds"
)
(assert_invalid
(module
(type $A (struct (field $vt (mut i32))))
(type $C (sub $A (struct (field $tv (mut i32)))))
)
"unknown field"
)
imikushin marked this conversation as resolved.
Show resolved Hide resolved
(assert_invalid
(module
(type $A (struct (field $vt (mut i32))))
(type $C (sub $A (struct (field (mut i32)) (field $vt (mut i32)))))
)
"field index mismatch"
)
3 changes: 3 additions & 0 deletions tests/local/gc/gc-subtypes.wat
Expand Up @@ -97,4 +97,7 @@
(type $y0 (sub $v0 (array (ref noextern))))
(type $y01 (sub $u0 (array (ref noextern))))
(type $z0 (sub $u0 (array nullexternref)))

(type $A (struct (field $vt (mut i32))))
(type $B (sub $A (struct (field $vt (mut i32)))))
)
2 changes: 2 additions & 0 deletions tests/snapshots/local/gc/gc-subtypes.wat.print
Expand Up @@ -68,4 +68,6 @@
(type $y0 (;66;) (sub $v0 (;65;) (array (ref noextern))))
(type $y01 (;67;) (sub $u0 (;64;) (array (ref noextern))))
(type $z0 (;68;) (sub $u0 (;64;) (array nullexternref)))
(type $A (;69;) (struct (field (mut i32))))
(type $B (;70;) (sub $A (;69;) (struct (field (mut i32)))))
)