Skip to content

Improve offset_front() #785

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

Jump to bottom
Merged
merged 7 commits into from
Mar 20, 2025
Merged

Improve offset_front() #785

merged 7 commits into from
Mar 20, 2025

Conversation

zhouwfang
Copy link
Contributor

#46

@zhouwfang zhouwfang requested a review from ia0 as a code owner March 8, 2025 04:52
zhouwfang
zhouwfang commented Mar 8, 2025
View reviewed changes
ia0
ia0 reviewed Mar 10, 2025
View reviewed changes
@zhouwfang zhouwfang changed the title Make offset_front() generic Improve offset_front() Mar 12, 2025
@zhouwfang zhouwfang requested a review from ia0 March 12, 2025 03:10
@zhouwfang
Copy link
Contributor Author

zhouwfang commented Mar 12, 2025 •
edited
Loading

Could you first take a look at the new offset functions? I'll apply them later. Thanks.

ia0
ia0 reviewed Mar 12, 2025
View reviewed changes
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Looks good, I've simplified a bit.

@zhouwfang zhouwfang requested a review from ia0 March 15, 2025 22:54
@zhouwfang
Copy link
Contributor Author

After this PR is merged, can we merge dev/fast-interp into main?

ia0
ia0 approved these changes Mar 20, 2025
View reviewed changes
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks! I've changed the logic to have the information at the right place (essentially func_body should be in Expr).

crates/interpreter/src/exec.rs Outdated
side_table,
0,
#[cfg(feature = "toctou")]
&[],
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not parser.save()?

crates/interpreter/src/valid.rs Outdated
@@ -221,6 +224,7 @@ struct Context<'m, M: ValidMode> {
globals: Vec<GlobalType>,
elems: Vec<RefType>,
datas: Option<usize>,
func_body: &'m [u8],
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

crates/interpreter/src/valid.rs Outdated
// SAFETY: This function is only called after parsing an End instruction.
target.parser = offset_front(target.parser, -1);
// This function is only called after parsing an End instruction.
target.parser = offset_front_check(self.context.func_body, target.parser, -1)?;
Copy link
Member

Choose a reason for hiding this comment

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

We don't need a check here, since it's not based on user input.

@ia0
Copy link
Member

ia0 commented Mar 20, 2025

After this PR is merged, can we merge dev/fast-interp into main?

Yes, I'll create a PR from a fork of this branch in my repo to avoid the PR getting double CI (once for push and once for pull_request).

@ia0 ia0 merged commit 5995cd2 into google:dev/fast-interp Mar 20, 2025
21 checks passed
@zhouwfang
Copy link
Contributor Author

After this PR is merged, can we merge dev/fast-interp into main?

Yes, I'll create a PR from a fork of this branch in my repo to avoid the PR getting double CI (once for push and once for pull_request).

Do you mind explaining the differences in more detail? You are ahead of me. Thanks!

@ia0
Copy link
Member

ia0 commented Mar 20, 2025

Do you mind explaining the differences in more detail? You are ahead of me. Thanks!

#737 is the PR where I merged the dev/opensk branch into main. You can see (if you click View details near the end) that there were 40 checks, while in this PR it's 21. There were twice more checks because for each commit I pushed to the branch after the PR was created, a push for dev/opensk and a pull_request for main workflow was created. You can see this in the checks job (which appears twice like almost all other jobs):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ia0 ia0 ia0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zhouwfang @ia0