-
Notifications
You must be signed in to change notification settings - Fork 11
Introduce dropsection #72
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
Conversation
| use parity_wasm::elements::*; | ||
|
|
||
| /// Enum on which ModuleTranslator is implemented. | ||
| pub enum DropSection<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably will rewrite to have this as DropSectionKind and DropSection as a vector of DropSectionKind. In that case the entire code can be rewritten somewhat, potentially with a single loop, though one must be careful when removing sections with a lower index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this rewrite I could also introduce with_preset with ewasm which removes the namessection and all custom sections with names debug.
libchisel/src/dropsection.rs
Outdated
|
|
||
| let sections = module.sections_mut(); | ||
| if index > sections.len() { | ||
| return Err(ModuleError::Custom("Not found.".to_string())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this should be considered an error or marked as Ok(false).
Potentially we need a flag in the translator configuration saying whether fields not found is an error or not and behave accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is no case where Ok(false) is returned and false indicates no mutation, it probably makes sense that this should be OK. Although a flag would make it more versatile.
libchisel/src/dropsection.rs
Outdated
| impl<'a> DropSection<'a> { | ||
| fn find_index(&self, module: &Module) -> Result<usize, String> { | ||
| Ok(match &self { | ||
| DropSection::NamesSection => names_section_index_for(module)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was named _for because it had a section enum as a parameter to be generic. If we keep these helpers, they need better naming.
| pub enum DropSection<'a> { | ||
| NamesSection, | ||
| /// Name of the custom section. | ||
| CustomSectionByName(&'a String), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps another mode for DropSection would be identifying data sections that are used for rust debugging and panic messages.
Identifying these is not exactly straightforward, but one option is just using a plain regex lookup for things like libcore in the data sections. Not sure what the risk of false positives for this would be.
|
@jakelang where do we stand with this? |
0c62c18 to
da86747
Compare
Implements #48.