Skip to content

Add attempt for better module configuration #161

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

Merged
merged 14 commits into from
Jan 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion libchisel/src/binaryenopt.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use std::collections::HashMap;

use parity_wasm::elements::Module;

use super::{ChiselModule, ModuleError, ModuleKind, ModulePreset, ModuleTranslator};
use super::{ChiselModule, ModuleConfig, ModuleError, ModuleKind, ModulePreset, ModuleTranslator};

// FIXME: change level names
pub enum BinaryenOptimiser {
Expand Down Expand Up @@ -29,6 +31,20 @@ impl<'a> ChiselModule<'a> for BinaryenOptimiser {
}
}

impl ModuleConfig for BinaryenOptimiser {
fn with_defaults() -> Result<Self, ModuleError> {
Ok(BinaryenOptimiser::O2)
}

fn with_config(config: &HashMap<String, String>) -> Result<Self, ModuleError> {
if let Some(preset) = config.get("preset") {
BinaryenOptimiser::with_preset(preset)
} else {
Err(ModuleError::NotSupported)
}
}
}

impl ModulePreset for BinaryenOptimiser {
fn with_preset(preset: &str) -> Result<Self, ModuleError> {
match preset {
Expand Down
14 changes: 13 additions & 1 deletion libchisel/src/checkfloat.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use std::collections::HashMap;

use parity_wasm::elements::{Instruction, Module};

use super::{ChiselModule, ModuleError, ModuleKind, ModuleValidator};
use super::{ChiselModule, ModuleConfig, ModuleError, ModuleKind, ModuleValidator};

/// Struct on which ModuleValidator is implemented.
pub struct CheckFloat {}
Expand All @@ -21,6 +23,16 @@ impl<'a> ChiselModule<'a> for CheckFloat {
}
}

impl ModuleConfig for CheckFloat {
fn with_defaults() -> Result<Self, ModuleError> {
Ok(CheckFloat {})
}

fn with_config(_config: &HashMap<String, String>) -> Result<Self, ModuleError> {
Err(ModuleError::NotSupported)
}
}

impl CheckFloat {
pub fn new() -> Self {
CheckFloat {}
Expand Down
21 changes: 20 additions & 1 deletion libchisel/src/checkstartfunc.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use std::collections::HashMap;

use parity_wasm::elements::Module;

use super::{ChiselModule, ModuleError, ModuleKind, ModuleValidator};
use super::{ChiselModule, ModuleConfig, ModuleError, ModuleKind, ModuleValidator};

/// Struct on which ModuleValidator is implemented.
pub struct CheckStartFunc {
Expand Down Expand Up @@ -31,6 +33,23 @@ impl<'a> ChiselModule<'a> for CheckStartFunc {
}
}

impl ModuleConfig for CheckStartFunc {
fn with_defaults() -> Result<Self, ModuleError> {
Err(ModuleError::NotSupported)
}

fn with_config(config: &HashMap<String, String>) -> Result<Self, ModuleError> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also split this into two (and that would remove the need for default):

  • "with config string"
  • "with complex config"

e.g.

fn with_config_string(config: &str)
fn with_config_map(config: &HashMap<String, String>)

and then with_config_string(&"") would mean "defaults"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think key-val configuration should be fine for now. No need to further complicate things with the config string, and we can definitely just implement the config string as a specified key config or something in the hashmap.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but the downside is the terrible syntax needed for creating a hashmap and passing it. So for simple modules a string is much more simple.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the config string method could just wrap that logic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by wrap? Define a way to encode hashmaps?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with_config_string would just call with_config where a dedicated key such as config_string is set to the value provided.

let require_start = if let Some(value) = config.get("require_start") {
value == "true"
} else {
false
};
Ok(CheckStartFunc {
start_required: require_start,
})
}
}

impl ModuleValidator for CheckStartFunc {
fn validate(&self, module: &Module) -> Result<bool, ModuleError> {
Ok(module.start_section().is_some() == self.start_required)
Expand Down
18 changes: 17 additions & 1 deletion libchisel/src/deployer.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use std::collections::HashMap;

use parity_wasm::builder;
use parity_wasm::elements::{CustomSection, Module};

use super::{ChiselModule, ModuleError, ModuleKind, ModulePreset, ModuleTranslator};
use super::{ChiselModule, ModuleConfig, ModuleError, ModuleKind, ModulePreset, ModuleTranslator};

/// Enum on which ModuleTranslator is implemented.
pub enum Deployer {
Expand All @@ -25,6 +27,20 @@ impl<'a> ChiselModule<'a> for Deployer {
}
}

impl ModuleConfig for Deployer {
fn with_defaults() -> Result<Self, ModuleError> {
Err(ModuleError::NotSupported)
}

fn with_config(config: &HashMap<String, String>) -> Result<Self, ModuleError> {
if let Some(preset) = config.get("preset") {
Deployer::with_preset(preset)
} else {
Err(ModuleError::NotSupported)
}
}
}

impl ModulePreset for Deployer {
fn with_preset(preset: &str) -> Result<Self, ModuleError> {
match preset {
Expand Down
71 changes: 70 additions & 1 deletion libchisel/src/dropsection.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
use std::collections::HashMap;
use std::error::Error;

use parity_wasm::elements::{Module, Section};

use super::{ChiselModule, ModuleError, ModuleKind, ModuleTranslator};
use super::{ChiselModule, ModuleConfig, ModuleError, ModuleKind, ModuleTranslator};

/// Enum on which ModuleTranslator is implemented.
#[derive(Debug)]
pub enum DropSection {
NamesSection,
/// Name of the custom section.
Expand All @@ -29,6 +33,58 @@ impl<'a> ChiselModule<'a> for DropSection {
}
}

impl From<std::num::ParseIntError> for ModuleError {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could make this more generic, and support all kinds of errors:

impl From<E: Error> for ModuleError {
    fn from(error: E) -> Self {
        ModuleError::Custom(error.description().to_string())
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, though I was considering here to wrap it with some text or use NotSupported.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently this is unstable: rust-lang/rust#52662

Copy link
Collaborator

@jakelang jakelang Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have this blanket impl and then have specialized impls for specific errors

fn from(error: std::num::ParseIntError) -> Self {
ModuleError::Custom(error.description().to_string())
}
}

impl ModuleConfig for DropSection {
fn with_defaults() -> Result<Self, ModuleError> {
Err(ModuleError::NotSupported)
}

fn with_config(config: &HashMap<String, String>) -> Result<Self, ModuleError> {
// Query all possible modes
let modes: [(&'static str, Option<&String>); 4] = [
("names", config.get("names".into())),
("custom_by_name", config.get("custom_by_name".into())),
("custom_by_index", config.get("custom_by_index".into())),
("unknown_by_index", config.get("unknown_by_index".into())),
];

// Filter out modes which were provided.
let mut matches: Vec<(&'static str, &String)> = modes
.iter()
.filter_map(|(k, v)| {
if let Some(v) = v {
Some((*k, *v))
} else {
None
}
})
.collect();

// Reject multiple modes
if matches.len() != 1 {
return Err(ModuleError::Custom(
"Only one mode allowed at a time".to_string(),
));
}

let (mode, val) = matches.pop().expect("Verified that one match is present");
match mode {
"names" => Ok(DropSection::NamesSection),
"custom_by_name" => Ok(DropSection::CustomSectionByName(val.clone())),
"custom_by_index" => Ok(DropSection::CustomSectionByIndex(str::parse::<usize>(val)?)),
"unknown_by_index" => Ok(DropSection::UnknownSectionByIndex(str::parse::<usize>(
val,
)?)),
_ => panic!("Only one of the above was present in the array"),
}
}
}

// TODO: consider upstreaming this
fn custom_section_index_for(module: &Module, name: &str) -> Option<usize> {
module.sections().iter().position(|e| match e {
Expand Down Expand Up @@ -258,4 +314,17 @@ mod tests {
assert!(custom_section_index_for(&module, "name").is_none());
assert!(custom_section_index_for(&module1, "name").is_none());
}

#[test]
fn with_config_multiple_modes() {
let mut conf = HashMap::new();
conf.insert("names".to_string(), "".to_string());
conf.insert("custom_by_name".to_string(), "name".to_string());

let module = DropSection::with_config(&conf);
assert_eq!(
module.unwrap_err(),
ModuleError::Custom("Only one mode allowed at a time".to_string())
);
}
}
12 changes: 12 additions & 0 deletions libchisel/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
pub use parity_wasm::elements::Module;

use std::collections::HashMap;
use std::{error, fmt};

pub mod imports;
Expand Down Expand Up @@ -71,6 +72,17 @@ pub trait ModulePreset {
Self: std::marker::Sized;
}

// TODO: move this to be part of ChiselModule and retire ModulePreset
pub trait ModuleConfig {
fn with_defaults() -> Result<Self, ModuleError>
where
Self: std::marker::Sized;

fn with_config(config: &HashMap<String, String>) -> Result<Self, ModuleError>
where
Self: std::marker::Sized;
}

impl From<String> for ModuleError {
fn from(error: String) -> Self {
ModuleError::Custom(error)
Expand Down
19 changes: 18 additions & 1 deletion libchisel/src/remapimports.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use std::collections::HashMap;

use parity_wasm::elements::{ImportEntry, ImportSection, Module};

use super::{
imports::ImportList, ChiselModule, ModuleError, ModuleKind, ModulePreset, ModuleTranslator,
imports::ImportList, ChiselModule, ModuleConfig, ModuleError, ModuleKind, ModulePreset,
ModuleTranslator,
};

pub struct RemapImports<'a> {
Expand Down Expand Up @@ -29,6 +32,20 @@ impl<'a> ChiselModule<'a> for RemapImports<'a> {
}
}

impl<'a> ModuleConfig for RemapImports<'a> {
fn with_defaults() -> Result<Self, ModuleError> {
Err(ModuleError::NotSupported)
}

fn with_config(config: &HashMap<String, String>) -> Result<Self, ModuleError> {
if let Some(preset) = config.get("preset") {
RemapImports::with_preset(preset)
} else {
Err(ModuleError::NotSupported)
}
}
}

impl<'a> ModulePreset for RemapImports<'a> {
fn with_preset(preset: &str) -> Result<Self, ModuleError> {
let mut interface_set: Vec<ImportInterface> = Vec::new();
Expand Down
19 changes: 18 additions & 1 deletion libchisel/src/remapstart.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use std::collections::HashMap;

use parity_wasm::elements::{ExportEntry, ExportSection, Internal, Module, Section};

use super::{ChiselModule, ModuleError, ModuleKind, ModulePreset, ModuleTranslator};
use super::{ChiselModule, ModuleConfig, ModuleError, ModuleKind, ModulePreset, ModuleTranslator};

pub struct RemapStart;

Expand Down Expand Up @@ -30,6 +32,21 @@ impl<'a> ChiselModule<'a> for RemapStart {
}
}

impl ModuleConfig for RemapStart {
fn with_defaults() -> Result<Self, ModuleError> {
Ok(RemapStart {})
}

// FIXME: drop this, no need for preset here
fn with_config(config: &HashMap<String, String>) -> Result<Self, ModuleError> {
if let Some(preset) = config.get("preset") {
RemapStart::with_preset(preset)
} else {
Err(ModuleError::NotSupported)
}
}
}

impl ModuleTranslator for RemapStart {
fn translate_inplace(&self, module: &mut Module) -> Result<bool, ModuleError> {
Ok(remap_start(module))
Expand Down
14 changes: 13 additions & 1 deletion libchisel/src/repack.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use std::collections::HashMap;

use parity_wasm::builder;
use parity_wasm::elements::Module;

use super::{ChiselModule, ModuleError, ModuleKind, ModuleTranslator};
use super::{ChiselModule, ModuleConfig, ModuleError, ModuleKind, ModuleTranslator};

pub struct Repack;

Expand All @@ -27,6 +29,16 @@ impl<'a> ChiselModule<'a> for Repack {
}
}

impl ModuleConfig for Repack {
fn with_defaults() -> Result<Self, ModuleError> {
Ok(Repack::new())
}

fn with_config(_config: &HashMap<String, String>) -> Result<Self, ModuleError> {
Err(ModuleError::NotSupported)
}
}

impl ModuleTranslator for Repack {
fn translate_inplace(&self, _module: &mut Module) -> Result<bool, ModuleError> {
Err(ModuleError::NotSupported)
Expand Down
28 changes: 27 additions & 1 deletion libchisel/src/snip.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use std::collections::HashMap;

use parity_wasm::elements::Module;

use super::{ChiselModule, ModuleError, ModuleKind, ModuleTranslator};
use super::{ChiselModule, ModuleConfig, ModuleError, ModuleKind, ModuleTranslator};

#[derive(Clone)]
pub struct Snip(wasm_snip::Options);
Expand Down Expand Up @@ -32,6 +34,30 @@ impl<'a> ChiselModule<'a> for Snip {
}
}

// TODO: consider making this a generic helper?
fn check_bool_option(config: &HashMap<String, String>, option: &str, default: bool) -> bool {
if let Some(value) = config.get(option) {
value == "true"
} else {
default
}
}

impl ModuleConfig for Snip {
fn with_defaults() -> Result<Self, ModuleError> {
Ok(Snip::new())
}

fn with_config(config: &HashMap<String, String>) -> Result<Self, ModuleError> {
let mut options = wasm_snip::Options::default();
options.snip_rust_fmt_code = check_bool_option(&config, "snip_rust_fmt_code", true);
options.snip_rust_panicking_code =
check_bool_option(&config, "snip_rust_panicking_code", true);
options.skip_producers_section = check_bool_option(&config, "skip_producers_section", true);
Ok(Snip { 0: options })
}
}

impl From<failure::Error> for ModuleError {
fn from(error: failure::Error) -> Self {
ModuleError::Custom(error.to_string())
Expand Down
Loading