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.

Sign up for GitHub

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 issue 1057 + improve llvm/state.rs code #1058

Merged
merged 3 commits into from
Dec 12, 2019
Merged

Fix issue 1057 + improve llvm/state.rs code #1058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6189c9a
rename some variable, add more precise error message, use checked_sub…
pventuzelo Dec 11, 2019
aa13337
add changelog entry
pventuzelo Dec 11, 2019
b84aff7
Merge branch 'master' into ventuzelo/fix-1057-substracte-overflow-peek1
syrusakbary Dec 12, 2019
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## **[Unreleased]**

- [#1058](https://github.com/wasmerio/wasmer/pull/1058) Fix minor panic issue when `wasmer::compile_with` called with llvm backend.
- [#858](https://github.com/wasmerio/wasmer/pull/858) Minor panic fix when wasmer binary with `loader` option run a module without exported `_start` function.
- [#1056](https://github.com/wasmerio/wasmer/pull/1056) Improved `--invoke` args parsing (supporting `i32`, `i64`, `f32` and `f32`) in Wasmer CLI
- [#1054](https://github.com/wasmerio/wasmer/pull/1054) Improve `--invoke` output in Wasmer CLI
Expand Down
65 changes: 34 additions & 31 deletions lib/llvm-backend/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,33 +231,41 @@ impl<'ctx> State<'ctx> {

pub fn outermost_frame(&self) -> Result<&ControlFrame<'ctx>, BinaryReaderError> {
self.control_stack.get(0).ok_or(BinaryReaderError {
message: "invalid control stack depth",
message: "outermost_frame: invalid control stack depth",
offset: -1isize as usize,
})
}

pub fn frame_at_depth(&self, depth: u32) -> Result<&ControlFrame<'ctx>, BinaryReaderError> {
let index = self.control_stack.len() - 1 - (depth as usize);
self.control_stack.get(index).ok_or(BinaryReaderError {
message: "invalid control stack depth",
offset: -1isize as usize,
})
let index = self
.control_stack
.len()
.checked_sub(1 + (depth as usize))
Copy link
Contributor

Choose a reason for hiding this comment

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

.checked_sub(1)
.checked_sub(depth as usize)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not possible because checked_sub return an Option.

Do you prefer this way?

        let tmp_index = self
            .control_stack
            .len()
            .checked_sub(1).ok_or(BinaryReaderError {
                message: "frame_at_depth: invalid control stack depth",
                offset: -1isize as usize,
            })?;
        let index = tmp_index.checked_sub(depth as usize)
            .ok_or(BinaryReaderError {
                message: "frame_at_depth: invalid control stack depth",
                offset: -1isize as usize,
            })?;
        Ok(&self.control_stack[index])

Copy link
Contributor

Choose a reason for hiding this comment

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

Doh! No, I prefer the code as it is, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be possible to use .and_then here. I noticed the potential for overflow there too, but I figured it's very unlikely. I think it should be fine as-is!

For future readers who may end up here who didn't understand what I meant:

self
            .control_stack
            .len()
            .checked_sub(1)
            .and_then(|n| n.checked_sub(depth as usize))
            .ok_or(BinaryReaderError {
                message: "frame_at_depth: invalid control stack depth",
                offset: -1isize as usize,
            })?;

and_then acts like Monadic bind (>>=) in Haskell.

.ok_or(BinaryReaderError {
message: "frame_at_depth: invalid control stack depth",
offset: -1isize as usize,
})?;
Ok(&self.control_stack[index])
}

pub fn frame_at_depth_mut(
&mut self,
depth: u32,
) -> Result<&mut ControlFrame<'ctx>, BinaryReaderError> {
let index = self.control_stack.len() - 1 - (depth as usize);
self.control_stack.get_mut(index).ok_or(BinaryReaderError {
message: "invalid control stack depth",
offset: -1isize as usize,
})
let index = self
.control_stack
.len()
.checked_sub(1 + (depth as usize))
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too.

.ok_or(BinaryReaderError {
message: "frame_at_depth_mut: invalid control stack depth",
offset: -1isize as usize,
})?;
Ok(&mut self.control_stack[index])
}

pub fn pop_frame(&mut self) -> Result<ControlFrame<'ctx>, BinaryReaderError> {
self.control_stack.pop().ok_or(BinaryReaderError {
message: "cannot pop from control stack",
message: "pop_frame: cannot pop from control stack",
offset: -1isize as usize,
})
}
Expand All @@ -283,7 +291,7 @@ impl<'ctx> State<'ctx> {

pub fn pop1_extra(&mut self) -> Result<(BasicValueEnum<'ctx>, ExtraInfo), BinaryReaderError> {
self.stack.pop().ok_or(BinaryReaderError {
message: "invalid value stack",
message: "pop1_extra: invalid value stack",
offset: -1isize as usize,
})
}
Expand Down Expand Up @@ -327,13 +335,11 @@ impl<'ctx> State<'ctx> {
}

pub fn peek1_extra(&self) -> Result<(BasicValueEnum<'ctx>, ExtraInfo), BinaryReaderError> {
self.stack
.get(self.stack.len() - 1)
.ok_or(BinaryReaderError {
message: "invalid value stack",
offset: -1isize as usize,
})
.map(|v| *v)
let index = self.stack.len().checked_sub(1).ok_or(BinaryReaderError {
message: "peek1_extra: invalid value stack",
offset: -1isize as usize,
})?;
Ok(self.stack[index])
}

pub fn peekn(&self, n: usize) -> Result<Vec<BasicValueEnum<'ctx>>, BinaryReaderError> {
Expand All @@ -344,12 +350,12 @@ impl<'ctx> State<'ctx> {
&self,
n: usize,
) -> Result<&[(BasicValueEnum<'ctx>, ExtraInfo)], BinaryReaderError> {
let new_len = self.stack.len().checked_sub(n).ok_or(BinaryReaderError {
message: "invalid value stack",
let index = self.stack.len().checked_sub(n).ok_or(BinaryReaderError {
message: "peekn_extra: invalid value stack",
offset: -1isize as usize,
})?;

Ok(&self.stack[new_len..])
Ok(&self.stack[index..])
}

pub fn popn_save_extra(
Expand All @@ -362,15 +368,12 @@ impl<'ctx> State<'ctx> {
}

pub fn popn(&mut self, n: usize) -> Result<(), BinaryReaderError> {
if self.stack.len() < n {
return Err(BinaryReaderError {
message: "invalid value stack",
offset: -1isize as usize,
});
}
let index = self.stack.len().checked_sub(n).ok_or(BinaryReaderError {
message: "popn: invalid value stack",
offset: -1isize as usize,
})?;

let new_len = self.stack.len() - n;
self.stack.truncate(new_len);
self.stack.truncate(index);
Ok(())
}

Expand Down