Skip to content

Commit

Permalink
Get rid of the wasm interface types return pointer hacks
Browse files Browse the repository at this point in the history 
Now that using standard Wasm interface types implies multi-value support, we
don't need these return pointer hacks anymore.
  • Loading branch information
@fitzgen
fitzgen committed Oct 3, 2019
1 parent 311ae19 commit 25dd84c
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 95 deletions.
55 changes: 18 additions & 37 deletions crates/cli-support/src/webidl/mod.rs
View file
Expand Up @@ -954,7 +954,10 @@ impl<'a> Context<'a> {
match op.kind {
decode::OperationKind::Regular => {
if op.is_static {
Ok((AuxImport::ValueWithThis(class, function.name.to_string()), false))
Ok((
AuxImport::ValueWithThis(class, function.name.to_string()),
false,
))
} else if structural {
Ok((
AuxImport::StructuralMethod(function.name.to_string()),
Expand Down Expand Up @@ -1361,48 +1364,26 @@ impl<'a> Context<'a> {
.bindings
.get(bind.binding)
.ok_or_else(|| format_err!("bad binding id"))?;
let mut return_via_outptr = None;
let (wasm_ty, webidl_ty, incoming, outgoing) = match binding {
ast::FunctionBinding::Export(e) => {
// This `match` is weird, see the comment at the top of
// `standard.rs` for what it's doing.
let outgoing = match e.result.bindings.get(0) {
Some(ast::OutgoingBindingExpression::As(a)) if a.idx == u32::max_value() => {
return_via_outptr = Some(vec![walrus::ValType::I32, walrus::ValType::I32]);
&e.result.bindings[1..]
}
_ => &e.result.bindings[..],
};
(
e.wasm_ty,
e.webidl_ty,
e.params.bindings.as_slice(),
outgoing,
)
}
ast::FunctionBinding::Import(e) => {
// This `match` is weird, see the comment at the top of
// `standard.rs` for what it's doing.
let incoming = match e.result.bindings.get(0) {
Some(ast::IncomingBindingExpression::Get(g)) if g.idx == u32::max_value() => {
return_via_outptr = Some(vec![walrus::ValType::I32, walrus::ValType::I32]);
&e.result.bindings[1..]
}
_ => &e.result.bindings[..],
};
(
e.wasm_ty,
e.webidl_ty,
incoming,
e.params.bindings.as_slice(),
)
}
ast::FunctionBinding::Export(e) => (
e.wasm_ty,
e.webidl_ty,
e.params.bindings.as_slice(),
&e.result.bindings[..],
),
ast::FunctionBinding::Import(e) => (
e.wasm_ty,
e.webidl_ty,
&e.result.bindings[..],
e.params.bindings.as_slice(),
),
};
let webidl_ty = standard::copy_ty(&mut self.bindings.types, webidl_ty, &std.types);
let webidl_ty = match webidl_ty {
ast::WebidlTypeRef::Id(id) => <ast::WebidlFunction as ast::WebidlTypeId>::wrap(id),
_ => bail!("invalid webidl type listed"),
};

Ok(Binding {
wasm_ty,
webidl_ty,
Expand All @@ -1416,7 +1397,7 @@ impl<'a> Context<'a> {
.cloned()
.map(NonstandardOutgoing::Standard)
.collect(),
return_via_outptr,
return_via_outptr: None,
})
}

Expand Down
62 changes: 4 additions & 58 deletions crates/cli-support/src/webidl/standard.rs
View file
Expand Up @@ -16,39 +16,6 @@
//! generating any JS glue. Any JS glue currently generated is also invalid if
//! the module contains the wasm bindings section and it's actually respected.

// NB: Returning strings is weird
//
// This module has what is currently a pretty gross hack for dealing with
// returning strings. One of the banner features of WebIDL bindings is not
// requiring any language-specific glue to use wasm files and you get all sorts
// of types like integers and strings by default. Note that strings are a huge
// thing here.
//
// Dealing with *incoming* strings is easy enough in that the binding expression
// has an allocator function to call and it's filled in and passed as two
// pointers. Outgoing strings are a little harder, however, for two reasons:
//
// * One is that we need to return two values, which requires multi-value
// * Another is that someone's got to free the string at some point
//
// Rust/wasm-bindgen don't support multi-value, and WebIDL bindings as literally
// spec'd right this red hot second don't support freeing strings that we pass
// out. These both have obvious fixes (supporting multi-value and extending the
// bindings spec to support freeing) but we also want something working right
// this red-hot second.
//
// To get things working we employ a terrible hack where the first bindign
// expression of the result a function may indicate "use a thing that's way off
// in the ether" and that's actually a sentinel for "oh ignore everything else
// and the string is returned through an out-ptr as the first argument". This
// manifests in all sorts of special hacks all over the place to handle this,
// and it's a real bummer.
//
// This is in general just an explainer for the current state of things and
// also a preemptive apology for writing the code to handle this in so many
// places. I, like you, look forward to actually fixing this for real as the
// spec continues to evolve and we implement more in wasm-bindgen.

use crate::descriptor::VectorKind;
use crate::webidl::{AuxExportKind, AuxImport, AuxValue, JsImport, JsImportName};
use crate::webidl::{NonstandardIncoming, NonstandardOutgoing};
Expand Down Expand Up @@ -240,26 +207,15 @@ pub fn add_section(
name
)
})?;
let mut result = extract_outgoing(&binding.outgoing).with_context(|_| {
let result = extract_outgoing(&binding.outgoing).with_context(|_| {
format!(
"failed to map return value for export `{}` to standard \
binding expressions",
name
)
})?;

// see comment at top of this module about returning strings for what
// this is doing and why it's weird
if binding.return_via_outptr.is_some() {
result.insert(
0,
ast::OutgoingBindingExpressionAs {
idx: u32::max_value(),
ty: ast::WebidlScalarType::Long.into(),
}
.into(),
);
}
assert!(binding.return_via_outptr.is_none());
let binding = section.bindings.insert(ast::ExportBinding {
wasm_ty: binding.wasm_ty,
webidl_ty: copy_ty(
Expand Down Expand Up @@ -293,24 +249,14 @@ pub fn add_section(
module_name, name,
)
})?;
let mut result = extract_incoming(&binding.incoming).with_context(|_| {
let result = extract_incoming(&binding.incoming).with_context(|_| {
format!(
"failed to map return value of import `{}::{}` to standard \
binding expressions",
module_name, name,
)
})?;
// see comment at top of this module about returning strings for what
// this is doing and why it's weird
if binding.return_via_outptr.is_some() {
result.insert(
0,
ast::IncomingBindingExpressionGet {
idx: u32::max_value(),
}
.into(),
);
}
assert!(binding.return_via_outptr.is_none());
let binding = section.bindings.insert(ast::ImportBinding {
wasm_ty: binding.wasm_ty,
webidl_ty: copy_ty(
Expand Down

0 comments on commit 25dd84c

Please sign in to comment.